186014 2018-05-26 12:32:52 -0700 testair sometimes crashes due to races in initialization of ARC4RandomNumberGenerator 2018-05-27 06:59:01 -0700 1 1 1 Unclassified WebKit Web Template Framework WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 186017 1 fpizlo fpizlo benjamin cdumez cmarcelo dbates ews-watchlist ggaren keith_miller mark.lam msaboff saam webkit-bug-importer ysuzuki oldest_to_newest 1427750 0 fpizlo 2018-05-26 12:32:52 -0700 Patch forthcoming. 1427751 1 341405 fpizlo 2018-05-26 12:35:58 -0700 Created attachment 341405 the patch 1427755 2 341405 ysuzuki 2018-05-26 13:18:59 -0700 Comment on attachment 341405 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. 1427762 3 fpizlo 2018-05-26 13:53:11 -0700 (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. 1427763 4 fpizlo 2018-05-26 13:55:33 -0700 (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. 1427764 5 fpizlo 2018-05-26 13:56:07 -0700 (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. 1427765 6 fpizlo 2018-05-26 13:57:19 -0700 (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. 1427766 7 fpizlo 2018-05-26 13:59:30 -0700 Landed in https://trac.webkit.org/changeset/232227/webkit 1427767 8 webkit-bug-importer 2018-05-26 14:00:21 -0700 <rdar://problem/40581599> 1427807 9 ysuzuki 2018-05-27 06:52:59 -0700 (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). :) 1427808 10 ysuzuki 2018-05-27 06:54:25 -0700 (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. 341405 2018-05-26 12:35:58 -0700 2018-05-26 13:18:59 -0700 the patch blah.patch text/plain 5115 fpizlo SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTU3RyaW5nLmNwcAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU1N0cmluZy5jcHAJKHJldmlz aW9uIDIzMjIwOSkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTU3RyaW5nLmNw cAkod29ya2luZyBjb3B5KQpAQCAtNTMsNyArNTMsMTAgQEAgdm9pZCBKU1JvcGVTdHJpbmc6OlJv cGVCdWlsZGVyPFJlY29yZE92ZQogCiB2b2lkIEpTU3RyaW5nOjpkZXN0cm95KEpTQ2VsbCogY2Vs bCkKIHsKLSAgICBzdGF0aWNfY2FzdDxKU1N0cmluZyo+KGNlbGwpLT5KU1N0cmluZzo6fkpTU3Ry aW5nKCk7CisgICAgSlNTdHJpbmcqIHN0cmluZyA9IHN0YXRpY19jYXN0PEpTU3RyaW5nKj4oY2Vs bCk7CisgICAgaWYgKCFzdHJpbmctPm1fdmFsdWUuaXNOdWxsKCkpCisgICAgICAgIGRhdGFMb2co IlJlYWxseSBkZXN0cm95aW5nIHN0cmluZy5cbiIpOworICAgIHN0cmluZy0+SlNTdHJpbmc6On5K U1N0cmluZygpOwogfQogCiB2b2lkIEpTU3RyaW5nOjpkdW1wVG9TdHJlYW0oY29uc3QgSlNDZWxs KiBjZWxsLCBQcmludFN0cmVhbSYgb3V0KQpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL3J1 bnRpbWUvSlNTdHJpbmcuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVu dGltZS9KU1N0cmluZy5oCShyZXZpc2lvbiAyMzIyMDkpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENv cmUvcnVudGltZS9KU1N0cmluZy5oCSh3b3JraW5nIGNvcHkpCkBAIC0xOTIsMTEgKzE5MiwxMiBA QCBwdWJsaWM6CiAgICAgICAgIElzOEJpdCA9IDF1CiAgICAgfTsKIAorICAgIGJvb2wgaXNSb3Bl KCkgY29uc3QgeyByZXR1cm4gbV92YWx1ZS5pc051bGwoKTsgfQorCiBwcm90ZWN0ZWQ6CiAgICAg ZnJpZW5kIGNsYXNzIEpTVmFsdWU7CiAKICAgICBKU19FWFBPUlRfUFJJVkFURSBib29sIGVxdWFs U2xvd0Nhc2UoRXhlY1N0YXRlKiwgSlNTdHJpbmcqIG90aGVyKSBjb25zdDsKLSAgICBib29sIGlz Um9wZSgpIGNvbnN0IHsgcmV0dXJuIG1fdmFsdWUuaXNOdWxsKCk7IH0KICAgICBib29sIGlzU3Vi c3RyaW5nKCkgY29uc3Q7CiAgICAgYm9vbCBpczhCaXQoKSBjb25zdCB7IHJldHVybiBtX2ZsYWdz ICYgSXM4Qml0OyB9CiAgICAgdm9pZCBzZXRJczhCaXQoYm9vbCBmbGFnKSBjb25zdApJbmRleDog U291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNTdHJpbmdIZWFwQ2VsbFR5cGUuY3BwCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTU3RyaW5nSGVhcENl bGxUeXBlLmNwcAkocmV2aXNpb24gMjMyMjA5KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL3J1 bnRpbWUvSlNTdHJpbmdIZWFwQ2VsbFR5cGUuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0zNCw3ICsz NCw4IEBAIG5hbWVzcGFjZSBKU0Mgewogc3RydWN0IEpTU3RyaW5nRGVzdHJveUZ1bmMgewogICAg IEFMV0FZU19JTkxJTkUgdm9pZCBvcGVyYXRvcigpKFZNJiwgSlNDZWxsKiBjZWxsKSBjb25zdAog ICAgIHsKLSAgICAgICAgc3RhdGljX2Nhc3Q8SlNTdHJpbmcqPihjZWxsKS0+SlNTdHJpbmc6On5K U1N0cmluZygpOworICAgICAgICBKU1N0cmluZyogc3RyaW5nID0gc3RhdGljX2Nhc3Q8SlNTdHJp bmcqPihjZWxsKTsKKyAgICAgICAgc3RyaW5nLT5KU1N0cmluZzo6fkpTU3RyaW5nKCk7CiAgICAg fQogfTsKIApJbmRleDogU291cmNlL1dURi9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNl L1dURi9DaGFuZ2VMb2cJKHJldmlzaW9uIDIzMjIyNikKKysrIFNvdXJjZS9XVEYvQ2hhbmdlTG9n CSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjYgQEAKKzIwMTgtMDUtMjYgIEZpbGlwIFBpemxv ICA8ZnBpemxvQGFwcGxlLmNvbT4KKworICAgICAgICB0ZXN0YWlyIHNvbWV0aW1lcyBjcmFzaGVz IGR1ZSB0byByYWNlcyBpbiBpbml0aWFsaXphdGlvbiBvZiBBUkM0UmFuZG9tTnVtYmVyR2VuZXJh dG9yCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODYw MTQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKyAgICAgICAgCisgICAg ICAgIHRlc3RhaXIgbGF1bmNoZXMgYSBidW5jaCBvZiB0aHJlYWRzIGFuZCB0aGUgdGhyZWFkcyBk byBCMyB0aGluZ3MgdGhhdCB1c2UgcmFuZG9tIG51bWJlcnMuCisgICAgICAgIFNvbWV0aW1lcyB0 d28gdGhyZWFkcyB3aWxsIGluaXRpYWxpemUgdGhlIHJhbmRvbSBudW1iZXIgZ2VuZXJhdG9yIGF0 IHRoZSBzYW1lIHRpbWUsIGJlY2F1c2UKKyAgICAgICAgdGhhdCdzIHdoYXQgaGFwcGVucyB3aGVu IHlvdSB1c2Ugc3RhdGljIE5ldmVyRGVzdHJveWVkPD4uCisgICAgICAgIAorICAgICAgICBUaGlz IGNoYW5nZXMgdGhhdCBjb2RlIHRvIHVzZSBzdGQ6OmNhbGxfb25jZSB0byBpbml0aWFsaXplIHRo ZSBzaGFyZWQKKyAgICAgICAgQVJDNFJhbmRvbU51bWJlckdlbmVyYXRvci4KKyAgICAgICAgCisg ICAgICAgIEFsc28sIHRoaXMgYWRkcyBhIGRpYWdub3N0aWMgbWVzc2FnZSB0byB0aGUgbG9jaydz IGFzc2VydGlvbi4gVGhpcyBhc3NlcnRpb24gd2FzIHRoZSBzeW1wdG9tCisgICAgICAgIG9mIHRo ZSByYWNlLCBhbmQga25vd2luZyB0aGUgc3RhdGUgb2YgdGhlIGxvY2sgd2hlbiB0aGUgYXNzZXJ0 aW9uIGZpcmVkIGdhdmUgYSBkYXJuIGdvb2QgY2x1ZQorICAgICAgICBhYm91dCB3aGF0IHdhcyBn b2luZyBvbjogdGhlIGxvY2sncyB2YWx1ZSB3YXMgMCBhdCB0aW1lIG9mIHVubG9jaywgaW1wbHlp bmcgdGhhdCBhbm90aGVyCisgICAgICAgIHRocmVhZCByZWluaXRpYWxpemVkIHRoZSBsb2NrIHRv IHplcm8gYnkgcmVydW5uaW5nIHRoZSBjb25zdHJ1Y3Rvci4KKworICAgICAgICAqIHd0Zi9Dcnlw dG9ncmFwaGljYWxseVJhbmRvbU51bWJlci5jcHA6CisgICAgICAgICogd3RmL0xvY2tBbGdvcml0 aG1JbmxpbmVzLmg6CisgICAgICAgIChXVEY6Okhvb2tzPjo6dW5sb2NrU2xvdyk6CisKIDIwMTgt MDUtMjUgIE1pY2hhZWwgU2Fib2ZmICA8bXNhYm9mZkBhcHBsZS5jb20+CiAKICAgICAgICAgSmF2 YVNjcmlwdENvcmU6IERpc2FibGUgMzItYml0IEpJVCBvbiBXaW5kb3dzCkluZGV4OiBTb3VyY2Uv V1RGL3d0Zi9DcnlwdG9ncmFwaGljYWxseVJhbmRvbU51bWJlci5jcHAKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g U291cmNlL1dURi93dGYvQ3J5cHRvZ3JhcGhpY2FsbHlSYW5kb21OdW1iZXIuY3BwCShyZXZpc2lv biAyMzIyMDkpCisrKyBTb3VyY2UvV1RGL3d0Zi9DcnlwdG9ncmFwaGljYWxseVJhbmRvbU51bWJl ci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTE1OSw5ICsxNTksMTUgQEAgdm9pZCBBUkM0UmFuZG9t TnVtYmVyR2VuZXJhdG9yOjpyYW5kb21WYQogCiBBUkM0UmFuZG9tTnVtYmVyR2VuZXJhdG9yJiBz aGFyZWRSYW5kb21OdW1iZXJHZW5lcmF0b3IoKQogewotICAgIHN0YXRpYyBOZXZlckRlc3Ryb3ll ZDxBUkM0UmFuZG9tTnVtYmVyR2VuZXJhdG9yPiByYW5kb21OdW1iZXJHZW5lcmF0b3I7CisgICAg c3RhdGljIEFSQzRSYW5kb21OdW1iZXJHZW5lcmF0b3IqIHJhbmRvbU51bWJlckdlbmVyYXRvcjsK KyAgICBzdGF0aWMgc3RkOjpvbmNlX2ZsYWcgb25jZUZsYWc7CisgICAgc3RkOjpjYWxsX29uY2Uo CisgICAgICAgIG9uY2VGbGFnLAorICAgICAgICBbXSB7CisgICAgICAgICAgICByYW5kb21OdW1i ZXJHZW5lcmF0b3IgPSBuZXcgQVJDNFJhbmRvbU51bWJlckdlbmVyYXRvcigpOworICAgICAgICB9 KTsKIAotICAgIHJldHVybiByYW5kb21OdW1iZXJHZW5lcmF0b3I7CisgICAgcmV0dXJuICpyYW5k b21OdW1iZXJHZW5lcmF0b3I7CiB9CiAKIH0KSW5kZXg6IFNvdXJjZS9XVEYvd3RmL0xvY2tBbGdv cml0aG1JbmxpbmVzLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dURi93dGYvTG9ja0FsZ29yaXRo bUlubGluZXMuaAkocmV2aXNpb24gMjMyMjA5KQorKysgU291cmNlL1dURi93dGYvTG9ja0FsZ29y aXRobUlubGluZXMuaAkod29ya2luZyBjb3B5KQpAQCAtMTEwLDkgKzExMCwxMSBAQCB2b2lkIExv Y2tBbGdvcml0aG08TG9ja1R5cGUsIGlzSGVsZEJpdCwgCiAgICAgLy8gYmUgaGVsZCBhbmQgcGFy a2VkIGlmIHNvbWVvbmUgYXR0ZW1wdHMgdG8gbG9jayBqdXN0IGFzIHdlIGFyZSB1bmxvY2tpbmcu CiAgICAgZm9yICg7OykgewogICAgICAgICB1aW50OF90IG9sZEJ5dGVWYWx1ZSA9IGxvY2subG9h ZCgpOwotICAgICAgICBSRUxFQVNFX0FTU0VSVCgKLSAgICAgICAgICAgIChvbGRCeXRlVmFsdWUg JiBtYXNrKSA9PSBpc0hlbGRCaXQKLSAgICAgICAgICAgIHx8IChvbGRCeXRlVmFsdWUgJiBtYXNr KSA9PSAoaXNIZWxkQml0IHwgaGFzUGFya2VkQml0KSk7CisgICAgICAgIGlmICgob2xkQnl0ZVZh bHVlICYgbWFzaykgIT0gaXNIZWxkQml0CisgICAgICAgICAgICAmJiAob2xkQnl0ZVZhbHVlICYg bWFzaykgIT0gKGlzSGVsZEJpdCB8IGhhc1BhcmtlZEJpdCkpIHsKKyAgICAgICAgICAgIGRhdGFM b2coIkludmFsaWQgdmFsdWUgZm9yIGxvY2s6ICIsIG9sZEJ5dGVWYWx1ZSwgIlxuIik7CisgICAg ICAgICAgICBSRUxFQVNFX0FTU0VSVF9OT1RfUkVBQ0hFRCgpOworICAgICAgICB9CiAgICAgICAg IAogICAgICAgICBpZiAoKG9sZEJ5dGVWYWx1ZSAmIG1hc2spID09IGlzSGVsZEJpdCkgewogICAg ICAgICAgICAgaWYgKGxvY2suY29tcGFyZUV4Y2hhbmdlV2VhayhvbGRCeXRlVmFsdWUsIEhvb2tz Ojp1bmxvY2tIb29rKG9sZEJ5dGVWYWx1ZSAmIH5pc0hlbGRCaXQpKSkK