Bug 186014 – testair sometimes crashes due to races in initialization of ARC4RandomNumberGenerator

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 341405 [details]
the patch

[Yusuke Suzuki]

Comment on attachment 341405 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341405&action=review

r=me I think WTF::RandomDevice would reuire similar change.

> Source/JavaScriptCore/runtime/JSString.cpp:58
> +        dataLog("Really destroying string.\n");

This part seems accidentally included.

> Source/WTF/wtf/CryptographicallyRandomNumber.cpp:162
> +    static ARC4RandomNumberGenerator* randomNumberGenerator;

I think LazyNeverDestroyed is better.

[Filip Pizlo]

(In reply to Yusuke Suzuki from comment #2)
> Comment on attachment 341405 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341405&action=review
> 
> r=me I think WTF::RandomDevice would reuire similar change.

Doesn't look like that uses NeverDestroyed<>.

> 
> > Source/JavaScriptCore/runtime/JSString.cpp:58
> > +        dataLog("Really destroying string.\n");
> 
> This part seems accidentally included.

Ooops!

> 
> > Source/WTF/wtf/CryptographicallyRandomNumber.cpp:162
> > +    static ARC4RandomNumberGenerator* randomNumberGenerator;
> 
> I think LazyNeverDestroyed is better.

I'll try that.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #3)
> (In reply to Yusuke Suzuki from comment #2)
> > Comment on attachment 341405 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=341405&action=review
> > 
> > r=me I think WTF::RandomDevice would reuire similar change.
> 
> Doesn't look like that uses NeverDestroyed<>.
> 
> > 
> > > Source/JavaScriptCore/runtime/JSString.cpp:58
> > > +        dataLog("Really destroying string.\n");
> > 
> > This part seems accidentally included.
> 
> Ooops!
> 
> > 
> > > Source/WTF/wtf/CryptographicallyRandomNumber.cpp:162
> > > +    static ARC4RandomNumberGenerator* randomNumberGenerator;
> > 
> > I think LazyNeverDestroyed is better.
> 
> I'll try that.

LazyNeverDestroyed has the same issue.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #4)
> (In reply to Filip Pizlo from comment #3)
> > (In reply to Yusuke Suzuki from comment #2)
> > > Comment on attachment 341405 [details]
> > > the patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=341405&action=review
> > > 
> > > r=me I think WTF::RandomDevice would reuire similar change.
> > 
> > Doesn't look like that uses NeverDestroyed<>.
> > 
> > > 
> > > > Source/JavaScriptCore/runtime/JSString.cpp:58
> > > > +        dataLog("Really destroying string.\n");
> > > 
> > > This part seems accidentally included.
> > 
> > Ooops!
> > 
> > > 
> > > > Source/WTF/wtf/CryptographicallyRandomNumber.cpp:162
> > > > +    static ARC4RandomNumberGenerator* randomNumberGenerator;
> > > 
> > > I think LazyNeverDestroyed is better.
> > 
> > I'll try that.
> 
> LazyNeverDestroyed has the same issue.

To clarify: it has the same race as NeverDestroyed.  So, using that would not fix the bug.

[Filip Pizlo]

(In reply to Filip Pizlo from comment #5)
> (In reply to Filip Pizlo from comment #4)
> > (In reply to Filip Pizlo from comment #3)
> > > (In reply to Yusuke Suzuki from comment #2)
> > > > Comment on attachment 341405 [details]
> > > > the patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=341405&action=review
> > > > 
> > > > r=me I think WTF::RandomDevice would reuire similar change.
> > > 
> > > Doesn't look like that uses NeverDestroyed<>.
> > > 
> > > > 
> > > > > Source/JavaScriptCore/runtime/JSString.cpp:58
> > > > > +        dataLog("Really destroying string.\n");
> > > > 
> > > > This part seems accidentally included.
> > > 
> > > Ooops!
> > > 
> > > > 
> > > > > Source/WTF/wtf/CryptographicallyRandomNumber.cpp:162
> > > > > +    static ARC4RandomNumberGenerator* randomNumberGenerator;
> > > > 
> > > > I think LazyNeverDestroyed is better.
> > > 
> > > I'll try that.
> > 
> > LazyNeverDestroyed has the same issue.
> 
> To clarify: it has the same race as NeverDestroyed.  So, using that would
> not fix the bug.

Wait, do you mean using LazyNeverDestroyed instead of pointing to it?  Took me a while to figure that out.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/232227/webkit

[Radar WebKit Bug Importer]

<rdar://problem/40581599>

[Yusuke Suzuki]

(In reply to Filip Pizlo from comment #6)
> (In reply to Filip Pizlo from comment #5)
> > (In reply to Filip Pizlo from comment #4)
> > > (In reply to Filip Pizlo from comment #3)
> > > > (In reply to Yusuke Suzuki from comment #2)
> > > > > Comment on attachment 341405 [details]
> > > > > the patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=341405&action=review
> > > > > 
> > > > > r=me I think WTF::RandomDevice would reuire similar change.
> > > > 
> > > > Doesn't look like that uses NeverDestroyed<>.
> > > > 
> > > > > 
> > > > > > Source/JavaScriptCore/runtime/JSString.cpp:58
> > > > > > +        dataLog("Really destroying string.\n");
> > > > > 
> > > > > This part seems accidentally included.
> > > > 
> > > > Ooops!
> > > > 
> > > > > 
> > > > > > Source/WTF/wtf/CryptographicallyRandomNumber.cpp:162
> > > > > > +    static ARC4RandomNumberGenerator* randomNumberGenerator;
> > > > > 
> > > > > I think LazyNeverDestroyed is better.
> > > > 
> > > > I'll try that.
> > > 
> > > LazyNeverDestroyed has the same issue.
> > 
> > To clarify: it has the same race as NeverDestroyed.  So, using that would
> > not fix the bug.
> 
> Wait, do you mean using LazyNeverDestroyed instead of pointing to it?  Took
> me a while to figure that out.

Yeah, I mean using LazyNeverDestroyed<> instead of pointer (and calling construct() in call_once). :)

[Yusuke Suzuki]

(In reply to Filip Pizlo from comment #3)
> (In reply to Yusuke Suzuki from comment #2)
> > Comment on attachment 341405 [details]
> > the patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=341405&action=review
> > 
> > r=me I think WTF::RandomDevice would reuire similar change.
> 
> Doesn't look like that uses NeverDestroyed<>.
> 
> > 
> > > Source/JavaScriptCore/runtime/JSString.cpp:58
> > > +        dataLog("Really destroying string.\n");
> > 
> > This part seems accidentally included.
> 
> Ooops!
> 
> > 
> > > Source/WTF/wtf/CryptographicallyRandomNumber.cpp:162
> > > +    static ARC4RandomNumberGenerator* randomNumberGenerator;
> > 
> > I think LazyNeverDestroyed is better.
> 
> I'll try that.

RandomDevice one is OSRandomSource.cpp's NeverDestroyed<RandomDevice>. I think it has similar problem.