16321 2007-12-06 00:37:37 -0800 new RegExp("[\u0097]{4,6}", "gmy") crashes in DEBUG builds 2007-12-06 12:26:10 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Mac OS X 10.4 RESOLVED FIXED HasReduction, InRadar, Regression P1 Normal --- 1 eric darin darin oldest_to_newest 63430 0 eric 2007-12-06 00:37:37 -0800 new RegExp("[\u0097]{4,6}", "gmy") crashes in DEBUG builds (probably release builds too) ASSERTION FAILED: code - codestart <= length (/Stuff/Projects/WebKit/JavaScriptCore/pcre/pcre_compile.cpp:2804 JSRegExp* jsRegExpCompile(const UChar*, int, JSRegExpIgnoreCaseOption, JSRegExpMultilineOption, unsigned int*, const char**)) 63432 1 eric 2007-12-06 00:43:10 -0800 This one too: new RegExp("[\Ñ]{2,4}"); 63433 2 eric 2007-12-06 00:48:26 -0800 And this one: new RegExp("[\£]{3,4}", "gmy"); 63436 3 eric 2007-12-06 00:50:58 -0800 Reduction: new RegExp("[\u0097]{4,6}"); 63458 4 17748 ddkilzer 2007-12-06 07:39:16 -0800 Created attachment 17748 Test case (will ASSERT) 63459 5 ddkilzer 2007-12-06 07:39:56 -0800 Does not crash WebKit nightly r28482 (a Release build), so the ASSERT() condition may need to be changed? 63470 6 darin 2007-12-06 08:09:01 -0800 No, the assertion is definitely correct. 63475 7 ddkilzer 2007-12-06 08:25:01 -0800 <rdar://problem/5632992> 63482 8 darin 2007-12-06 08:40:42 -0800 These test cases are extended character classes followed by repeat sequences. Should be easy to track down. 63484 9 darin 2007-12-06 08:53:12 -0800 The preflight is computing a length of 16, the compiled regular expression has a length of 17. 63491 10 darin 2007-12-06 09:34:21 -0800 If we want to see if this crashed in older builds, we should repeat the expression multiple times, and use a max number much higher than 6. 63492 11 darin 2007-12-06 09:35:58 -0800 Famous last words, but once again I don't see how this could be a regression. The logic has not changed since Leopard. Last time I was mistaken about this. 63494 12 17750 darin 2007-12-06 09:38:36 -0800 Created attachment 17750 more stressful test case 63496 13 17752 darin 2007-12-06 09:48:24 -0800 Created attachment 17752 patch 63497 14 17752 eric 2007-12-06 10:30:58 -0800 Comment on attachment 17752 patch Looks good. r=me 63498 15 eric 2007-12-06 10:32:59 -0800 I should note, the ASSERT is actually not quite in the right place. We ASSERT right after we've walked off the end of the buffer. We could be more aggressive and ASSERT right before we add the final KET, so as to catch cases like this were we're off by one (we'd still be asserting/returning late for cases which were off by more than 1). I actually changed my local tree to do that, and still hit some bad off by > 1 cases. I'll file those additional cases once darin's patch lands. 63499 16 darin 2007-12-06 10:36:30 -0800 (In reply to comment #15) > I should note, the ASSERT is actually not quite in the right place. We ASSERT > right after we've walked off the end of the buffer. We could be more > aggressive and ASSERT right before we add the final KET, so as to catch cases > like this were we're off by one (we'd still be asserting/returning late for > cases which were off by more than 1). I don't understand what you're saying. The current assertion fires if and only if we're running off the end of the buffer. If the assert doesn't fire, then we're not running off the end of the buffer. The final KET takes 3 bytes and the END opcode takes 1 byte -- I don't see how asserting before writing out those last 4 bytes would be more accurate. Maybe I'll understand when I see your patch. 63500 17 eric 2007-12-06 10:45:32 -0800 I mispoke. OP_END instead of OP_KET. I moved the error return before writing of OP_END and made it check to make sure there was enough space to write OP_END. Left the ASSERT checking if we were already over the code buffer before writing the OP_END. That allowed me to continue fuzz testing while ignoring the off-by-one case caused most often by this bug, and was not a permanent solution. What I was proposing above was moving the return, but still ASSERTing if there was not enough space for the OP_END. I agree, it's probably not worth the hassle to bother avoiding writing the OP_END. it's a programmer error if we ever are writing the OP_END off the end of the buffer, no sense in doing contortions in the runtime to workaround that. 63503 18 darin 2007-12-06 11:05:49 -0800 (In reply to comment #17) > I mispoke. OP_END instead of OP_KET. > > I moved the error return before writing of OP_END and made it check to make > sure there was enough space to write OP_END. Left the ASSERT checking if we > were already over the code buffer before writing the OP_END. That allowed me > to continue fuzz testing while ignoring the off-by-one case caused most often > by this bug, and was not a permanent solution. What I was proposing above was > moving the return, but still ASSERTing if there was not enough space for the > OP_END. > > I agree, it's probably not worth the hassle to bother avoiding writing the > OP_END. it's a programmer error if we ever are writing the OP_END off the end > of the buffer, no sense in doing contortions in the runtime to workaround that. We could allocate a bigger-than-needed buffer in debug versions. Then we could catch overruns and be able to continue testing, I suppose. 63514 19 darin 2007-12-06 12:26:10 -0800 Committed revision 28491. 17748 2007-12-06 07:39:16 -0800 2007-12-06 07:39:16 -0800 Test case (will ASSERT) bug-16321-test.html text/html 64 ddkilzer PHNjcmlwdD4KdmFyIHJlID0gbmV3IFJlZ0V4cCgiW1x1MDA5N117NCw2fSIsICJnbXkiKTsKPC9z Y3JpcHQ+Cg== 17750 2007-12-06 09:38:36 -0800 2007-12-06 09:38:36 -0800 more stressful test case bug-16321-test-2.html text/html 171 darin PHNjcmlwdD4KdmFyIHJlID0gbmV3IFJlZ0V4cCgiW1x1MDA5N117NCwxMDB9W1x1MDA5N117NCwx MDB9W1x1MDA5N117NCwxMDB9W1x1MDA5N117NCwxMDB9W1x1MDA5N117NCwxMDB9W1x1MDA5N117 NCwxMDB9W1x1MDA5N117NCwxMDB9W1x1MDA5N117NCwxMDB9IiwgImdteSIpOwo8L3NjcmlwdD4K 17752 2007-12-06 09:48:24 -0800 2007-12-06 10:30:58 -0800 patch RepeatOptCharClassPatch.txt text/plain 4636 darin SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDI4NDg1KQorKysgSmF2YVNjcmlwdENvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTggQEAKKzIwMDctMTItMDYgIERhcmluIEFk bGVyICA8ZGFyaW5AYXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIC0gZml4IGh0dHA6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE2MzIxCisgICAgICAgICAgbmV3IFJlZ0V4cCgiW1x1MDA5N117NCw2fSIsICJnbXkiKSBj cmFzaGVzIGluIERFQlVHIGJ1aWxkcworICAgICAgICAgIDxyZGFyOi8vcHJvYmxlbS81NjMyOTky PgorCisgICAgICAgIFRlc3Q6IGZhc3QvanMvcmVnZXhwLW92ZWZsb3cuaHRtbAorCisgICAgICAg ICogcGNyZS9wY3JlX2NvbXBpbGUuY3BwOgorICAgICAgICAoY2FsY3VsYXRlQ29tcGlsZWRQYXR0 ZXJuTGVuZ3RoQW5kRmxhZ3MpOiBJbiB0aGUgY2FzZSB3aGVyZSBhIHNpbmdsZSBjaGFyYWN0ZXIK KyAgICAgICAgY2hhcmFjdGVyIGNsYXNzIGlzIG9wdGltaXplZCB0byBub3QgdXNlIGEgY2hhcmFj dGVyIGNsYXNzIGF0IGFsbCwgdGhlIHByZWZsaWdodAorICAgICAgICBjb2RlIHdhcyBub3Qgc2V0 dGluZyB0aGUgbGFzdGl0ZW1sZW5ndGggdmFyaWFibGUuCisKIDIwMDctMTItMDUgIE1hcmsgUm93 ZSAgPG1yb3dlQGFwcGxlLmNvbT4KIAogICAgICAgICBRdCBXaW5kb3dzIGJ1aWxkIGZpeC4gIElu Y2x1ZGUgdGhlIHRpbWUtcmVsYXRlZCBoZWFkZXJzIGluIHRoZSBjb3JyZWN0IHBsYWNlLgpJbmRl eDogSmF2YVNjcmlwdENvcmUvcGNyZS9wY3JlX2NvbXBpbGUuY3BwCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEph dmFTY3JpcHRDb3JlL3BjcmUvcGNyZV9jb21waWxlLmNwcAkocmV2aXNpb24gMjg0ODUpCisrKyBK YXZhU2NyaXB0Q29yZS9wY3JlL3BjcmVfY29tcGlsZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTI1 MTUsMTMgKzI1MTUsMTYgQEAgc3RhdGljIGludCBjYWxjdWxhdGVDb21waWxlZFBhdHRlcm5MZW5n dAogICAgICAgICAgICAgICAgICAgICByZXR1cm4gLTE7CiAgICAgICAgICAgICAgICAgfQogICAg ICAgICAgICAgICAgIAotICAgICAgICAgICAgICAgIC8qIFdlIGNhbiBvcHRpbWl6ZSB3aGVuIHRo ZXJlIHdhcyBvbmx5IG9uZSBvcHRpbWl6YWJsZSBjaGFyYWN0ZXIuIFJlcGVhdHMKLSAgICAgICAg ICAgICAgICAgZm9yIHBvc2l0aXZlIGFuZCBuZWdhdGVkIHNpbmdsZSBvbmUtYnl0ZSBjaGFycyBh cmUgaGFuZGxlZCBieSB0aGUgZ2VuZXJhbAotICAgICAgICAgICAgICAgICBjb2RlLiBIZXJlLCB3 ZSBoYW5kbGUgcmVwZWF0cyBmb3IgdGhlIGNsYXNzIG9wY29kZXMuICovCi0gICAgICAgICAgICAg ICAgCisgICAgICAgICAgICAgICAgLyogV2UgY2FuIG9wdGltaXplIHdoZW4gdGhlcmUgd2FzIG9u bHkgb25lIG9wdGltaXphYmxlIGNoYXJhY3Rlci4KKyAgICAgICAgICAgICAgICAgTm90ZSB0aGF0 IHRoaXMgZG9lcyBub3QgZGV0ZWN0IHRoZSBjYXNlIG9mIGEgbmVnYXRlZCBzaW5nbGUgY2hhcmFj dGVyLgorICAgICAgICAgICAgICAgICBJbiB0aGF0IGNhc2Ugd2UgZG8gYW4gaW5jb3JyZWN0IGxl bmd0aCBjb21wdXRhdGlvbiwgYnV0IGl0J3Mgbm90IGEgc2VyaW91cworICAgICAgICAgICAgICAg ICBwcm9ibGVtIGJlY2F1c2UgdGhlIGNvbXB1dGVkIGxlbmd0aCBpcyB0b28gbGFyZ2UgcmF0aGVy IHRoYW4gdG9vIHNtYWxsLiAqLworCiAgICAgICAgICAgICAgICAgaWYgKGNsYXNzX29wdGNvdW50 ID09IDEpCi0gICAgICAgICAgICAgICAgICAgIGxlbmd0aCArPSAzOwotICAgICAgICAgICAgICAg IGVsc2UgeworICAgICAgICAgICAgICAgICAgICBnb3RvIE5PUk1BTF9DSEFSOworCisgICAgICAg ICAgICAgICAgLyogSGVyZSwgd2UgaGFuZGxlIHJlcGVhdHMgZm9yIHRoZSBjbGFzcyBvcGNvZGVz LiAqLworICAgICAgICAgICAgICAgIHsKICAgICAgICAgICAgICAgICAgICAgbGVuZ3RoICs9IDMz OwogICAgICAgICAgICAgICAgICAgICAKICAgICAgICAgICAgICAgICAgICAgLyogQSByZXBlYXQg bmVlZHMgZWl0aGVyIDEgb3IgNSBieXRlcy4gSWYgaXQgaXMgYSBwb3NzZXNzaXZlIHF1YW50aWZp ZXIsCkluZGV4OiBMYXlvdXRUZXN0cy9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gTGF5b3V0VGVz dHMvQ2hhbmdlTG9nCShyZXZpc2lvbiAyODQ4NSkKKysrIExheW91dFRlc3RzL0NoYW5nZUxvZwko d29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDA3LTEyLTA2ICBEYXJpbiBBZGxlciAg PGRhcmluQGFwcGxlLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICAtIGZpeCBodHRwOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x NjMyMQorICAgICAgICAgIG5ldyBSZWdFeHAoIltcdTAwOTddezQsNn0iLCAiZ215IikgY3Jhc2hl cyBpbiBERUJVRyBidWlsZHMKKyAgICAgICAgICA8cmRhcjovL3Byb2JsZW0vNTYzMjk5Mj4KKwor ICAgICAgICAqIGZhc3QvanMvcmVnZXhwLW92ZXJmbG93LWV4cGVjdGVkLnR4dDogVXBkYXRlZC4K KyAgICAgICAgKiBmYXN0L2pzL3Jlc291cmNlcy9yZWdleHAtb3ZlcmZsb3cuanM6IEFkZGVkIHRl c3QuCisKIDIwMDctMTItMDUgIFJvYiBCdWlzICA8YnVpc0BrZGUub3JnPgogCiAgICAgICAgIFJl dmlld2VkIGJ5IE1pdHouCkluZGV4OiBMYXlvdXRUZXN0cy9mYXN0L2pzL3JlZ2V4cC1vdmVyZmxv dy1leHBlY3RlZC50eHQKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gTGF5b3V0VGVzdHMvZmFzdC9qcy9yZWdleHAt b3ZlcmZsb3ctZXhwZWN0ZWQudHh0CShyZXZpc2lvbiAyODQ4NSkKKysrIExheW91dFRlc3RzL2Zh c3QvanMvcmVnZXhwLW92ZXJmbG93LWV4cGVjdGVkLnR4dAkod29ya2luZyBjb3B5KQpAQCAtOCw2 ICs4LDggQEAgUEFTUyAvXihcZHsxLDJ9KShbIC06XC9cLl17MX0pKFxkezEsMn0pKAogUEFTUyAv XihcZHs0fSkoWyAtOlwvXC5dezF9KShcZHsxLDJ9KShcMnsxfSkoXGR7MSwyfSlUKFxkezEsMn0p KFsgLTpcL1wuXXsxfSkoXGR7MSwyfSkoXDd7MX0pKFxkezEsMn0pWiQvLmV4ZWMoIjEyMzQ6NTo2 VDcvOC85WiIpLnRvU3RyaW5nKCkgaXMgIjEyMzQ6NTo2VDcvOC85WiwxMjM0LDosNSw6LDYsNywv LDgsLyw5IgogUEFTUyAvXFtbIidcc117MCwxfShbXHctXSopWyInXHNdezAsMX0oW1xXXXswLDF9 PSl7MCwyfVsiJ1xzXXswLDF9KFtcdy1dKilbIidcc117MCwxfVxdJC8uZXhlYygiW10iKS50b1N0 cmluZygpIGlzICJbXSwsLCIKIFBBU1MgLyh4KXswLDJ9Ly5leGVjKCIiKS50b1N0cmluZygpIGlz ICIsIgorUEFTUyAvW8KhXXs0LDZ9Ly5leGVjKCLCocKhwqHCoSIpLnRvU3RyaW5nKCkgaXMgIsKh wqHCocKhIgorUEFTUyAvW8KhXXsxLDEwMH1bwqFdezEsMTAwfVvCoV17MSwxMDB9W8KhXXsxLDEw MH1bwqFdezEsMTAwfVvCoV17MSwxMDB9W8KhXXsxLDEwMH1bwqFdezEsMTAwfS8uZXhlYygiwqHC ocKhwqHCocKhwqHCoSIpLnRvU3RyaW5nKCkgaXMgIsKhwqHCocKhwqHCocKhwqEiCiAKIFBBU1Mg c3VjY2Vzc2Z1bGx5UGFyc2VkIGlzIHRydWUKIApJbmRleDogTGF5b3V0VGVzdHMvZmFzdC9qcy9y ZXNvdXJjZXMvcmVnZXhwLW92ZXJmbG93LmpzCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2Zh c3QvanMvcmVzb3VyY2VzL3JlZ2V4cC1vdmVyZmxvdy5qcwkocmV2aXNpb24gMjg0ODUpCisrKyBM YXlvdXRUZXN0cy9mYXN0L2pzL3Jlc291cmNlcy9yZWdleHAtb3ZlcmZsb3cuanMJKHdvcmtpbmcg Y29weSkKQEAgLTE0LDYgKzE0LDkgQEAgc2hvdWxkQmUoJy9cXFtbIlwnXFxzXXswLDF9KFtcXHct XSopWyJcJwogCiBzaG91bGRCZSgnLyh4KXswLDJ9Ly5leGVjKCIiKS50b1N0cmluZygpJywgJyIs IicpOwogCitzaG91bGRCZSgnL1tcdTAwQTFdezQsNn0vLmV4ZWMoIlx1MDBBMVx1MDBBMVx1MDBB MVx1MDBBMSIpLnRvU3RyaW5nKCknLCAnIlx1MDBBMVx1MDBBMVx1MDBBMVx1MDBBMSInKTsKK3No b3VsZEJlKCcvW1x1MDBBMV17MSwxMDB9W1x1MDBBMV17MSwxMDB9W1x1MDBBMV17MSwxMDB9W1x1 MDBBMV17MSwxMDB9W1x1MDBBMV17MSwxMDB9W1x1MDBBMV17MSwxMDB9W1x1MDBBMV17MSwxMDB9 W1x1MDBBMV17MSwxMDB9Ly5leGVjKCJcdTAwQTFcdTAwQTFcdTAwQTFcdTAwQTFcdTAwQTFcdTAw QTFcdTAwQTFcdTAwQTEiKS50b1N0cmluZygpJywgJyJcdTAwQTFcdTAwQTFcdTAwQTFcdTAwQTFc dTAwQTFcdTAwQTFcdTAwQTFcdTAwQTEiJyk7CisKIGRlYnVnKCcnKTsKIAogdmFyIHN1Y2Nlc3Nm dWxseVBhcnNlZCA9IHRydWU7Cg==