Bug 186014 - testair sometimes crashes due to races in initialization of ARC4RandomNumberGenerator
Summary: testair sometimes crashes due to races in initialization of ARC4RandomNumberG...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks: 186017
  Show dependency treegraph
 
Reported: 2018-05-26 12:32 PDT by Filip Pizlo
Modified: 2018-05-27 06:59 PDT (History)
12 users (show)

See Also:


Attachments
the patch (5.00 KB, patch)
2018-05-26 12:35 PDT, Filip Pizlo
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2018-05-26 12:32:52 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2018-05-26 12:35:58 PDT
Created attachment 341405 [details]
the patch
Comment 2 Yusuke Suzuki 2018-05-26 13:18:59 PDT
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.
Comment 3 Filip Pizlo 2018-05-26 13:53:11 PDT
(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.
Comment 4 Filip Pizlo 2018-05-26 13:55:33 PDT
(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.
Comment 5 Filip Pizlo 2018-05-26 13:56:07 PDT
(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.
Comment 6 Filip Pizlo 2018-05-26 13:57:19 PDT
(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.
Comment 7 Filip Pizlo 2018-05-26 13:59:30 PDT
Landed in https://trac.webkit.org/changeset/232227/webkit
Comment 8 Radar WebKit Bug Importer 2018-05-26 14:00:21 PDT
<rdar://problem/40581599>
Comment 9 Yusuke Suzuki 2018-05-27 06:52:59 PDT
(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). :)
Comment 10 Yusuke Suzuki 2018-05-27 06:54:25 PDT
(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.