Bug 16321 – new RegExp("[\u0097]{4,6}", "gmy") crashes in DEBUG builds

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Eric Seidel (no email)]

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**))

[Eric Seidel (no email)]

This one too:
new RegExp("[\Ñ]{2,4}");

[Eric Seidel (no email)]

And this one: new RegExp("[\£]{3,4}", "gmy");

[Eric Seidel (no email)]

Reduction: new RegExp("[\u0097]{4,6}");

[David Kilzer (:ddkilzer)]

Created attachment 17748 [details]
Test case (will ASSERT)

[David Kilzer (:ddkilzer)]

Does not crash WebKit nightly r28482 (a Release build), so the ASSERT() condition may need to be changed?

[Darin Adler]

No, the assertion is definitely correct.

[David Kilzer (:ddkilzer)]

<rdar://problem/5632992>

[Darin Adler]

These test cases are extended character classes followed by repeat sequences. Should be easy to track down.

[Darin Adler]

The preflight is computing a length of 16, the compiled regular expression has a length of 17.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

Created attachment 17750 [details]
more stressful test case

[Darin Adler]

Created attachment 17752 [details]
patch

[Eric Seidel (no email)]

Comment on attachment 17752 [details]
patch

Looks good.  r=me

[Eric Seidel (no email)]

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.

[Darin Adler]

(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.

[Eric Seidel (no email)]

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.

[Darin Adler]

(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.

[Darin Adler]

Committed revision 28491.