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**))
This one too: new RegExp("[\Ñ]{2,4}");
And this one: new RegExp("[\£]{3,4}", "gmy");
Reduction: new RegExp("[\u0097]{4,6}");
Created attachment 17748 [details] Test case (will ASSERT)
Does not crash WebKit nightly r28482 (a Release build), so the ASSERT() condition may need to be changed?
No, the assertion is definitely correct.
<rdar://problem/5632992>
These test cases are extended character classes followed by repeat sequences. Should be easy to track down.
The preflight is computing a length of 16, the compiled regular expression has a length of 17.
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.
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.
Created attachment 17750 [details] more stressful test case
Created attachment 17752 [details] patch
Comment on attachment 17752 [details] patch Looks good. r=me
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.
(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.
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.
(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.
Committed revision 28491.