Bug 16127 – Reproducible crash inside PCRE under guard malloc

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Rowe (bdash)]

Loading a JavaScript snippet containing the following regexp literal will cause a crash under guard malloc:
/\)[;\s]+/

This leads to indexing off the end of an array.  My reading of the PCRE 7.4 sources suggest that they also have this problem.  I recall seeing valgrind warnings in similar places when playing with it on Linux recently.

Steps to reproduce:
1) Save attachment as test.js.
2) DYLD_INSERT_LIBRARIES=/usr/lib/libgmalloc.dylib DYLD_FRAMEWORK_PATH=WebKitBuild/Debug ./WebitBuild/Debug/testkjs test.js

[Mark Rowe (bdash)]

Created attachment 17499 [details]
Reduced test case

[Mark Rowe (bdash)]

Created attachment 17500 [details]
Crash log

0   com.apple.JavaScriptCore      	0x0031a0f2 calculateCompiledPatternLengthAndFlags(unsigned short const*, int, JSRegExpIgnoreCaseOption, compile_data&, ErrorCode) + 1268 (pcre_compile.cpp:2550)
1   com.apple.JavaScriptCore      	0x0031ab56 jsRegExpCompile(unsigned short const*, int, JSRegExpIgnoreCaseOption, JSRegExpMultilineOption, unsigned int*, char const**) + 82 (pcre_compile.cpp:3013)
2   com.apple.JavaScriptCore      	0x002883df KJS::RegExp::RegExp(KJS::UString const&, KJS::UString const&) + 417 (regexp.cpp:70)

[Mark Rowe (bdash)]

<rdar://problem/5611792>

[Alexey Proskuryakov]

I don't think that original PCRE has this problem - it relies on strings being null-terminated.

*** This bug has been marked as a duplicate of 15839 ***

[Eric Seidel (no email)]

bug 15839 no longer crashes on TOT, however this still does.  Reopening.

[Eric Seidel (no email)]

Created attachment 17603 [details]
Check against patternEnd to make sure we don't walk of the end of the string

 JavaScriptCore/ChangeLog                           |   12 +++++
 JavaScriptCore/pcre/pcre_compile.cpp               |   47 ++++++++++----------
 LayoutTests/ChangeLog                              |   10 ++++
 .../fast/js/regexp-compile-crash-expected.txt      |    5 ++
 LayoutTests/fast/js/regexp-compile-crash.html      |   22 +++++++++
 5 files changed, 72 insertions(+), 24 deletions(-)

[Eric Seidel (no email)]

Created attachment 17604 [details]
Check against patternEnd to make sure we don't walk off the end of the string

 JavaScriptCore/ChangeLog                           |   12 +++++
 JavaScriptCore/pcre/pcre_compile.cpp               |   47 ++++++++++----------
 LayoutTests/ChangeLog                              |   10 ++++
 .../fast/js/regexp-compile-crash-expected.txt      |   11 +++++
 LayoutTests/fast/js/regexp-compile-crash.html      |   22 +++++++++
 5 files changed, 78 insertions(+), 24 deletions(-)

[Darin Adler]

Comment on attachment 17604 [details]
Check against patternEnd to make sure we don't walk off the end of the string

This is not the correct way to write tests in fast/js -- the actual test should be in a resources file, and the wrapper generated by the make-js-test-wrappers script. With that structure we can possibly run them standalone in the future.

+                while ((++ptr < patternEnd) && ((c = *ptr) != ']')) {
                     if (c > 127)
                         c = getCharAndAdvanceIfSurrogate(ptr);

This still has a "run off the end" problem, because getCharAndAdvanceIfSurrogate doesn't check the end. But that problem will go away if we merge my change to remove support for UTF-16 surrogate pairs, currently up for review.

I'm a little bit opposed to this code using parentheses around the patternEnd conditions, when all the other similar code in the same file omits parens in cases like these.

r=me, but better to improve the tests.

[Eric Seidel (no email)]

r28260