209042 2020-03-13 00:28:57 -0700 reportZappedCellAndCrash should handle PreciseAllocation in IsoSubspace 2020-03-15 03:52:15 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ysuzuki ysuzuki bfulgham darin ddkilzer ews-watchlist keith_miller mark.lam msaboff rniwa saam tzagallo webkit-bug-importer oldest_to_newest 1629518 0 ysuzuki 2020-03-13 00:28:57 -0700 Let's extend. 1629971 1 393567 ysuzuki 2020-03-13 18:08:31 -0700 Created attachment 393567 Patch 1630097 2 ysuzuki 2020-03-14 14:59:11 -0700 To collect more cell info in ASan bots. 1630104 3 393567 mark.lam 2020-03-14 15:41:50 -0700 Comment on attachment 393567 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393567&action=review > Source/JavaScriptCore/runtime/JSCell.cpp:387 > + variousState |= static_cast<uint64_t>(!isFreeListed) << 1; Is this correct? Does !!isFreeListed mean isAllocated? 1630105 4 mark.lam 2020-03-14 15:42:52 -0700 (In reply to Mark Lam from comment #3) > Comment on attachment 393567 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393567&action=review > > > Source/JavaScriptCore/runtime/JSCell.cpp:387 > > + variousState |= static_cast<uint64_t>(!isFreeListed) << 1; > > Is this correct? Does !!isFreeListed mean isAllocated? typo: I meant !isFreeListed 1630126 5 393567 mark.lam 2020-03-14 17:25:45 -0700 Comment on attachment 393567 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393567&action=review r=me >>> Source/JavaScriptCore/runtime/JSCell.cpp:387 >>> + variousState |= static_cast<uint64_t>(!isFreeListed) << 1; >> >> Is this correct? Does !!isFreeListed mean isAllocated? > > typo: I meant !isFreeListed Nevermind, I've read thru the PreciseAllocation code and this patch some more. This is correct because foundPreciseAllocation is only true if this cell is found in the lower tier of this IsoSubspace, and it is only allocated if it's not on the free list. > Source/JavaScriptCore/runtime/JSCell.cpp:390 > + variousState |= static_cast<uint64_t>(foundPreciseAllocation->isEmpty()) << 2; > + variousState |= static_cast<uint64_t>(foundPreciseAllocation->isNewlyAllocated()) << 4; The isEmpty and isNewlyAllocated flags previously meant that the cell pointer was found in a MarkedBlock of that state. For PreciseAllocations, I think knowing whether it isFreeListed or isAllocated is sufficient. I'm not quite sure what isEmpty and isNewlyAllocated would mean in this case. I suggest just leaving these unset. Instead, can you also set the cellIsProperlyAligned flag ( << 5) in variousState based on whether (foundPreciseAllocation->cell() == cell)? If the cell pointer is found in a PreciseAllocation but does not equal it's expected cell() pointer, then we have a corrupted pointer or someone unscrupulous is making bad pointers. Can you also set the needsDestruction flag in variousState based on whether the SubSpace CellAttributes says it NeedsDestruction or not? 1630159 6 393567 ysuzuki 2020-03-15 03:41:55 -0700 Comment on attachment 393567 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393567&action=review >> Source/JavaScriptCore/runtime/JSCell.cpp:390 >> + variousState |= static_cast<uint64_t>(foundPreciseAllocation->isNewlyAllocated()) << 4; > > The isEmpty and isNewlyAllocated flags previously meant that the cell pointer was found in a MarkedBlock of that state. For PreciseAllocations, I think knowing whether it isFreeListed or isAllocated is sufficient. I'm not quite sure what isEmpty and isNewlyAllocated would mean in this case. I suggest just leaving these unset. > > Instead, can you also set the cellIsProperlyAligned flag ( << 5) in variousState based on whether (foundPreciseAllocation->cell() == cell)? If the cell pointer is found in a PreciseAllocation but does not equal it's expected cell() pointer, then we have a corrupted pointer or someone unscrupulous is making bad pointers. > > Can you also set the needsDestruction flag in variousState based on whether the SubSpace CellAttributes says it NeedsDestruction or not? I think isNewlyAllocated (In Eden or not) is nice to have. And isEmpty is also nice too (WeakSet is claered etc.). I'm querying this only when `!isFreeListed` since (1) isNewlyAllocated is meaningless if it is not in free list and (2) if this cell is not in freelist, we are not guaranteeing that WeakSet (isEmpty is accessing this) is not broken. Adding cellIsProperlyAligned sounds nice. Fixed. Not sure whether NeedsDestruction is important since this can be super easily detected based on IsoSubspace, but maybe it would be ok. Added. 1630160 7 ysuzuki 2020-03-15 03:51:09 -0700 Committed r258479: <https://trac.webkit.org/changeset/258479> 1630161 8 webkit-bug-importer 2020-03-15 03:52:15 -0700 <rdar://problem/60466867> 393567 2020-03-13 18:08:31 -0700 2020-03-14 17:25:45 -0700 Patch bug-209042-20200313180830.patch text/plain 5319 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjU4NDUzCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCAy NGU0ZDVjYmQ5MWMwZWI2ZTg4YjExMzdlNGEwZGUzNDY3M2ZkODNlLi44YWEwOWZmYjljYmQ5ZDI3 NDEyZDdjOWNiZjAxNzk4OWU0MGRlYzgyIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOSBAQAorMjAyMC0wMy0xMyAgWXVzdWtlIFN1enVraSAgPHlzdXp1a2lAYXBwbGUuY29t PgorCisgICAgICAgIHJlcG9ydFphcHBlZENlbGxBbmRDcmFzaCBzaG91bGQgaGFuZGxlIFByZWNp c2VBbGxvY2F0aW9uIGluIElzb1N1YnNwYWNlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0yMDkwNDIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBUaGlzIHBhdGNoIGFkZHMgc3VwcG9ydCBvZiBQcmVjaXNlQWxs b2NhdGlvbiBjZWxscyB0byByZXBvcnRaYXBwZWRDZWxsQW5kQ3Jhc2gsIHNpbmNlIG5vdyBpdCBp cyBmcmVxdWVudGx5IHVzZWQKKyAgICAgICAgYXMgYSBsb3dlci10aWVyIGNlbGxzIGluIElzb1N1 YnNwYWNlLgorCisgICAgICAgICogaGVhcC9Jc29TdWJzcGFjZS5oOgorICAgICAgICAqIGhlYXAv SXNvU3Vic3BhY2VJbmxpbmVzLmg6CisgICAgICAgIChKU0M6Oklzb1N1YnNwYWNlOjpmb3JFYWNo TG93ZXJUaWVyRnJlZUxpc3RlZFByZWNpc2VBbGxvY2F0aW9uKToKKyAgICAgICAgKiBydW50aW1l L0pTQ2VsbC5jcHA6CisgICAgICAgIChKU0M6OnJlcG9ydFphcHBlZENlbGxBbmRDcmFzaCk6CisK IDIwMjAtMDMtMTMgIFl1c3VrZSBTdXp1a2kgIDx5c3V6dWtpQGFwcGxlLmNvbT4KIAogICAgICAg ICBVbnJldmlld2VkLCBmaXggSlNDIC8gdGVzdDI2MiB0ZXN0cwpkaWZmIC0tZ2l0IGEvU291cmNl L0phdmFTY3JpcHRDb3JlL2hlYXAvSXNvU3Vic3BhY2UuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS9oZWFwL0lzb1N1YnNwYWNlLmgKaW5kZXggMGRiZDNiMGNjMzEwZDc4ZjNmMDdmY2JjNTE1NmUz NmFlYTQzMGJiMS4uYTM3YjRiYjQ0YWVhOWJhNjExZDgwODI3ODkyMGU0MTcyNDZjMzE1YSAxMDA2 NDQKLS0tIGEvU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvSXNvU3Vic3BhY2UuaAorKysgYi9T b3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9Jc29TdWJzcGFjZS5oCkBAIC01Niw2ICs1Niw4IEBA IGNsYXNzIElzb1N1YnNwYWNlIDogcHVibGljIFN1YnNwYWNlIHsKIAogICAgIHZvaWQgc3dlZXAo KTsKIAorICAgIHRlbXBsYXRlPHR5cGVuYW1lIEZ1bmM+IHZvaWQgZm9yRWFjaExvd2VyVGllckZy ZWVMaXN0ZWRQcmVjaXNlQWxsb2NhdGlvbihjb25zdCBGdW5jJik7CisKIHByaXZhdGU6CiAgICAg ZnJpZW5kIGNsYXNzIElzb0NlbGxTZXQ7CiAgICAgCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNj cmlwdENvcmUvaGVhcC9Jc29TdWJzcGFjZUlubGluZXMuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS9oZWFwL0lzb1N1YnNwYWNlSW5saW5lcy5oCmluZGV4IDQ2NjBlNzc4M2FhNjcyZWM4YTI5ZjRm NGYzM2MyMzJjYjQ2ZGY3NjIuLjhlZDBjYzc3YmJjYTlkMGI1YmUxYTJjYmRkOGI2ZmM0MGNjMGNi MzggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9oZWFwL0lzb1N1YnNwYWNlSW5s aW5lcy5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9oZWFwL0lzb1N1YnNwYWNlSW5saW5l cy5oCkBAIC01NCw1ICs1NCwxMSBAQCBpbmxpbmUgdm9pZCBJc29TdWJzcGFjZTo6c3dlZXAoKQog ICAgIH0pOwogfQogCit0ZW1wbGF0ZTx0eXBlbmFtZSBGdW5jPgordm9pZCBJc29TdWJzcGFjZTo6 Zm9yRWFjaExvd2VyVGllckZyZWVMaXN0ZWRQcmVjaXNlQWxsb2NhdGlvbihjb25zdCBGdW5jJiBm dW5jKQoreworICAgIG1fbG93ZXJUaWVyRnJlZUxpc3QuZm9yRWFjaChmdW5jKTsKK30KKwogfSAv LyBuYW1lc3BhY2UgSlNDCiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50 aW1lL0pTQ2VsbC5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU0NlbGwuY3Bw CmluZGV4IGJkYmVmMmUyODVmMmNhN2NjMzA0OGE3NDZlMDdjMmZmODVjYzMwNDQuLjg0YjM0ZGE4 MjVjMDE1YzYwNzk0MTY4MzExMTc2MmJkZjNmNjA2OGYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS9ydW50aW1lL0pTQ2VsbC5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRDb3Jl L3J1bnRpbWUvSlNDZWxsLmNwcApAQCAtMjUsNiArMjUsNyBAQAogCiAjaW5jbHVkZSAiQXJyYXlC dWZmZXJWaWV3LmgiCiAjaW5jbHVkZSAiQmxvY2tEaXJlY3RvcnlJbmxpbmVzLmgiCisjaW5jbHVk ZSAiSXNvU3Vic3BhY2VJbmxpbmVzLmgiCiAjaW5jbHVkZSAiSlNDSW5saW5lcy5oIgogI2luY2x1 ZGUgIkpTQ2FzdC5oIgogI2luY2x1ZGUgIkpTRnVuY3Rpb24uaCIKQEAgLTMyLDYgKzMzLDcgQEAK ICNpbmNsdWRlICJKU09iamVjdC5oIgogI2luY2x1ZGUgIk1hcmtlZEJsb2NrSW5saW5lcy5oIgog I2luY2x1ZGUgIk51bWJlck9iamVjdC5oIgorI2luY2x1ZGUgIlN1YnNwYWNlSW5saW5lcy5oIgog I2luY2x1ZGUgPHd0Zi9Mb2NrQWxnb3JpdGhtSW5saW5lcy5oPgogI2luY2x1ZGUgPHd0Zi9NYXRo RXh0cmFzLmg+CiAKQEAgLTMzMCw3ICszMzIsNyBAQCBORVZFUl9JTkxJTkUgTk9fUkVUVVJOX0RV RV9UT19DUkFTSCBOT1RfVEFJTF9DQUxMRUQgdm9pZCByZXBvcnRaYXBwZWRDZWxsQW5kQ3Jhcwog ICAgIHVuc2lnbmVkIHN1YnNwYWNlSGFzaCA9IDA7CiAgICAgc2l6ZV90IGNlbGxTaXplID0gMDsK IAotICAgIGhlYXAub2JqZWN0U3BhY2UoKS5mb3JFYWNoQmxvY2soWyZdIChNYXJrZWRCbG9jazo6 SGFuZGxlKiBibG9ja0hhbmRsZSkgeworICAgIGhlYXAub2JqZWN0U3BhY2UoKS5mb3JFYWNoQmxv Y2soWyZdKE1hcmtlZEJsb2NrOjpIYW5kbGUqIGJsb2NrSGFuZGxlKSB7CiAgICAgICAgIGlmIChi bG9ja0hhbmRsZS0+Y29udGFpbnMoYml0d2lzZV9jYXN0PEpTQ2VsbCo+KGNlbGwpKSkgewogICAg ICAgICAgICAgZm91bmRCbG9ja0hhbmRsZSA9IGJsb2NrSGFuZGxlOwogICAgICAgICAgICAgcmV0 dXJuIEl0ZXJhdGlvblN0YXR1czo6RG9uZTsKQEAgLTM1NCw2ICszNTYsNDAgQEAgTkVWRVJfSU5M SU5FIE5PX1JFVFVSTl9EVUVfVE9fQ1JBU0ggTk9UX1RBSUxfQ0FMTEVEIHZvaWQgcmVwb3J0WmFw cGVkQ2VsbEFuZENyYXMKICAgICAgICAgcHRyZGlmZl90IGNlbGxPZmZzZXQgPSBjZWxsQWRkcmVz cyAtIHJlaW50ZXJwcmV0X2Nhc3Q8dWludDY0X3Q+KGZvdW5kQmxvY2tIYW5kbGUtPnN0YXJ0KCkp OwogICAgICAgICBib29sIGNlbGxJc1Byb3Blcmx5QWxpZ25lZCA9ICEoY2VsbE9mZnNldCAlIGNl bGxTaXplKTsKICAgICAgICAgdmFyaW91c1N0YXRlIHw9IHN0YXRpY19jYXN0PHVpbnQ2NF90Pihj ZWxsSXNQcm9wZXJseUFsaWduZWQpIDw8IDU7CisgICAgfSBlbHNlIHsKKyAgICAgICAgYm9vbCBp c0ZyZWVMaXN0ZWQgPSBmYWxzZTsKKyAgICAgICAgUHJlY2lzZUFsbG9jYXRpb24qIGZvdW5kUHJl Y2lzZUFsbG9jYXRpb24gPSBudWxscHRyOworICAgICAgICBoZWFwLm9iamVjdFNwYWNlKCkuZm9y RWFjaFN1YnNwYWNlKFsmXShTdWJzcGFjZSYgc3Vic3BhY2UpIHsKKyAgICAgICAgICAgIHN1YnNw YWNlLmZvckVhY2hQcmVjaXNlQWxsb2NhdGlvbihbJl0oUHJlY2lzZUFsbG9jYXRpb24qIGFsbG9j YXRpb24pIHsKKyAgICAgICAgICAgICAgICBpZiAoYWxsb2NhdGlvbi0+Y29udGFpbnMoY2VsbCkp CisgICAgICAgICAgICAgICAgICAgIGZvdW5kUHJlY2lzZUFsbG9jYXRpb24gPSBhbGxvY2F0aW9u OworICAgICAgICAgICAgfSk7CisgICAgICAgICAgICBpZiAoZm91bmRQcmVjaXNlQWxsb2NhdGlv bikKKyAgICAgICAgICAgICAgICByZXR1cm4gSXRlcmF0aW9uU3RhdHVzOjpEb25lOworCisgICAg ICAgICAgICBpZiAoc3Vic3BhY2UuaXNJc29TdWJzcGFjZSgpKSB7CisgICAgICAgICAgICAgICAg c3RhdGljX2Nhc3Q8SXNvU3Vic3BhY2UmPihzdWJzcGFjZSkuZm9yRWFjaExvd2VyVGllckZyZWVM aXN0ZWRQcmVjaXNlQWxsb2NhdGlvbihbJl0oUHJlY2lzZUFsbG9jYXRpb24qIGFsbG9jYXRpb24p IHsKKyAgICAgICAgICAgICAgICAgICAgaWYgKGFsbG9jYXRpb24tPmNvbnRhaW5zKGNlbGwpKSB7 CisgICAgICAgICAgICAgICAgICAgICAgICBmb3VuZFByZWNpc2VBbGxvY2F0aW9uID0gYWxsb2Nh dGlvbjsKKyAgICAgICAgICAgICAgICAgICAgICAgIGlzRnJlZUxpc3RlZCA9IHRydWU7CisgICAg ICAgICAgICAgICAgICAgIH0KKyAgICAgICAgICAgICAgICB9KTsKKyAgICAgICAgICAgIH0KKyAg ICAgICAgICAgIGlmIChmb3VuZFByZWNpc2VBbGxvY2F0aW9uKQorICAgICAgICAgICAgICAgIHJl dHVybiBJdGVyYXRpb25TdGF0dXM6OkRvbmU7CisgICAgICAgICAgICByZXR1cm4gSXRlcmF0aW9u U3RhdHVzOjpDb250aW51ZTsKKyAgICAgICAgfSk7CisgICAgICAgIGlmIChmb3VuZFByZWNpc2VB bGxvY2F0aW9uKSB7CisgICAgICAgICAgICBzdWJzcGFjZUhhc2ggPSBTdHJpbmdIYXNoZXI6OmNv bXB1dGVIYXNoKGZvdW5kUHJlY2lzZUFsbG9jYXRpb24tPnN1YnNwYWNlKCktPm5hbWUoKSk7Cisg ICAgICAgICAgICBjZWxsU2l6ZSA9IGZvdW5kUHJlY2lzZUFsbG9jYXRpb24tPmNlbGxTaXplKCk7 CisKKyAgICAgICAgICAgIHZhcmlvdXNTdGF0ZSB8PSBzdGF0aWNfY2FzdDx1aW50NjRfdD4oaXNG cmVlTGlzdGVkKSA8PCAwOworICAgICAgICAgICAgdmFyaW91c1N0YXRlIHw9IHN0YXRpY19jYXN0 PHVpbnQ2NF90PighaXNGcmVlTGlzdGVkKSA8PCAxOworICAgICAgICAgICAgaWYgKCFpc0ZyZWVM aXN0ZWQpIHsKKyAgICAgICAgICAgICAgICB2YXJpb3VzU3RhdGUgfD0gc3RhdGljX2Nhc3Q8dWlu dDY0X3Q+KGZvdW5kUHJlY2lzZUFsbG9jYXRpb24tPmlzRW1wdHkoKSkgPDwgMjsKKyAgICAgICAg ICAgICAgICB2YXJpb3VzU3RhdGUgfD0gc3RhdGljX2Nhc3Q8dWludDY0X3Q+KGZvdW5kUHJlY2lz ZUFsbG9jYXRpb24tPmlzTmV3bHlBbGxvY2F0ZWQoKSkgPDwgNDsKKyAgICAgICAgICAgIH0KKyAg ICAgICAgfQogICAgIH0KIAogICAgIENSQVNIX1dJVEhfSU5GTyhjZWxsQWRkcmVzcywgaGVhZGVy V29yZCwgemFwUmVhc29uQW5kTW9yZSwgc3Vic3BhY2VIYXNoLCBjZWxsU2l6ZSwgZm91bmRCbG9j aywgdmFyaW91c1N0YXRlKTsK