214914 2020-07-29 01:49:43 -0700 Update some JSArrayBufferView comments and add some assertions. 2020-07-29 09:56:19 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mark.lam mark.lam darin ews-watchlist keith_miller msaboff saam tzagallo webkit-bug-importer ysuzuki oldest_to_newest 1675929 0 mark.lam 2020-07-29 01:49:43 -0700 ... 1675930 1 405444 mark.lam 2020-07-29 01:53:09 -0700 Created attachment 405444 proposed patch. 1676023 2 405444 darin 2020-07-29 09:39:44 -0700 Comment on attachment 405444 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=405444&action=review > Source/JavaScriptCore/runtime/ArrayBuffer.cpp:213 > + ASSERT(!Gigacage::isEnabled() || (Gigacage::contains(data) && Gigacage::contains(bitwise_cast<const uint8_t*>(data) + byteLength - 1))); Converting from const void* to const uint8_t* requires only a static_cast, not a bitwise_cast. We should reserve bitwise_cast for the cases where it’s needed. > Source/JavaScriptCore/runtime/JSArrayBufferView.cpp:51 > + ASSERT(!Gigacage::isEnabled() || (Gigacage::contains(vector) && Gigacage::contains(bitwise_cast<const uint8_t*>(vector) + length - 1))); Ditto. Also, I suggest casting to uint8_t* for brevity rather than adding const. That can still be passed to a function that expects const uint8_t. 1676033 3 405444 mark.lam 2020-07-29 09:50:54 -0700 Comment on attachment 405444 proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=405444&action=review Thanks for the review. >> Source/JavaScriptCore/runtime/ArrayBuffer.cpp:213 >> + ASSERT(!Gigacage::isEnabled() || (Gigacage::contains(data) && Gigacage::contains(bitwise_cast<const uint8_t*>(data) + byteLength - 1))); > > Converting from const void* to const uint8_t* requires only a static_cast, not a bitwise_cast. We should reserve bitwise_cast for the cases where it’s needed. I'll switch static_cast. const is still needed. Otherwise Clang will complain about: error: static_cast from 'const void *' to 'uint8_t *' (aka 'unsigned char *') casts away qualifiers. >> Source/JavaScriptCore/runtime/JSArrayBufferView.cpp:51 >> + ASSERT(!Gigacage::isEnabled() || (Gigacage::contains(vector) && Gigacage::contains(bitwise_cast<const uint8_t*>(vector) + length - 1))); > > Ditto. Also, I suggest casting to uint8_t* for brevity rather than adding const. That can still be passed to a function that expects const uint8_t. Ditto. 1676035 4 mark.lam 2020-07-29 09:55:37 -0700 Landed in r265045: <http://trac.webkit.org/r265045>. 1676036 5 webkit-bug-importer 2020-07-29 09:56:19 -0700 <rdar://problem/66278689> 405444 2020-07-29 01:53:09 -0700 2020-07-29 09:39:44 -0700 proposed patch. bug-214914.patch text/plain 5885 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjY1MDM1KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDIwLTA3LTI5ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBV cGRhdGUgc29tZSBKU0FycmF5QnVmZmVyVmlldyBjb21tZW50cyBhbmQgYWRkIHNvbWUgYXNzZXJ0 aW9ucy4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIx NDkxNAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICog cnVudGltZS9BcnJheUJ1ZmZlci5jcHA6CisgICAgICAgIChKU0M6OkFycmF5QnVmZmVyOjpjcmVh dGVBZG9wdGVkKToKKyAgICAgICAgKiBydW50aW1lL0pTQXJyYXlCdWZmZXJWaWV3LmNwcDoKKyAg ICAgICAgKEpTQzo6SlNBcnJheUJ1ZmZlclZpZXc6OkNvbnN0cnVjdGlvbkNvbnRleHQ6OkNvbnN0 cnVjdGlvbkNvbnRleHQpOgorICAgICAgICAoSlNDOjpKU0FycmF5QnVmZmVyVmlldzo6ZmluYWxp emUpOgorICAgICAgICAqIHJ1bnRpbWUvSlNBcnJheUJ1ZmZlclZpZXcuaDoKKwogMjAyMC0wNy0y OSAgWXVzdWtlIFN1enVraSAgPHlzdXp1a2lAYXBwbGUuY29tPgogCiAgICAgICAgIFtKU0NdIFJl ZmxlY3Qgb2JqZWN0IHNob3VsZCBoYXZlIHRvU3RyaW5nVGFnIHdpdGggIlJlZmxlY3QiCkluZGV4 OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9BcnJheUJ1ZmZlci5jcHAKPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvQXJyYXlCdWZmZXIuY3BwCShyZXZp c2lvbiAyNjUwMzEpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9BcnJheUJ1ZmZl ci5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTEsNSArMSw1IEBACiAvKgotICogQ29weXJpZ2h0IChD KSAyMDA5LTIwMTggQXBwbGUgSW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkLgorICogQ29weXJpZ2h0 IChDKSAyMDA5LTIwMjAgQXBwbGUgSW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkLgogICoKICAqIFJl ZGlzdHJpYnV0aW9uIGFuZCB1c2UgaW4gc291cmNlIGFuZCBiaW5hcnkgZm9ybXMsIHdpdGggb3Ig d2l0aG91dAogICogbW9kaWZpY2F0aW9uLCBhcmUgcGVybWl0dGVkIHByb3ZpZGVkIHRoYXQgdGhl IGZvbGxvd2luZyBjb25kaXRpb25zCkBAIC0yMTAsNiArMjEwLDcgQEAgUmVmPEFycmF5QnVmZmVy PiBBcnJheUJ1ZmZlcjo6Y3JlYXRlKEFycgogLy8gICBmcm9tIHRoZSBjYWdlLgogUmVmPEFycmF5 QnVmZmVyPiBBcnJheUJ1ZmZlcjo6Y3JlYXRlQWRvcHRlZChjb25zdCB2b2lkKiBkYXRhLCB1bnNp Z25lZCBieXRlTGVuZ3RoKQogeworICAgIEFTU0VSVCghR2lnYWNhZ2U6OmlzRW5hYmxlZCgpIHx8 IChHaWdhY2FnZTo6Y29udGFpbnMoZGF0YSkgJiYgR2lnYWNhZ2U6OmNvbnRhaW5zKGJpdHdpc2Vf Y2FzdDxjb25zdCB1aW50OF90Kj4oZGF0YSkgKyBieXRlTGVuZ3RoIC0gMSkpKTsKICAgICByZXR1 cm4gY3JlYXRlRnJvbUJ5dGVzKGRhdGEsIGJ5dGVMZW5ndGgsIEFycmF5QnVmZmVyOjpwcmltaXRp dmVHaWdhY2FnZURlc3RydWN0b3IoKSk7CiB9CiAKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29y ZS9ydW50aW1lL0pTQXJyYXlCdWZmZXJWaWV3LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2 YVNjcmlwdENvcmUvcnVudGltZS9KU0FycmF5QnVmZmVyVmlldy5jcHAJKHJldmlzaW9uIDI2NTAz MSkKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTQXJyYXlCdWZmZXJWaWV3LmNw cAkod29ya2luZyBjb3B5KQpAQCAtMSw1ICsxLDUgQEAKIC8qCi0gKiBDb3B5cmlnaHQgKEMpIDIw MTMtMjAxOSBBcHBsZSBJbmMuIEFsbCByaWdodHMgcmVzZXJ2ZWQuCisgKiBDb3B5cmlnaHQgKEMp IDIwMTMtMjAyMCBBcHBsZSBJbmMuIEFsbCByaWdodHMgcmVzZXJ2ZWQuCiAgKgogICogUmVkaXN0 cmlidXRpb24gYW5kIHVzZSBpbiBzb3VyY2UgYW5kIGJpbmFyeSBmb3Jtcywgd2l0aCBvciB3aXRo b3V0CiAgKiBtb2RpZmljYXRpb24sIGFyZSBwZXJtaXR0ZWQgcHJvdmlkZWQgdGhhdCB0aGUgZm9s bG93aW5nIGNvbmRpdGlvbnMKQEAgLTQ4LDYgKzQ4LDcgQEAgSlNBcnJheUJ1ZmZlclZpZXc6OkNv bnN0cnVjdGlvbkNvbnRleHQ6OgogICAgICwgbV9tb2RlKEZhc3RUeXBlZEFycmF5KQogICAgICwg bV9idXR0ZXJmbHkobnVsbHB0cikKIHsKKyAgICBBU1NFUlQoIUdpZ2FjYWdlOjppc0VuYWJsZWQo KSB8fCAoR2lnYWNhZ2U6OmNvbnRhaW5zKHZlY3RvcikgJiYgR2lnYWNhZ2U6OmNvbnRhaW5zKGJp dHdpc2VfY2FzdDxjb25zdCB1aW50OF90Kj4odmVjdG9yKSArIGxlbmd0aCAtIDEpKSk7CiAgICAg QVNTRVJUKHZlY3RvciA9PSByZW1vdmVBcnJheVB0clRhZyh2ZWN0b3IpKTsKICAgICBSRUxFQVNF X0FTU0VSVChsZW5ndGggPD0gZmFzdFNpemVMaW1pdCk7CiB9CkBAIC0xOTAsNiArMTkxLDEwIEBA IEFycmF5QnVmZmVyKiBKU0FycmF5QnVmZmVyVmlldzo6dW5zaGFyZWQKIHZvaWQgSlNBcnJheUJ1 ZmZlclZpZXc6OmZpbmFsaXplKEpTQ2VsbCogY2VsbCkKIHsKICAgICBKU0FycmF5QnVmZmVyVmll dyogdGhpc09iamVjdCA9IHN0YXRpY19jYXN0PEpTQXJyYXlCdWZmZXJWaWV3Kj4oY2VsbCk7CisK KyAgICAvLyBUaGlzIEpTQXJyYXlCdWZmZXJWaWV3IGNvdWxkIGJlIGFuIE92ZXJzaXplVHlwZWRB cnJheSB0aGF0IHdhcyBjb252ZXJ0ZWQKKyAgICAvLyB0byBhIFdhc3RlZnVsVHlwZWRBcnJheSB2 aWEgc2xvd0Rvd25BbmRXYXN0ZU1lbW9yeSgpLiBIZW5jZSwgaXQgaXMgcG9zc2libGUKKyAgICAv LyB0byBnZXQgdG8gdGhpcyBmaW5hbGl6ZXIgYW5kIGZvdW5kIHRoZSBtb2RlIHRvIGJlIFdhc3Rl ZnVsVHlwZWRBcnJheS4KICAgICBBU1NFUlQodGhpc09iamVjdC0+bV9tb2RlID09IE92ZXJzaXpl VHlwZWRBcnJheSB8fCB0aGlzT2JqZWN0LT5tX21vZGUgPT0gV2FzdGVmdWxUeXBlZEFycmF5KTsK ICAgICBpZiAodGhpc09iamVjdC0+bV9tb2RlID09IE92ZXJzaXplVHlwZWRBcnJheSkKICAgICAg ICAgR2lnYWNhZ2U6OmZyZWUoR2lnYWNhZ2U6OlByaW1pdGl2ZSwgdGhpc09iamVjdC0+dmVjdG9y KCkpOwpJbmRleDogU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNBcnJheUJ1ZmZlclZp ZXcuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU0FycmF5 QnVmZmVyVmlldy5oCShyZXZpc2lvbiAyNjUwMzEpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUv cnVudGltZS9KU0FycmF5QnVmZmVyVmlldy5oCSh3b3JraW5nIGNvcHkpCkBAIC0xLDUgKzEsNSBA QAogLyoKLSAqIENvcHlyaWdodCAoQykgMjAxMy0yMDE5IEFwcGxlIEluYy4gQWxsIHJpZ2h0cyBy ZXNlcnZlZC4KKyAqIENvcHlyaWdodCAoQykgMjAxMy0yMDIwIEFwcGxlIEluYy4gQWxsIHJpZ2h0 cyByZXNlcnZlZC4KICAqCiAgKiBSZWRpc3RyaWJ1dGlvbiBhbmQgdXNlIGluIHNvdXJjZSBhbmQg YmluYXJ5IGZvcm1zLCB3aXRoIG9yIHdpdGhvdXQKICAqIG1vZGlmaWNhdGlvbiwgYXJlIHBlcm1p dHRlZCBwcm92aWRlZCB0aGF0IHRoZSBmb2xsb3dpbmcgY29uZGl0aW9ucwpAQCAtNTUsMjUgKzU1 LDMwIEBAIGNsYXNzIExMSW50T2Zmc2V0c0V4dHJhY3RvcjsKIC8vIHdoZXRoZXIgdGhlIHVzZXIg aGFzIGRvbmUgYW55dGhpbmcgdGhhdCByZXF1aXJlcyBhIHNlcGFyYXRlIGJhY2tpbmcKIC8vIGJ1 ZmZlciBvciB0aGUgRE9NLXNwZWNpZmllZCBuZXV0ZXJpbmcgY2FwYWJpbGl0aWVzLgogZW51bSBU eXBlZEFycmF5TW9kZSA6IHVpbnQzMl90IHsKKyAgICAvLyBMZWdlbmQ6CisgICAgLy8gQjogSlNB cnJheUJ1ZmZlclZpZXc6Om1fYnV0dGVyZmx5IHBvaW50ZXIKKyAgICAvLyBWOiBKU0FycmF5QnVm ZmVyVmlldzo6bV92ZWN0b3IgcG9pbnRlcgorICAgIC8vIE06IEpTQXJyYXlCdWZmZXJWaWV3Ojpt X21vZGUKKwogICAgIC8vIFNtYWxsIGFuZCBmYXN0IHR5cGVkIGFycmF5LiBCIGlzIHVudXNlZCwg ViBwb2ludHMgdG8gYSB2ZWN0b3IKLSAgICAvLyBhbGxvY2F0ZWQgaW4gY29waWVkIHNwYWNlLCBh bmQgTSA9IEZhc3RUeXBlZEFycmF5LiBWJ3MgbGl2ZW5lc3MgaXMKLSAgICAvLyBkZXRlcm1pbmVk IGVudGlyZWx5IGJ5IHRoZSB2aWV3J3MgbGl2ZW5lc3MuCisgICAgLy8gYWxsb2NhdGVkIGluIHRo ZSBwcmltaXRpdmUgR2lnYWNhZ2UsIGFuZCBNID0gRmFzdFR5cGVkQXJyYXkuIFYncworICAgIC8v IGxpdmVuZXNzIGlzIGRldGVybWluZWQgZW50aXJlbHkgYnkgdGhlIHZpZXcncyBsaXZlbmVzcy4K ICAgICBGYXN0VHlwZWRBcnJheSwKLSAgICAKKwogICAgIC8vIEEgbGFyZ2UgdHlwZWQgYXJyYXkg dGhhdCBzdGlsbCBhdHRlbXB0cyBub3QgdG8gd2FzdGUgdG9vIG11Y2gKLSAgICAvLyBtZW1vcnku IEIgaXMgaW5pdGlhbGl6ZWQgdG8gcG9pbnQgdG8gYSBzbG90IHRoYXQgY291bGQgaG9sZCBhCi0g ICAgLy8gYnVmZmVyIHBvaW50ZXIsIFYgcG9pbnRzIHRvIGEgdmVjdG9yIGFsbG9jYXRlZCB1c2lu ZyBmYXN0Q2FsbG9jKCksCi0gICAgLy8gYW5kIE0gPSBPdmVyc2l6ZVR5cGVkQXJyYXkuIFYncyBs aXZlbmVzcyBpcyBkZXRlcm1pbmVkIGVudGlyZWx5IGJ5Ci0gICAgLy8gdGhlIHZpZXcncyBsaXZl bmVzcywgYW5kIHRoZSB2aWV3IHdpbGwgYWRkIGEgZmluYWxpemVyIHRvIGRlbGV0ZSBWLgorICAg IC8vIG1lbW9yeS4gQiBpcyB1bnVzZWQsIFYgcG9pbnRzIHRvIGEgdmVjdG9yIGFsbG9jYXRlZCB1 c2luZworICAgIC8vIEdpZ2FjYWdlOjp0cnlNYWxsb2MoKSwgYW5kIE0gPSBPdmVyc2l6ZVR5cGVk QXJyYXkuIFYncyBsaXZlbmVzcyBpcworICAgIC8vIGRldGVybWluZWQgZW50aXJlbHkgYnkgdGhl IHZpZXcncyBsaXZlbmVzcywgYW5kIHRoZSB2aWV3IHdpbGwgYWRkIGEKKyAgICAvLyBmaW5hbGl6 ZXIgdG8gZGVsZXRlIFYuCiAgICAgT3ZlcnNpemVUeXBlZEFycmF5LAotICAgIAorCiAgICAgLy8g QSB0eXBlZCBhcnJheSB0aGF0IHdhcyB1c2VkIGluIHNvbWUgY3Jhenkgd2F5LiBCJ3MgSW5kZXhp bmdIZWFkZXIKICAgICAvLyBpcyBoaWphY2tlZCB0byBjb250YWluIGEgcmVmZXJlbmNlIHRvIHRo ZSBuYXRpdmUgYXJyYXkgYnVmZmVyLiBUaGUKICAgICAvLyBuYXRpdmUgdHlwZWQgYXJyYXkgdmll dyBwb2ludHMgYmFjayB0byB0aGUgSlMgdmlldy4gViBwb2ludHMgdG8gYQogICAgIC8vIHZlY3Rv ciBhbGxvY2F0ZWQgdXNpbmcgd2hvLWtub3dzLXdoYXQsIGFuZCBNID0gV2FzdGVmdWxUeXBlZEFy cmF5LgogICAgIC8vIFRoZSB2aWV3IGRvZXMgbm90IG93biB0aGUgdmVjdG9yLgogICAgIFdhc3Rl ZnVsVHlwZWRBcnJheSwKLSAgICAKKwogICAgIC8vIEEgZGF0YSB2aWV3LiBCIGlzIHVudXNlZCwg ViBwb2ludHMgdG8gYSB2ZWN0b3IgYWxsb2NhdGVkIHVzaW5nIHdoby0KICAgICAvLyBrbm93cy13 aGF0LCBhbmQgTSA9IERhdGFWaWV3TW9kZS4gVGhlIHZpZXcgZG9lcyBub3Qgb3duIHRoZSB2ZWN0 b3IuCiAgICAgLy8gVGhlcmUgaXMgYW4gZXh0cmEgZmllbGQgKGluIEpTRGF0YVZpZXcpIHRoYXQg cG9pbnRzIHRvIHRoZQo=