210314 2020-04-09 19:00:11 -0700 Fix bad tests in testmasm's testCagePreservesPACFailureBit(). 2020-04-10 13:20:31 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 210252 1 mark.lam mark.lam ews-watchlist keith_miller msaboff saam tzagallo webkit-bug-importer ysuzuki oldest_to_newest 1639991 0 mark.lam 2020-04-09 19:00:11 -0700 Some of these tests will crash when validated untagging is enabled. 1639992 1 webkit-bug-importer 2020-04-09 19:00:34 -0700 <rdar://problem/61556785> 1639993 2 396032 mark.lam 2020-04-09 19:10:13 -0700 Created attachment 396032 proposed patch. 1640047 3 396032 ysuzuki 2020-04-09 23:18:18 -0700 Comment on attachment 396032 proposed patch. r=me 1640048 4 mark.lam 2020-04-09 23:26:34 -0700 Thanks for the review. Landed in r259848: <http://trac.webkit.org/r259848>. 1640241 5 396032 keith_miller 2020-04-10 10:19:34 -0700 Comment on attachment 396032 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=396032&action=review > Source/JavaScriptCore/assembler/testmasm.cpp:2136 > + CHECK_NOT_EQ(invoke<void*>(cage, taggedNotCagedPtr, 1), ptr); This isn't quite a strong as the old test. It doesn't guarantee that the cage gave you a pointer that will crash when dereferenced. However, I think that can be tested by also asserting, CHECK_NOT_EQ(invoke<void*>(cage, taggedNotCagedPtr, 1), removeArrayPtrTag(invoke<void*>(cage, taggedNotCagedPtr, 1)))); 1640311 6 mark.lam 2020-04-10 12:34:58 -0700 Landed the additional test in r259890: <http://trac.webkit.org/r259890>. 1640312 7 396032 saam 2020-04-10 12:53:44 -0700 Comment on attachment 396032 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=396032&action=review > Source/JavaScriptCore/ChangeLog:9 > + Some of these tests will crash when validated untagging is enabled. why? 1640313 8 396032 saam 2020-04-10 12:54:01 -0700 Comment on attachment 396032 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=396032&action=review >> Source/JavaScriptCore/ChangeLog:9 >> + Some of these tests will crash when validated untagging is enabled. > > why? (You should say that here, not just what, but why) 1640315 9 mark.lam 2020-04-10 12:59:28 -0700 (In reply to Saam Barati from comment #8) > Comment on attachment 396032 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396032&action=review > > >> Source/JavaScriptCore/ChangeLog:9 > >> + Some of these tests will crash when validated untagging is enabled. > > > > why? > > (You should say that here, not just what, but why) It's in the title: "when validated untagging is enabled". When we start validating the untagged pointer (as in crash if validation fails), we will crash if we are untagging badly tagged pointers on purpose in the test. 1640316 10 mark.lam 2020-04-10 13:00:22 -0700 (In reply to Mark Lam from comment #9) > (In reply to Saam Barati from comment #8) > > Comment on attachment 396032 [details] > > proposed patch. > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=396032&action=review > > > > >> Source/JavaScriptCore/ChangeLog:9 > > >> + Some of these tests will crash when validated untagging is enabled. > > > > > > why? > > > > (You should say that here, not just what, but why) > > It's in the title: "when validated untagging is enabled". When we start > validating the untagged pointer (as in crash if validation fails), we will > crash if we are untagging badly tagged pointers on purpose in the test. Sorry, not title. It's in the bug and ChangeLog description. 1640318 11 396032 saam 2020-04-10 13:03:17 -0700 Comment on attachment 396032 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=396032&action=review >> Source/JavaScriptCore/assembler/testmasm.cpp:2136 >> + CHECK_NOT_EQ(invoke<void*>(cage, taggedNotCagedPtr, 1), ptr); > > This isn't quite a strong as the old test. It doesn't guarantee that the cage gave you a pointer that will crash when dereferenced. However, I think that can be tested by also asserting, > > CHECK_NOT_EQ(invoke<void*>(cage, taggedNotCagedPtr, 1), removeArrayPtrTag(invoke<void*>(cage, taggedNotCagedPtr, 1)))); can't we just assert using some functions that we're looking at unmapped memory? 1640323 12 396032 keith_miller 2020-04-10 13:20:31 -0700 Comment on attachment 396032 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=396032&action=review >>> Source/JavaScriptCore/assembler/testmasm.cpp:2136 >>> + CHECK_NOT_EQ(invoke<void*>(cage, taggedNotCagedPtr, 1), ptr); >> >> This isn't quite a strong as the old test. It doesn't guarantee that the cage gave you a pointer that will crash when dereferenced. However, I think that can be tested by also asserting, >> >> CHECK_NOT_EQ(invoke<void*>(cage, taggedNotCagedPtr, 1), removeArrayPtrTag(invoke<void*>(cage, taggedNotCagedPtr, 1)))); > > can't we just assert using some functions that we're looking at unmapped memory? The point is to ensure you have PAC bits. 396032 2020-04-09 19:10:13 -0700 2020-04-09 23:18:18 -0700 proposed patch. bug-210314.patch text/plain 2826 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjU5ODQyKQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBA CisyMDIwLTA0LTA5ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBG aXggYmFkIHRlc3RzIGluIHRlc3RtYXNtJ3MgdGVzdENhZ2VQcmVzZXJ2ZXNQQUNGYWlsdXJlQml0 KCkuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMTAz MTQKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzYxNTU2Nzg1PgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFNvbWUgb2YgdGhlc2UgdGVzdHMgd2lsbCBj cmFzaCB3aGVuIHZhbGlkYXRlZCB1bnRhZ2dpbmcgaXMgZW5hYmxlZC4KKworICAgICAgICAqIGFz c2VtYmxlci90ZXN0bWFzbS5jcHA6CisgICAgICAgIChKU0M6OnRlc3RDYWdlUHJlc2VydmVzUEFD RmFpbHVyZUJpdCk6CisKIDIwMjAtMDQtMDkgIERldmluIFJvdXNzbyAgPGRyb3Vzc29AYXBwbGUu Y29tPgogCiAgICAgICAgIERvbid0IGVtaXQgdGhlIHJocyB0d2ljZSBpbiBgQXNzaWduUmVzb2x2 ZU5vZGVgCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvYXNzZW1ibGVyL3Rlc3RtYXNtLmNw cAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvYXNzZW1ibGVyL3Rlc3RtYXNt LmNwcAkocmV2aXNpb24gMjU5ODQyKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxl ci90ZXN0bWFzbS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTEsNSArMSw1IEBACiAvKgotICogQ29w eXJpZ2h0IChDKSAyMDE3LTIwMTkgQXBwbGUgSW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkLgorICog Q29weXJpZ2h0IChDKSAyMDE3LTIwMjAgQXBwbGUgSW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkLgog ICoKICAqIFJlZGlzdHJpYnV0aW9uIGFuZCB1c2UgaW4gc291cmNlIGFuZCBiaW5hcnkgZm9ybXMs IHdpdGggb3Igd2l0aG91dAogICogbW9kaWZpY2F0aW9uLCBhcmUgcGVybWl0dGVkIHByb3ZpZGVk IHRoYXQgdGhlIGZvbGxvd2luZyBjb25kaXRpb25zCkBAIC0yMTMyLDkgKzIxMzIsOCBAQCBzdGF0 aWMgdm9pZCB0ZXN0Q2FnZVByZXNlcnZlc1BBQ0ZhaWx1cmVCCiAgICAgdm9pZCogdGFnZ2VkTm90 Q2FnZWRQdHIgPSB0YWdBcnJheVB0cihub3RDYWdlZFB0ciwgMSk7CiAKICAgICBpZiAoaXNBUk02 NEUoKSkgewotICAgICAgICAvLyBGSVhNRTogVGhpcyB3b24ndCB3b3JrIGlmIGF1dGhlbnRpY2F0 aW9uIGZhaWx1cmVzIHRyYXAgYnV0IEkgZG9uJ3Qga25vdyBob3cgdG8gdGVzdCBmb3IgdGhhdCBy aWdodCBub3cuCiAgICAgICAgIENIRUNLX05PVF9FUShpbnZva2U8dm9pZCo+KGNhZ2UsIHRhZ2dl ZFB0ciwgMiksIHB0cik7Ci0gICAgICAgIENIRUNLX0VRKGludm9rZTx2b2lkKj4oY2FnZSwgdGFn Z2VkTm90Q2FnZWRQdHIsIDEpLCB1bnRhZ0FycmF5UHRyKHRhZ2dlZFB0ciwgMikpOworICAgICAg ICBDSEVDS19OT1RfRVEoaW52b2tlPHZvaWQqPihjYWdlLCB0YWdnZWROb3RDYWdlZFB0ciwgMSks IHB0cik7CiAgICAgfSBlbHNlCiAgICAgICAgIENIRUNLX0VRKGludm9rZTx2b2lkKj4oY2FnZSwg dGFnZ2VkUHRyLCAyKSwgcHRyKTsKIApAQCAtMjE1MCwxMSArMjE0OSwxMCBAQCBzdGF0aWMgdm9p ZCB0ZXN0Q2FnZVByZXNlcnZlc1BBQ0ZhaWx1cmVCCiAKICAgICBDSEVDS19FUShpbnZva2U8dm9p ZCo+KGNhZ2VXaXRob3V0QXV0aGVudGljYXRpb24sIHRhZ2dlZFB0ciksIHRhZ2dlZFB0cik7CiAg ICAgaWYgKGlzQVJNNjRFKCkpIHsKLSAgICAgICAgLy8gRklYTUU6IFRoaXMgd29uJ3Qgd29yayBp ZiBhdXRoZW50aWNhdGlvbiBmYWlsdXJlcyB0cmFwIGJ1dCBJIGRvbid0IGtub3cgaG93IHRvIHRl c3QgZm9yIHRoYXQgcmlnaHQgbm93LgogICAgICAgICBDSEVDS19OT1RfRVEoaW52b2tlPHZvaWQq PihjYWdlV2l0aG91dEF1dGhlbnRpY2F0aW9uLCB0YWdnZWROb3RDYWdlZFB0ciksIHRhZ2dlZE5v dENhZ2VkUHRyKTsKLSAgICAgICAgQ0hFQ0tfTk9UX0VRKHVudGFnQXJyYXlQdHIoaW52b2tlPHZv aWQqPihjYWdlV2l0aG91dEF1dGhlbnRpY2F0aW9uLCB0YWdnZWROb3RDYWdlZFB0ciksIDEpLCBu b3RDYWdlZFB0cik7CisgICAgICAgIENIRUNLX05PVF9FUShpbnZva2U8dm9pZCo+KGNhZ2VXaXRo b3V0QXV0aGVudGljYXRpb24sIHRhZ2dlZE5vdENhZ2VkUHRyKSwgdGFnQXJyYXlQdHIobm90Q2Fn ZWRQdHIsIDEpKTsKICAgICAgICAgQ0hFQ0tfTk9UX0VRKGludm9rZTx2b2lkKj4oY2FnZVdpdGhv dXRBdXRoZW50aWNhdGlvbiwgdGFnZ2VkTm90Q2FnZWRQdHIpLCB0YWdnZWRQdHIpOwotICAgICAg ICBDSEVDS19OT1RfRVEodW50YWdBcnJheVB0cihpbnZva2U8dm9pZCo+KGNhZ2VXaXRob3V0QXV0 aGVudGljYXRpb24sIHRhZ2dlZE5vdENhZ2VkUHRyKSwgMSksIHB0cik7CisgICAgICAgIENIRUNL X05PVF9FUShpbnZva2U8dm9pZCo+KGNhZ2VXaXRob3V0QXV0aGVudGljYXRpb24sIHRhZ2dlZE5v dENhZ2VkUHRyKSwgdGFnQXJyYXlQdHIocHRyLCAxKSk7CiAgICAgfQogCiAgICAgR2lnYWNhZ2U6 OmZyZWUoR2lnYWNhZ2U6OlByaW1pdGl2ZSwgcHRyKTsK