188741 2018-08-20 02:40:51 -0700 [JSC] Suppress UBSan warnings for KeywordLookup 2020-06-12 20:29:23 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified NEW P2 Normal --- 196533 1 ysuzuki ysuzuki benjamin cdumez cmarcelo dbates don.olmstead ews-watchlist fpizlo fujii keith_miller mark.lam mcatanzaro msaboff saagarjha saam oldest_to_newest 1451561 0 ysuzuki 2018-08-20 02:40:51 -0700 [JSC] Suppress UBSan warnings for KeywordLookup 1451563 1 347489 ysuzuki 2018-08-20 02:47:14 -0700 Created attachment 347489 Patch 1453374 2 fujii 2018-08-23 19:46:34 -0700 Can you use WTF::unalignedLoad instead of reinterpret_cast in this case? There is a code path for CPU(NEEDS_ALIGNED_ACCESS). How about the idea to use the code path if UBSan is enabled? 1453375 3 fujii 2018-08-23 19:47:57 -0700 Don, IIUC, we (SIE) want to remove undefined behaviors as much as possible for CFI. Right? Control Flow Integrity https://clang.llvm.org/docs/ControlFlowIntegrity.html 1453474 4 don.olmstead 2018-08-24 10:09:38 -0700 (In reply to Fujii Hironori from comment #3) > Don, > > IIUC, we (SIE) want to remove undefined behaviors as much as possible for > CFI. Right? > > Control Flow Integrity > https://clang.llvm.org/docs/ControlFlowIntegrity.html For CFI the unaligned loads should not be a problem. Patch as is looks fine I'm just interested in knowing if something like the WTF functions mentioned would be of use here just for reuse purposes. 1523879 5 347489 fpizlo 2019-04-03 13:33:02 -0700 Comment on attachment 347489 Patch I’m not sure we should be splatting these blacklist things - it makes the code uglier. 1523882 6 don.olmstead 2019-04-03 13:35:56 -0700 (In reply to Filip Pizlo from comment #5) > Comment on attachment 347489 [details] > Patch > > I’m not sure we should be splatting these blacklist things - it makes the > code uglier. We can use a blacklist file or annotate the code. The benefit from having it in code is that you can see right away that the code is relying on undefined behavior. I don't have a strong opinion either way. 1526773 7 mcatanzaro 2019-04-12 09:57:50 -0700 (In reply to Filip Pizlo from comment #5) > Comment on attachment 347489 [details] > Patch > > I’m not sure we should be splatting these blacklist things - it makes the > code uglier. This looks like really low-cost to me. WebKit already has SUPPRESS_ASAN because we recognize asan is an extremely valuable tool for finding security bugs, and using SUPPRESS_ASAN in a couple odd places is less-invasive than rewriting those places to not trigger asan. (If we don't rewrite the code, and don't use SUPPRESS_ASAN, then we simply can't use asan.) Now, ubsan is less-valuable than asan, but that's only because asan is really really good. Making a couple small tweaks to the code like this to please ubsan seems valuable to me, since it will make it much easier to run WebKit under ubsan. This seems much more valuable to me than a blacklist file, since that would require extra work to configure and would reduce the ease of running WebKit under ubsan. 1526782 8 fpizlo 2019-04-12 10:14:12 -0700 (In reply to Michael Catanzaro from comment #7) > (In reply to Filip Pizlo from comment #5) > > Comment on attachment 347489 [details] > > Patch > > > > I’m not sure we should be splatting these blacklist things - it makes the > > code uglier. > > This looks like really low-cost to me. WebKit already has SUPPRESS_ASAN > because we recognize asan is an extremely valuable tool for finding security > bugs, and using SUPPRESS_ASAN in a couple odd places is less-invasive than > rewriting those places to not trigger asan. (If we don't rewrite the code, > and don't use SUPPRESS_ASAN, then we simply can't use asan.) I don’t mind these annotations for asan. I was only talking about ubsan. > > Now, ubsan is less-valuable than asan, but that's only because asan is > really really good. Making a couple small tweaks to the code like this to > please ubsan seems valuable to me, since it will make it much easier to run > WebKit under ubsan. This seems much more valuable to me than a blacklist > file, since that would require extra work to configure and would reduce the > ease of running WebKit under ubsan. Ubsan is such a silly concept. The case of reporting bad shift amounts as errors is particularly silly: the only fix for that is to mask the shift amount. If that’s the only fix then we should ask for something that just applies those masks for us everywhere. The same argument can be made for most other kinds of UB. 1553513 9 saagarjha 2019-07-17 08:33:59 -0700 The code exhibiting undefined behavior that this bug mentions has been rewritten to be standards-compliant in https://bugs.webkit.org/show_bug.cgi?id=199650, so I don't think we need that part of the changeset anymore. I think that adding annotations is still useful, however: there are lot of places in WebKit that remain noncompliant, and an annotation like this would be useful to mark all the places we haven't gotten around to fixing yet as well as ensuring that new code remains free of undefined behavior. 1662313 10 347489 ysuzuki 2020-06-12 20:29:23 -0700 Comment on attachment 347489 Patch For now, I'm not planning to do this. 347489 2018-08-20 02:47:14 -0700 2020-06-12 20:29:23 -0700 Patch bug-188741-20180820184714.patch text/plain 3384 ysuzuki U3VidmVyc2lvbiBSZXZpc2lvbjogMjM1MDI1CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA4 ZjRmMzM4MGI5ZDY4MDgxZjE0YjQ1YTVjYzUzZDNiNzA5ODNmZjkxLi5hNzI5OTkzN2QwNmFmMWU4 NjkyYTJmNzc2NTQ1YThhYjkxMDk1YmZiIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNiBAQAorMjAxOC0wOC0yMCAgWXVzdWtlIFN1enVraSAgPHl1c3VrZXN1enVraUBzbG93 c3RhcnQub3JnPgorCisgICAgICAgIFtKU0NdIFN1cHByZXNzIFVCU2FuIHdhcm5pbmdzIGZvciBL ZXl3b3JkTG9va3VwCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xODg3NDEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBBbm5vdGF0ZSBLZXl3b3JkTG9va3VwIGZ1bmN0aW9uIHdpdGggU1VQUFJFU1NfVUJTQU4g c2luY2UgaXQgdXRpbGl6ZXMgdW5hbGlnbmVkCisgICAgICAgIGFjY2Vzc2VzIGhlYXZpbHkgZm9y IHNvbWUgQ1BVcyBzdWNoIGFzIHg4Ni4KKworICAgICAgICAqIEtleXdvcmRMb29rdXBHZW5lcmF0 b3IucHk6CisgICAgICAgIChUcmllLnByaW50QXNDKToKKwogMjAxOC0wOC0xOSAgQ2FybG9zIEdh cmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CiAKICAgICAgICAgW0dMSUJdIEFkZCBB UEkgdG8gdGhyb3cgZXhjZXB0aW9ucyB1c2luZyBwcmludGYgZm9ybWF0dGVkIHN0cmluZ3MKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nIGIvU291cmNlL1dURi9DaGFuZ2VMb2cKaW5k ZXggZWFjMTY4NWY3NTQwMGZkMjE1MzViNDM1ZjIyZGE4MjUwMzBlNDViOC4uODVmYWViM2I0MTIy N2JmMmUyYzE3NjI4NTEyYWJhNzZhMjM2MDk5NSAxMDA2NDQKLS0tIGEvU291cmNlL1dURi9DaGFu Z2VMb2cKKysrIGIvU291cmNlL1dURi9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNCBAQAorMjAxOC0w OC0yMCAgWXVzdWtlIFN1enVraSAgPHl1c3VrZXN1enVraUBzbG93c3RhcnQub3JnPgorCisgICAg ICAgIFtKU0NdIFN1cHByZXNzIFVCU2FuIHdhcm5pbmdzIGZvciBLZXl3b3JkTG9va3VwCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODg3NDEKKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBBZGQgU1VQUFJFU1Nf VUJTQU4KKworICAgICAgICAqIHd0Zi9Db21waWxlci5oOgorCiAyMDE4LTA4LTE5ICBZdXN1a2Ug U3V6dWtpICA8eXVzdWtlc3V6dWtpQHNsb3dzdGFydC5vcmc+CiAKICAgICAgICAgW1dURl0gQWRk IFdURjo6dW5hbGlnbmVkTG9hZCBhbmQgV1RGOjp1bmFsaWduZWRTdG9yZQpkaWZmIC0tZ2l0IGEv U291cmNlL0phdmFTY3JpcHRDb3JlL0tleXdvcmRMb29rdXBHZW5lcmF0b3IucHkgYi9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvS2V5d29yZExvb2t1cEdlbmVyYXRvci5weQppbmRleCBkMTNkYWJhNjFk MjQ3NThiMGEwNjJhOTY5NWExZTE4MjUwOTRjNDZlLi5hY2M0YTA1M2FiMDJlYTlhYzhhYTRlYWNi MTUyN2Q3NDg2NzNhMWIwIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvS2V5d29y ZExvb2t1cEdlbmVyYXRvci5weQorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvS2V5d29yZExv b2t1cEdlbmVyYXRvci5weQpAQCAtMTkwLDcgKzE5MCw3IEBAIGRlZiBwcmludEFzQyhzZWxmKToK ICAgICAgICAgcHJpbnQoInN0YXRpYyBjb25zdCBpbnQgbWF4VG9rZW5MZW5ndGggPSAlZDsiICUg KHNlbGYubWF4TGVuZ3RoKCkgKyAxKSkKICAgICAgICAgcHJpbnQoIiIpCiAgICAgICAgIHByaW50 KCJ0ZW1wbGF0ZSA8PiIpCi0gICAgICAgIHByaW50KCJ0ZW1wbGF0ZSA8Ym9vbCBzaG91bGRDcmVh dGVJZGVudGlmaWVyPiBBTFdBWVNfSU5MSU5FIEpTVG9rZW5UeXBlIExleGVyPFVDaGFyPjo6cGFy c2VLZXl3b3JkKEpTVG9rZW5EYXRhKiBkYXRhKSIpCisgICAgICAgIHByaW50KCJ0ZW1wbGF0ZSA8 Ym9vbCBzaG91bGRDcmVhdGVJZGVudGlmaWVyPiBTVVBQUkVTU19VQlNBTiBBTFdBWVNfSU5MSU5F IEpTVG9rZW5UeXBlIExleGVyPFVDaGFyPjo6cGFyc2VLZXl3b3JkKEpTVG9rZW5EYXRhKiBkYXRh KSIpCiAgICAgICAgIHByaW50KCJ7IikKICAgICAgICAgcHJpbnQoIiAgICBBU1NFUlQobV9jb2Rl RW5kIC0gbV9jb2RlID49IG1heFRva2VuTGVuZ3RoKTsiKQogICAgICAgICBwcmludCgiIikKQEAg LTIwMCw3ICsyMDAsNyBAQCBkZWYgcHJpbnRBc0Moc2VsZik6CiAgICAgICAgIHByaW50KCJ9IikK ICAgICAgICAgcHJpbnQoIiIpCiAgICAgICAgIHByaW50KCJ0ZW1wbGF0ZSA8PiIpCi0gICAgICAg IHByaW50KCJ0ZW1wbGF0ZSA8Ym9vbCBzaG91bGRDcmVhdGVJZGVudGlmaWVyPiBBTFdBWVNfSU5M SU5FIEpTVG9rZW5UeXBlIExleGVyPExDaGFyPjo6cGFyc2VLZXl3b3JkKEpTVG9rZW5EYXRhKiBk YXRhKSIpCisgICAgICAgIHByaW50KCJ0ZW1wbGF0ZSA8Ym9vbCBzaG91bGRDcmVhdGVJZGVudGlm aWVyPiBTVVBQUkVTU19VQlNBTiBBTFdBWVNfSU5MSU5FIEpTVG9rZW5UeXBlIExleGVyPExDaGFy Pjo6cGFyc2VLZXl3b3JkKEpTVG9rZW5EYXRhKiBkYXRhKSIpCiAgICAgICAgIHByaW50KCJ7IikK ICAgICAgICAgcHJpbnQoIiAgICBBU1NFUlQobV9jb2RlRW5kIC0gbV9jb2RlID49IG1heFRva2Vu TGVuZ3RoKTsiKQogICAgICAgICBwcmludCgiIikKZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvd3Rm L0NvbXBpbGVyLmggYi9Tb3VyY2UvV1RGL3d0Zi9Db21waWxlci5oCmluZGV4IDc5OGMzMDkwYzli MWMxODU4MGEzYTZiZTZmYWI2NWJkYWNlNmZmYWEuLmVkYWViYWNkZjlmZGY4NzNmZjgzYTExMzk5 NzBkZjY0YjUxN2VkYjcgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvd3RmL0NvbXBpbGVyLmgKKysr IGIvU291cmNlL1dURi93dGYvQ29tcGlsZXIuaApAQCAtMTU3LDYgKzE1NywxNCBAQAogI2RlZmlu ZSBTVVBQUkVTU19BU0FOCiAjZW5kaWYKIAorLyogU1VQUFJFU1NfVUJTQU4gKi8KKworI2lmIENP TVBJTEVSKEdDQ19PUl9DTEFORykKKyNkZWZpbmUgU1VQUFJFU1NfVUJTQU4gX19hdHRyaWJ1dGVf Xygobm9fc2FuaXRpemUoInVuZGVmaW5lZCIpKSkKKyNlbHNlCisjZGVmaW5lIFNVUFBSRVNTX1VC U0FOCisjZW5kaWYKKwogLyogPT09PSBDb21waWxlci1pbmRlcGVuZGVudCBtYWNyb3MgZm9yIHZh cmlvdXMgY29tcGlsZXIgZmVhdHVyZXMsIGluIGFscGhhYmV0aWNhbCBvcmRlciA9PT09ICovCiAK IC8qIEFMV0FZU19JTkxJTkUgKi8K