177944 2017-10-05 08:25:19 -0700 Avoid integer overflow in DFGStrengthReduction.cpp 2017-10-06 09:34:46 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 rmorisset rmorisset buildbot commit-queue keith_miller mark.lam msaboff saam webkit-bug-importer oldest_to_newest 1357026 0 rmorisset 2017-10-05 08:25:19 -0700 The check that we won't cause integer overflow is itself a signed integer overflow! I believe this is undefined behavior, and a future compiler could remove the check altogether. Instead, make it an explicit check that value != INT32_MIN. 1357027 1 322838 rmorisset 2017-10-05 08:27:52 -0700 Created attachment 322838 Patch 1357032 2 322838 saam 2017-10-05 08:58:36 -0700 Comment on attachment 322838 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322838&action=review > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:158 > + if (value != INT32_MIN) { Why not use Checked here? 1357545 3 rmorisset 2017-10-06 03:15:10 -0700 (In reply to Saam Barati from comment #2) > Comment on attachment 322838 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322838&action=review > > > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:158 > > + if (value != INT32_MIN) { > > Why not use Checked here? I just did not know about the existence of Checked. I could use it here, but I am not sure it would be a win: the check here is trivial, and using Checked would be a bit more bothersome: Checked<int32_t, RecordOnOverflow> checkedNewValue = 0; checkedNewValue -= value; int32_t newValue; if (CheckedState::DidNotOverflow == checkedNewValue.safeGet(newValue)) { ... (newValue)... } Instead of if (value != INT32_MIN) { ... (-value) ... } If you think it would be better, I can do the change and use it, it is as you want. 1357592 4 saam 2017-10-06 08:56:05 -0700 (In reply to Robin Morisset from comment #3) > (In reply to Saam Barati from comment #2) > > Comment on attachment 322838 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=322838&action=review > > > > > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:158 > > > + if (value != INT32_MIN) { > > > > Why not use Checked here? > > I just did not know about the existence of Checked. > > I could use it here, but I am not sure it would be a win: the check here is > trivial, and using Checked would be a bit more bothersome: > Checked<int32_t, RecordOnOverflow> checkedNewValue = 0; > checkedNewValue -= value; > int32_t newValue; > if (CheckedState::DidNotOverflow == checkedNewValue.safeGet(newValue)) { > ... (newValue)... > } > Instead of > if (value != INT32_MIN) { > ... (-value) ... > } > > If you think it would be better, I can do the change and use it, it is as > you want. Keep it as you have it. r=me still 1357607 5 322838 commit-queue 2017-10-06 09:33:46 -0700 Comment on attachment 322838 Patch Clearing flags on attachment: 322838 Committed r222981: <http://trac.webkit.org/changeset/222981> 1357608 6 commit-queue 2017-10-06 09:33:47 -0700 All reviewed patches have been landed. Closing bug. 1357609 7 webkit-bug-importer 2017-10-06 09:34:46 -0700 <rdar://problem/34857276> 322838 2017-10-05 08:27:52 -0700 2017-10-06 09:33:46 -0700 Patch bug-177944-20171005172751.patch text/plain 1782 rmorisset U3VidmVyc2lvbiBSZXZpc2lvbjogMjIyOTA4CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA1 YzRlNGM5ZDEyODI1OTNlN2FhZGZmNDMwNDEwZTZiOWE3NTc2ZGU1Li45MzVkZDdhZDk1ZWVhNGEy NWVhNDQxNGIyNzAxY2IxZDdhNzEwM2VlIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNiBAQAorMjAxNy0xMC0wNSAgUm9iaW4gTW9yaXNzZXQgIDxybW9yaXNzZXRAYXBwbGUu Y29tPgorCisgICAgICAgIEF2b2lkIGludGVnZXIgb3ZlcmZsb3cgaW4gREZHU3RyZW5ndGhSZWR1 Y3Rpb24uY3BwCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xNzc5NDQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBUaGUgY2hlY2sgdGhhdCB3ZSB3b24ndCBkbyBpbnRlZ2VyIG92ZXJmbG93IGJ5IG5lZ2F0aW5n IElOVDMyX01JTiB3YXMgaXRzZWxmIGFuIGludGVnZXIgb3ZlcmZsb3cuCisgICAgICAgIEkgdGhp bmsgdGhhdCBzaWduZWQgaW50ZWdlciBvdmVyZmxvdyBpcyB1bmRlZmluZWQgYmVoYXZpb3VyIGlu IEMsIHNvIEkgcmVwbGFjZSBpdCBieSBhbiBleHBsaWNpdCBjaGVjayB0aGF0IHZhbHVlICE9IElO VDMyX01JTiBpbnN0ZWFkLgorCisgICAgICAgICogZGZnL0RGR1N0cmVuZ3RoUmVkdWN0aW9uUGhh c2UuY3BwOgorICAgICAgICAoSlNDOjpERkc6OlN0cmVuZ3RoUmVkdWN0aW9uUGhhc2U6OmhhbmRs ZU5vZGUpOgorCiAyMDE3LTEwLTA1ICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgog CiAgICAgICAgIE1ha2Ugc3VyZSBhbGwgcHJvdG90eXBlcyB1bmRlciBwb2x5IHByb3RvIGdldCBh ZGRlZCBpbnRvIHRoZSBWTSdzIHByb3RvdHlwZSBtYXAKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZh U2NyaXB0Q29yZS9kZmcvREZHU3RyZW5ndGhSZWR1Y3Rpb25QaGFzZS5jcHAgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvZGZnL0RGR1N0cmVuZ3RoUmVkdWN0aW9uUGhhc2UuY3BwCmluZGV4IGY5MTU4 NDFhOWQ0NmIxN2IwNjM5OGI0M2M2ZjI2ZTc3NzBkNTdkYTUuLmM0YjlhMWYxMjZlMjcyNTBhN2Y2 ZDZiNTIwNDA3OTdmMGZkNDEzYTEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9k ZmcvREZHU3RyZW5ndGhSZWR1Y3Rpb25QaGFzZS5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL2RmZy9ERkdTdHJlbmd0aFJlZHVjdGlvblBoYXNlLmNwcApAQCAtMTU1LDcgKzE1NSw3IEBA IHByaXZhdGU6CiAgICAgICAgICAgICBpZiAobV9ub2RlLT5jaGlsZDIoKS0+aXNJbnQzMkNvbnN0 YW50KCkKICAgICAgICAgICAgICAgICAmJiBtX25vZGUtPmlzQmluYXJ5VXNlS2luZChJbnQzMlVz ZSkpIHsKICAgICAgICAgICAgICAgICBpbnQzMl90IHZhbHVlID0gbV9ub2RlLT5jaGlsZDIoKS0+ YXNJbnQzMigpOwotICAgICAgICAgICAgICAgIGlmICgtdmFsdWUgIT0gdmFsdWUpIHsKKyAgICAg ICAgICAgICAgICBpZiAodmFsdWUgIT0gSU5UMzJfTUlOKSB7CiAgICAgICAgICAgICAgICAgICAg IG1fbm9kZS0+c2V0T3AoQXJpdGhBZGQpOwogICAgICAgICAgICAgICAgICAgICBtX25vZGUtPmNo aWxkMigpLnNldE5vZGUoCiAgICAgICAgICAgICAgICAgICAgICAgICBtX2luc2VydGlvblNldC5p bnNlcnRDb25zdGFudCgK