211201 2020-04-29 13:16:19 -0700 Freezing of Gigacage and JSC Configs should be thread safe. 2020-04-29 13:40:23 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mark.lam mark.lam ews-watchlist keith_miller msaboff saam tzagallo webkit-bug-importer ysuzuki oldest_to_newest 1647126 0 mark.lam 2020-04-29 13:16:19 -0700 If a client creates multiple VM instances in different threads concurrently, the following race can occur: Config::permanentlyFreeze() contains the following code: if (!g_jscConfig.isPermanentlyFrozen) // Point P1 g_jscConfig.isPermanentlyFrozen = true; // Point P2 Let's say there are 2 threads T1 and T2. 1. T1 creates a VM and gets to point P1, and sees that g_jscConfig.isPermanentlyFrozen is not set. T1 is about to execute P2 when it gets gets pre-empted. 2. T2 creates a VM and gets to point P1, and sees that g_jscConfig.isPermanentlyFrozen is not set. T2 proceeds to point P2 and sets g_jscConfig.isPermanentlyFrozen to true. T2 goes on to freeze the Config and makes it not writable. 3. T1 gets to run again, and proceeds to point P2. T1 tries to set g_jscConfig.isPermanentlyFrozen to true. But because the Config has been frozen against writes, the write to g_jscConfig.isPermanentlyFrozen results in a crash. This is a classic TOCTOU bug. The fix is simply to ensure that only one thread can enter Config::permanentlyFreeze() at a time. Ditto for Gigacage::permanentlyFreezeGigacageConfig(). <rdar://problem/62597619> 1647131 1 397992 mark.lam 2020-04-29 13:25:07 -0700 Created attachment 397992 proposed patch. 1647132 2 397992 ysuzuki 2020-04-29 13:27:50 -0700 Comment on attachment 397992 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=397992&action=review > Source/JavaScriptCore/runtime/JSCConfig.cpp:58 > + static Lock configLock; > + LockHolder locker(configLock); Why not using std::call_once? > Source/bmalloc/bmalloc/Gigacage.cpp:122 > + static Mutex configLock; > + LockHolder locking(configLock); > + Ditto. 1647135 3 397992 mark.lam 2020-04-29 13:32:33 -0700 Comment on attachment 397992 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=397992&action=review >> Source/JavaScriptCore/runtime/JSCConfig.cpp:58 >> + LockHolder locker(configLock); > > Why not using std::call_once? Because I want each call to permanentlyFreeze() to go thru the vm_protect and affirm that the setting is as we expect it. This is why there's a RELEASE_ASSERT on the result. 1647137 4 397992 ysuzuki 2020-04-29 13:32:54 -0700 Comment on attachment 397992 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=397992&action=review Discussed with Mark. We actually want to call this function multiple times, so std::call_once is not suitable. But while executing this function, this function breaks an invariant temporarily which is required by the call from the other thread. So guarding this with Lock is the appropriate way. >> Source/JavaScriptCore/runtime/JSCConfig.cpp:58 >> + LockHolder locker(configLock); > > Why not using std::call_once? Use `auto locker = holdLock(configLock)` in WTF. 1647142 5 mark.lam 2020-04-29 13:40:23 -0700 Thanks for the review. (In reply to Yusuke Suzuki from comment #4) > Use `auto locker = holdLock(configLock)` in WTF. Applied. Landed in r260913: <http://trac.webkit.org/r260913>. 397992 2020-04-29 13:25:07 -0700 2020-04-29 13:32:54 -0700 proposed patch. bug-211201.patch text/plain 4767 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjYwOTEwKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDQxIEBA CisyMDIwLTA0LTI5ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBG cmVlemluZyBvZiBHaWdhY2FnZSBhbmQgSlNDIENvbmZpZ3Mgc2hvdWxkIGJlIHRocmVhZCBzYWZl LgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjExMjAx CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS82MjU5NzYxOT4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJZiBhIGNsaWVudCBjcmVhdGVzIG11bHRpcGxl IFZNIGluc3RhbmNlcyBpbiBkaWZmZXJlbnQgdGhyZWFkcyBjb25jdXJyZW50bHksIHRoZQorICAg ICAgICBmb2xsb3dpbmcgcmFjZSBjYW4gb2NjdXI6CisKKyAgICAgICAgQ29uZmlnOjpwZXJtYW5l bnRseUZyZWV6ZSgpIGNvbnRhaW5zIHRoZSBmb2xsb3dpbmcgY29kZToKKworICAgICAgICAgICAg aWYgKCFnX2pzY0NvbmZpZy5pc1Blcm1hbmVudGx5RnJvemVuKSAgICAgICAgIC8vIFBvaW50IFAx CisgICAgICAgICAgICAgICAgZ19qc2NDb25maWcuaXNQZXJtYW5lbnRseUZyb3plbiA9IHRydWU7 ICAgLy8gUG9pbnQgUDIKKworICAgICAgICBMZXQncyBzYXkgdGhlcmUgYXJlIDIgdGhyZWFkcyBU MSBhbmQgVDIuCisKKyAgICAgICAgMS4gVDEgY3JlYXRlcyBhIFZNIGFuZCBnZXRzIHRvIHBvaW50 IFAxLCBhbmQgc2VlcyB0aGF0IGdfanNjQ29uZmlnLmlzUGVybWFuZW50bHlGcm96ZW4gaXMgbm90 IHNldC4KKyAgICAgICAgICAgVDEgaXMgYWJvdXQgdG8gZXhlY3V0ZSBQMiB3aGVuIGl0IGdldHMg cHJlLWVtcHRlZC4KKworICAgICAgICAyLiBUMiBjcmVhdGVzIGEgVk0gYW5kIGdldHMgdG8gcG9p bnQgUDEsIGFuZCBzZWVzIHRoYXQgZ19qc2NDb25maWcuaXNQZXJtYW5lbnRseUZyb3plbiBpcyBu b3Qgc2V0LgorICAgICAgICAgICBUMiBwcm9jZWVkcyB0byBwb2ludCBQMiBhbmQgc2V0cyBnX2pz Y0NvbmZpZy5pc1Blcm1hbmVudGx5RnJvemVuIHRvIHRydWUuCisgICAgICAgICAgIFQyIGdvZXMg b24gdG8gZnJlZXplIHRoZSBDb25maWcgYW5kIG1ha2VzIGl0IG5vdCB3cml0YWJsZS4KKworICAg ICAgICAzLiBUMSBnZXRzIHRvIHJ1biBhZ2FpbiwgYW5kIHByb2NlZWRzIHRvIHBvaW50IFAyLgor ICAgICAgICAgICBUMSB0cmllcyB0byBzZXQgZ19qc2NDb25maWcuaXNQZXJtYW5lbnRseUZyb3pl biB0byB0cnVlLgorICAgICAgICAgICBCdXQgYmVjYXVzZSB0aGUgQ29uZmlnIGhhcyBiZWVuIGZy b3plbiBhZ2FpbnN0IHdyaXRlcywgdGhlIHdyaXRlIHRvCisgICAgICAgICAgIGdfanNjQ29uZmln LmlzUGVybWFuZW50bHlGcm96ZW4gcmVzdWx0cyBpbiBhIGNyYXNoLgorCisgICAgICAgIFRoaXMg aXMgYSBjbGFzc2ljIFRPQ1RPVSBidWcuICBUaGUgZml4IGlzIHNpbXBseSB0byBlbnN1cmUgdGhh dCBvbmx5IG9uZSB0aHJlYWQKKyAgICAgICAgY2FuIGVudGVyIENvbmZpZzo6cGVybWFuZW50bHlG cmVlemUoKSBhdCBhIHRpbWUuCisKKyAgICAgICAgRGl0dG8gZm9yIEdpZ2FjYWdlOjpwZXJtYW5l bnRseUZyZWV6ZUdpZ2FjYWdlQ29uZmlnKCkuCisKKyAgICAgICAgKiBydW50aW1lL0pTQ0NvbmZp Zy5jcHA6CisgICAgICAgIChKU0M6OkNvbmZpZzo6cGVybWFuZW50bHlGcmVlemUpOgorCiAyMDIw LTA0LTI5ICBZdXN1a2UgU3V6dWtpICA8eXN1enVraUBhcHBsZS5jb20+CiAKICAgICAgICAgW0pT Q10gSlNTdHJpbmdKb2luZXIgaXMgbWlzc2luZyBCaWdJbnQgaGFuZGxpbmcKSW5kZXg6IFNvdXJj ZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTQ0NvbmZpZy5jcHAKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNDQ29uZmlnLmNwcAkocmV2aXNpb24gMjYwOTEw KQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNDQ29uZmlnLmNwcAkod29ya2lu ZyBjb3B5KQpAQCAtMSw1ICsxLDUgQEAKIC8qCi0gKiBDb3B5cmlnaHQgKEMpIDIwMTkgQXBwbGUg SW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkLgorICogQ29weXJpZ2h0IChDKSAyMDE5LTIwMjAgQXBw bGUgSW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkLgogICoKICAqIFJlZGlzdHJpYnV0aW9uIGFuZCB1 c2UgaW4gc291cmNlIGFuZCBiaW5hcnkgZm9ybXMsIHdpdGggb3Igd2l0aG91dAogICogbW9kaWZp Y2F0aW9uLCBhcmUgcGVybWl0dGVkIHByb3ZpZGVkIHRoYXQgdGhlIGZvbGxvd2luZyBjb25kaXRp b25zCkBAIC0yNiw2ICsyNiw3IEBACiAjaW5jbHVkZSAiY29uZmlnLmgiCiAjaW5jbHVkZSAiSlND Q29uZmlnLmgiCiAKKyNpbmNsdWRlIDx3dGYvTG9jay5oPgogI2luY2x1ZGUgPHd0Zi9SZXNvdXJj ZVVzYWdlLmg+CiAjaW5jbHVkZSA8d3RmL1N0ZExpYkV4dHJhcy5oPgogCkBAIC01Myw2ICs1NCw5 IEBAIHZvaWQgQ29uZmlnOjplbmFibGVSZXN0cmljdGVkT3B0aW9ucygpCiAgICAgCiB2b2lkIENv bmZpZzo6cGVybWFuZW50bHlGcmVlemUoKQogeworICAgIHN0YXRpYyBMb2NrIGNvbmZpZ0xvY2s7 CisgICAgTG9ja0hvbGRlciBsb2NrZXIoY29uZmlnTG9jayk7CisKICAgICBSRUxFQVNFX0FTU0VS VChyb3VuZFVwVG9NdWx0aXBsZU9mKHBhZ2VTaXplKCksIENvbmZpZ1NpemVUb1Byb3RlY3QpID09 IENvbmZpZ1NpemVUb1Byb3RlY3QpOwogCiAgICAgaWYgKCFnX2pzY0NvbmZpZy5pc1Blcm1hbmVu dGx5RnJvemVuKQpJbmRleDogU291cmNlL2JtYWxsb2MvQ2hhbmdlTG9nCj09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0t IFNvdXJjZS9ibWFsbG9jL0NoYW5nZUxvZwkocmV2aXNpb24gMjYwOTEwKQorKysgU291cmNlL2Jt YWxsb2MvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIwMjAtMDQt MjkgIE1hcmsgTGFtICA8bWFyay5sYW1AYXBwbGUuY29tPgorCisgICAgICAgIEZyZWV6aW5nIG9m IEdpZ2FjYWdlIGFuZCBKU0MgQ29uZmlncyBzaG91bGQgYmUgdGhyZWFkIHNhZmUuCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMTEyMDEKKyAgICAgICAg PHJkYXI6Ly9wcm9ibGVtLzYyNTk3NjE5PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgICogYm1hbGxvYy9HaWdhY2FnZS5jcHA6CisgICAgICAgIChHaWdh Y2FnZTo6Ym1hbGxvYzo6cGVybWFuZW50bHlGcmVlemVHaWdhY2FnZUNvbmZpZyk6CisKIDIwMjAt MDQtMjUgIERhcmluIEFkbGVyICA8ZGFyaW5AYXBwbGUuY29tPgogCiAgICAgICAgIFtDb2NvYV0g RGVhbCB3aXRoIGFub3RoZXIgcm91bmQgb2YgWGNvZGUgdXBncmFkZSBjaGVja3MKSW5kZXg6IFNv dXJjZS9ibWFsbG9jL2JtYWxsb2MvR2lnYWNhZ2UuY3BwCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9i bWFsbG9jL2JtYWxsb2MvR2lnYWNhZ2UuY3BwCShyZXZpc2lvbiAyNjA5MTApCisrKyBTb3VyY2Uv Ym1hbGxvYy9ibWFsbG9jL0dpZ2FjYWdlLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMSw1ICsxLDUg QEAKIC8qCi0gKiBDb3B5cmlnaHQgKEMpIDIwMTctMjAxOSBBcHBsZSBJbmMuIEFsbCByaWdodHMg cmVzZXJ2ZWQuCisgKiBDb3B5cmlnaHQgKEMpIDIwMTctMjAyMCBBcHBsZSBJbmMuIEFsbCByaWdo dHMgcmVzZXJ2ZWQuCiAgKgogICogUmVkaXN0cmlidXRpb24gYW5kIHVzZSBpbiBzb3VyY2UgYW5k IGJpbmFyeSBmb3Jtcywgd2l0aCBvciB3aXRob3V0CiAgKiBtb2RpZmljYXRpb24sIGFyZSBwZXJt aXR0ZWQgcHJvdmlkZWQgdGhhdCB0aGUgZm9sbG93aW5nIGNvbmRpdGlvbnMKQEAgLTI3LDEzICsy NywxMyBAQAogCiAjaW5jbHVkZSAiQ3J5cHRvUmFuZG9tLmgiCiAjaW5jbHVkZSAiRW52aXJvbm1l bnQuaCIKKyNpbmNsdWRlICJNdXRleC5oIgogI2luY2x1ZGUgIlByb2Nlc3NDaGVjay5oIgogI2lu Y2x1ZGUgIlN0YXRpY1BlclByb2Nlc3MuaCIKICNpbmNsdWRlICJWTUFsbG9jYXRlLmgiCiAjaW5j bHVkZSAiVmVjdG9yLmgiCiAjaW5jbHVkZSAiYm1hbGxvYy5oIgogI2luY2x1ZGUgPGNzdGRpbz4K LSNpbmNsdWRlIDxtdXRleD4KIAogI2lmIEJPUyhEQVJXSU4pCiAjaW5jbHVkZSA8bWFjaC9tYWNo Lmg+CkBAIC0xMTcsNiArMTE3LDkgQEAgc3RhdGljIHZvaWQgdW5mcmVlemVHaWdhY2FnZUNvbmZp ZygpCiAKIHN0YXRpYyB2b2lkIHBlcm1hbmVudGx5RnJlZXplR2lnYWNhZ2VDb25maWcoKQogewor ICAgIHN0YXRpYyBNdXRleCBjb25maWdMb2NrOworICAgIExvY2tIb2xkZXIgbG9ja2luZyhjb25m aWdMb2NrKTsKKwogICAgIGlmICghZ19naWdhY2FnZUNvbmZpZy5pc1Blcm1hbmVudGx5RnJvemVu KSB7CiAgICAgICAgIHVuZnJlZXplR2lnYWNhZ2VDb25maWcoKTsKICAgICAgICAgZ19naWdhY2Fn ZUNvbmZpZy5pc1Blcm1hbmVudGx5RnJvemVuID0gdHJ1ZTsK