163955 2016-10-25 10:59:53 -0700 Explore increasing max JSString::m_length to UINT_MAX. 2017-08-25 14:53:26 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Local Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 mark.lam keith_miller commit-queue fpizlo ggaren jfbastien keith_miller msaboff saam webkit-bug-importer ysuzuki oldest_to_newest 1244281 0 mark.lam 2016-10-25 10:59:53 -0700 If there's no perf implications, we should increase max JSString::m_length to UINT_MAX to match the max StringImpl length. 1342407 1 319083 keith_miller 2017-08-25 09:33:09 -0700 Created attachment 319083 Patch 1342409 2 319085 keith_miller 2017-08-25 09:36:33 -0700 Created attachment 319085 Patch 1342410 3 keith_miller 2017-08-25 09:37:15 -0700 rdar://problem/32001499 1342411 4 keith_miller 2017-08-25 09:38:33 -0700 I'll watch the bots for perf changes. I doubt this should make a difference to perf 1342413 5 319085 jfbastien 2017-08-25 09:40:12 -0700 Comment on attachment 319085 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319085&action=review What prevents size overflow without this check? Don't you want to check UINT_MAX instead of INT_MAX? > Source/JavaScriptCore/ChangeLog:3 > + Explore increasing max JSString::m_length to UINT_MAX. Weird to still be "exploring" here :) 1342416 6 319085 jfbastien 2017-08-25 09:43:36 -0700 Comment on attachment 319085 Patch OK Keith pointed out that size_t is misleading because string's .length returns unsigned. The assumption I have now is that tryMakeString will fail on its own, so this patch is OK. Please confirm that's the case before committing. r=me if so. Maybe needs a test? 1342434 7 319085 commit-queue 2017-08-25 10:27:50 -0700 Comment on attachment 319085 Patch Clearing flags on attachment: 319085 Committed r221192: <http://trac.webkit.org/changeset/221192> 1342435 8 commit-queue 2017-08-25 10:27:52 -0700 All reviewed patches have been landed. Closing bug. 1342492 9 ggaren 2017-08-25 13:18:54 -0700 I don't think this patch can be right. It deletes an ASSERT and a comment explaining a problem, but it doesn't fix the problem. Consider ThunkGenerators.cpp::stringCharLoad and similar functions: // load index jit.loadInt32Argument(0, SpecializedThunkJIT::regT1); // regT1 contains the index // Do an unsigned compare to simultaneously filter negative indices as well as indices that are too large jit.appendFailure(jit.branch32(MacroAssembler::AboveOrEqual, SpecializedThunkJIT::regT1, SpecializedThunkJIT::regT2)); ... 1342502 10 keith_miller 2017-08-25 13:44:22 -0700 (In reply to Geoffrey Garen from comment #9) > I don't think this patch can be right. It deletes an ASSERT and a comment > explaining a problem, but it doesn't fix the problem. > > Consider ThunkGenerators.cpp::stringCharLoad and similar functions: > > // load index > jit.loadInt32Argument(0, SpecializedThunkJIT::regT1); // regT1 contains > the index > > // Do an unsigned compare to simultaneously filter negative indices as > well as indices that are too large > jit.appendFailure(jit.branch32(MacroAssembler::AboveOrEqual, > SpecializedThunkJIT::regT1, SpecializedThunkJIT::regT2)); > > ... I think this is fine. That thunk calls the slow path (the C++ code) if it fails that check. The C++ code does the right thing for positive offsets. It will only fail if the unsigned offset they are looking for is > 2GB, which is also exceedingly unlikely. It also doesn't seem like any of the other code does the wrong thing. 1342534 11 saam 2017-08-25 14:53:26 -0700 (In reply to Keith Miller from comment #10) > (In reply to Geoffrey Garen from comment #9) > > I don't think this patch can be right. It deletes an ASSERT and a comment > > explaining a problem, but it doesn't fix the problem. > > > > Consider ThunkGenerators.cpp::stringCharLoad and similar functions: > > > > // load index > > jit.loadInt32Argument(0, SpecializedThunkJIT::regT1); // regT1 contains > > the index > > > > // Do an unsigned compare to simultaneously filter negative indices as > > well as indices that are too large > > jit.appendFailure(jit.branch32(MacroAssembler::AboveOrEqual, > > SpecializedThunkJIT::regT1, SpecializedThunkJIT::regT2)); > > > > ... > > I think this is fine. That thunk calls the slow path (the C++ code) if it > fails that check. The C++ code does the right thing for positive offsets. It > will only fail if the unsigned offset they are looking for is > 2GB, which > is also exceedingly unlikely. It also doesn't seem like any of the other > code does the wrong thing. What's "any of the other code" in this context? 319083 2017-08-25 09:33:09 -0700 2017-08-25 09:36:29 -0700 Patch bug-163955-20170825093308.patch text/plain 2938 keith_miller U3VidmVyc2lvbiBSZXZpc2lvbjogMjIxMDIxCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA3 NjM0ZTE5MDM5ZWMyYjI2ODRjNTU2MThkYjk1MDQxYzAwNjE5OWVjLi43MzM1M2FjY2MxMjllM2Iz ZmQxMjM5OWUyZTQyYmZlZTYzNTQ2M2QxIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyMCBAQAorMjAxNy0wOC0yNSAgS2VpdGggTWlsbGVyICA8a2VpdGhfbWlsbGVyQGFwcGxl LmNvbT4KKworICAgICAgICBFeHBsb3JlIGluY3JlYXNpbmcgbWF4IEpTU3RyaW5nOjptX2xlbmd0 aCB0byBVSU5UX01BWC4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTE2Mzk1NQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIFRoaXMgY2FuIGNhdXNlIHVzIHRvIHJlbGVhc2UgYXNzZXJ0IG9uIHNvbWUgY29kZSBw YXRocy4gSSBkb24ndAorICAgICAgICBzZWUgYSByZWFzb24gdG8gbWFpbnRhaW4gdGhpcyByZXN0 cmljdGlvbi4KKworICAgICAgICAqIHJ1bnRpbWUvSlNTdHJpbmcuaDoKKyAgICAgICAgKEpTQzo6 SlNTdHJpbmc6Omxlbmd0aCBjb25zdCk6CisgICAgICAgIChKU0M6OkpTU3RyaW5nOjpzZXRMZW5n dGgpOgorICAgICAgICAoSlNDOjpKU1N0cmluZzo6aXNWYWxpZExlbmd0aCk6IERlbGV0ZWQuCisg ICAgICAgICogcnVudGltZS9KU1N0cmluZ0J1aWxkZXIuaDoKKyAgICAgICAgKEpTQzo6anNNYWtl Tm9udHJpdmlhbFN0cmluZyk6CisKIDIwMTctMDgtMjIgIEtlaXRoIE1pbGxlciAgPGtlaXRoX21p bGxlckBhcHBsZS5jb20+CiAKICAgICAgICAgVW5yaXZpZXdlZCwgZml4IHdpbmRvd3MgYnVpbGQu Li4gZm9yIHJlYWx6LgpkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUv SlNTdHJpbmcuaCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTU3RyaW5nLmgKaW5k ZXggMzEwNTcwNTU5YTg4MGNlMzYyOTkwMTRmODRiYjBkYTM4ODlmNGJiOC4uYWI1NDU3MmI5NTJk N2Q1ZjMwODVlMjc2OGM2YTZkY2U2MjMwODY3OSAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3Jp cHRDb3JlL3J1bnRpbWUvSlNTdHJpbmcuaAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVu dGltZS9KU1N0cmluZy5oCkBAIC0xNjQsMTQgKzE2NCw2IEBAIHB1YmxpYzoKICAgICBjb25zdCBT dHJpbmcmIHRyeUdldFZhbHVlKCkgY29uc3Q7CiAgICAgY29uc3QgU3RyaW5nSW1wbCogdHJ5R2V0 VmFsdWVJbXBsKCkgY29uc3Q7CiAgICAgQUxXQVlTX0lOTElORSB1bnNpZ25lZCBsZW5ndGgoKSBj b25zdCB7IHJldHVybiBtX2xlbmd0aDsgfQotICAgIEFMV0FZU19JTkxJTkUgc3RhdGljIGJvb2wg aXNWYWxpZExlbmd0aChzaXplX3QgbGVuZ3RoKQotICAgIHsKLSAgICAgICAgLy8gV2hpbGUgbGVu Z3RoIGlzIG9mIHR5cGUgdW5zaWduZWQsIHRoZSBydW50aW1lIGFuZCBjb21waWxlcnMgYXJlIGFs bAotICAgICAgICAvLyBleHBlY3RpbmcgdGhhdCBtX2xlbmd0aCBpcyBhIHBvc2l0aXZlIHZhbHVl IDw9IElOVF9NQVguCi0gICAgICAgIC8vIEZJWE1FOiBMb29rIGludG8gbWFraW5nIHRoZSBtYXgg bGVuZ3RoIFVJTlRfTUFYIHRvIG1hdGNoIFN0cmluZ0ltcGwncyBtYXggbGVuZ3RoLgotICAgICAg ICAvLyBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTYzOTU1Ci0gICAg ICAgIHJldHVybiBsZW5ndGggPD0gc3RkOjpudW1lcmljX2xpbWl0czxpbnQzMl90Pjo6bWF4KCk7 Ci0gICAgfQogCiAgICAgSlNWYWx1ZSB0b1ByaW1pdGl2ZShFeGVjU3RhdGUqLCBQcmVmZXJyZWRQ cmltaXRpdmVUeXBlKSBjb25zdDsKICAgICBib29sIHRvQm9vbGVhbigpIGNvbnN0IHsgcmV0dXJu ICEhbGVuZ3RoKCk7IH0KQEAgLTIxOSw3ICsyMTEsNiBAQCBwcm90ZWN0ZWQ6CiAKICAgICBBTFdB WVNfSU5MSU5FIHZvaWQgc2V0TGVuZ3RoKHVuc2lnbmVkIGxlbmd0aCkKICAgICB7Ci0gICAgICAg IFJFTEVBU0VfQVNTRVJUKGlzVmFsaWRMZW5ndGgobGVuZ3RoKSk7CiAgICAgICAgIG1fbGVuZ3Ro ID0gbGVuZ3RoOwogICAgIH0KIApkaWZmIC0tZ2l0IGEvU291cmNlL0phdmFTY3JpcHRDb3JlL3J1 bnRpbWUvSlNTdHJpbmdCdWlsZGVyLmggYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9K U1N0cmluZ0J1aWxkZXIuaAppbmRleCA4OGFkNmRkZWYxZjQ0N2NjNTU4NzVlMDJkMzlhNTRkYzE4 MDgyODJjLi45YjQyOTRhYWRhZTFkZGNjNmMzMDBlMzE2NDJlOWFhMDA5MTViZmY5IDEwMDY0NAot LS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU1N0cmluZ0J1aWxkZXIuaAorKysg Yi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU1N0cmluZ0J1aWxkZXIuaApAQCAtMTMx LDcgKzEzMSw3IEBAIGlubGluZSBKU1ZhbHVlIGpzTWFrZU5vbnRyaXZpYWxTdHJpbmcoRXhlY1N0 YXRlKiBleGVjLCBjb25zdCBTdHJpbmdUeXBlJiBzdHJpbmcsCiAgICAgVk0mIHZtID0gZXhlYy0+ dm0oKTsKICAgICBhdXRvIHNjb3BlID0gREVDTEFSRV9USFJPV19TQ09QRSh2bSk7CiAgICAgU3Ry aW5nIHJlc3VsdCA9IHRyeU1ha2VTdHJpbmcoc3RyaW5nLCBzdHJpbmdzLi4uKTsKLSAgICBpZiAo VU5MSUtFTFkoIXJlc3VsdCB8fCAhSlNTdHJpbmc6OmlzVmFsaWRMZW5ndGgocmVzdWx0Lmxlbmd0 aCgpKSkpCisgICAgaWYgKFVOTElLRUxZKCFyZXN1bHQpKQogICAgICAgICByZXR1cm4gdGhyb3dP dXRPZk1lbW9yeUVycm9yKGV4ZWMsIHNjb3BlKTsKICAgICByZXR1cm4ganNOb250cml2aWFsU3Ry aW5nKGV4ZWMsIFdURk1vdmUocmVzdWx0KSk7CiB9Cg== 319085 2017-08-25 09:36:33 -0700 2017-08-25 10:27:50 -0700 Patch bug-163955-20170825093632.patch text/plain 2973 keith_miller U3VidmVyc2lvbiBSZXZpc2lvbjogMjIxMDIxCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA3 NjM0ZTE5MDM5ZWMyYjI2ODRjNTU2MThkYjk1MDQxYzAwNjE5OWVjLi43NjI5MzcxNzIzNmM2MzMx YzNmMzFiOWMyOWJhYTE3MzFkYzU2YjliIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyMSBAQAorMjAxNy0wOC0yNSAgS2VpdGggTWlsbGVyICA8a2VpdGhfbWlsbGVyQGFwcGxl LmNvbT4KKworICAgICAgICBFeHBsb3JlIGluY3JlYXNpbmcgbWF4IEpTU3RyaW5nOjptX2xlbmd0 aCB0byBVSU5UX01BWC4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTE2Mzk1NQorICAgICAgICA8cmRhcjovL3Byb2JsZW0vMzIwMDE0OTk+CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhpcyBjYW4gY2F1c2Ug dXMgdG8gcmVsZWFzZSBhc3NlcnQgb24gc29tZSBjb2RlIHBhdGhzLiBJIGRvbid0CisgICAgICAg IHNlZSBhIHJlYXNvbiB0byBtYWludGFpbiB0aGlzIHJlc3RyaWN0aW9uLgorCisgICAgICAgICog cnVudGltZS9KU1N0cmluZy5oOgorICAgICAgICAoSlNDOjpKU1N0cmluZzo6bGVuZ3RoIGNvbnN0 KToKKyAgICAgICAgKEpTQzo6SlNTdHJpbmc6OnNldExlbmd0aCk6CisgICAgICAgIChKU0M6OkpT U3RyaW5nOjppc1ZhbGlkTGVuZ3RoKTogRGVsZXRlZC4KKyAgICAgICAgKiBydW50aW1lL0pTU3Ry aW5nQnVpbGRlci5oOgorICAgICAgICAoSlNDOjpqc01ha2VOb250cml2aWFsU3RyaW5nKToKKwog MjAxNy0wOC0yMiAgS2VpdGggTWlsbGVyICA8a2VpdGhfbWlsbGVyQGFwcGxlLmNvbT4KIAogICAg ICAgICBVbnJpdmlld2VkLCBmaXggd2luZG93cyBidWlsZC4uLiBmb3IgcmVhbHouCmRpZmYgLS1n aXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU1N0cmluZy5oIGIvU291cmNlL0ph dmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNTdHJpbmcuaAppbmRleCAzMTA1NzA1NTlhODgwY2UzNjI5 OTAxNGY4NGJiMGRhMzg4OWY0YmI4Li5hYjU0NTcyYjk1MmQ3ZDVmMzA4NWUyNzY4YzZhNmRjZTYy MzA4Njc5IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU1N0cmlu Zy5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTU3RyaW5nLmgKQEAgLTE2 NCwxNCArMTY0LDYgQEAgcHVibGljOgogICAgIGNvbnN0IFN0cmluZyYgdHJ5R2V0VmFsdWUoKSBj b25zdDsKICAgICBjb25zdCBTdHJpbmdJbXBsKiB0cnlHZXRWYWx1ZUltcGwoKSBjb25zdDsKICAg ICBBTFdBWVNfSU5MSU5FIHVuc2lnbmVkIGxlbmd0aCgpIGNvbnN0IHsgcmV0dXJuIG1fbGVuZ3Ro OyB9Ci0gICAgQUxXQVlTX0lOTElORSBzdGF0aWMgYm9vbCBpc1ZhbGlkTGVuZ3RoKHNpemVfdCBs ZW5ndGgpCi0gICAgewotICAgICAgICAvLyBXaGlsZSBsZW5ndGggaXMgb2YgdHlwZSB1bnNpZ25l ZCwgdGhlIHJ1bnRpbWUgYW5kIGNvbXBpbGVycyBhcmUgYWxsCi0gICAgICAgIC8vIGV4cGVjdGlu ZyB0aGF0IG1fbGVuZ3RoIGlzIGEgcG9zaXRpdmUgdmFsdWUgPD0gSU5UX01BWC4KLSAgICAgICAg Ly8gRklYTUU6IExvb2sgaW50byBtYWtpbmcgdGhlIG1heCBsZW5ndGggVUlOVF9NQVggdG8gbWF0 Y2ggU3RyaW5nSW1wbCdzIG1heCBsZW5ndGguCi0gICAgICAgIC8vIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjM5NTUKLSAgICAgICAgcmV0dXJuIGxlbmd0aCA8PSBz dGQ6Om51bWVyaWNfbGltaXRzPGludDMyX3Q+OjptYXgoKTsKLSAgICB9CiAKICAgICBKU1ZhbHVl IHRvUHJpbWl0aXZlKEV4ZWNTdGF0ZSosIFByZWZlcnJlZFByaW1pdGl2ZVR5cGUpIGNvbnN0Owog ICAgIGJvb2wgdG9Cb29sZWFuKCkgY29uc3QgeyByZXR1cm4gISFsZW5ndGgoKTsgfQpAQCAtMjE5 LDcgKzIxMSw2IEBAIHByb3RlY3RlZDoKIAogICAgIEFMV0FZU19JTkxJTkUgdm9pZCBzZXRMZW5n dGgodW5zaWduZWQgbGVuZ3RoKQogICAgIHsKLSAgICAgICAgUkVMRUFTRV9BU1NFUlQoaXNWYWxp ZExlbmd0aChsZW5ndGgpKTsKICAgICAgICAgbV9sZW5ndGggPSBsZW5ndGg7CiAgICAgfQogCmRp ZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU1N0cmluZ0J1aWxkZXIu aCBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTU3RyaW5nQnVpbGRlci5oCmluZGV4 IDg4YWQ2ZGRlZjFmNDQ3Y2M1NTg3NWUwMmQzOWE1NGRjMTgwODI4MmMuLjliNDI5NGFhZGFlMWRk Y2M2YzMwMGUzMTY0MmU5YWEwMDkxNWJmZjkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9ydW50aW1lL0pTU3RyaW5nQnVpbGRlci5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29y ZS9ydW50aW1lL0pTU3RyaW5nQnVpbGRlci5oCkBAIC0xMzEsNyArMTMxLDcgQEAgaW5saW5lIEpT VmFsdWUganNNYWtlTm9udHJpdmlhbFN0cmluZyhFeGVjU3RhdGUqIGV4ZWMsIGNvbnN0IFN0cmlu Z1R5cGUmIHN0cmluZywKICAgICBWTSYgdm0gPSBleGVjLT52bSgpOwogICAgIGF1dG8gc2NvcGUg PSBERUNMQVJFX1RIUk9XX1NDT1BFKHZtKTsKICAgICBTdHJpbmcgcmVzdWx0ID0gdHJ5TWFrZVN0 cmluZyhzdHJpbmcsIHN0cmluZ3MuLi4pOwotICAgIGlmIChVTkxJS0VMWSghcmVzdWx0IHx8ICFK U1N0cmluZzo6aXNWYWxpZExlbmd0aChyZXN1bHQubGVuZ3RoKCkpKSkKKyAgICBpZiAoVU5MSUtF TFkoIXJlc3VsdCkpCiAgICAgICAgIHJldHVybiB0aHJvd091dE9mTWVtb3J5RXJyb3IoZXhlYywg c2NvcGUpOwogICAgIHJldHVybiBqc05vbnRyaXZpYWxTdHJpbmcoZXhlYywgV1RGTW92ZShyZXN1 bHQpKTsKIH0K