RESOLVED FIXED 16127
Reproducible crash inside PCRE under guard malloc
https://bugs.webkit.org/show_bug.cgi?id=16127
Summary Reproducible crash inside PCRE under guard malloc
Mark Rowe (bdash)
Reported 2007-11-24 22:24:00 PST
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
Attachments
Reduced test case (11 bytes, text/plain)
2007-11-24 22:24 PST, Mark Rowe (bdash)
no flags
Crash log (7.59 KB, text/plain)
2007-11-24 22:25 PST, Mark Rowe (bdash)
no flags
Check against patternEnd to make sure we don't walk of the end of the string (8.63 KB, patch)
2007-11-30 05:39 PST, Eric Seidel (no email)
no flags
Check against patternEnd to make sure we don't walk off the end of the string (8.85 KB, patch)
2007-11-30 05:55 PST, Eric Seidel (no email)
darin: review+
Mark Rowe (bdash)
Comment 1 2007-11-24 22:24:40 PST
Created attachment 17499 [details] Reduced test case
Mark Rowe (bdash)
Comment 2 2007-11-24 22:25:23 PST
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)
Comment 3 2007-11-24 22:26:28 PST
Alexey Proskuryakov
Comment 4 2007-11-25 00:03:00 PST
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)
Comment 5 2007-11-30 05:18:00 PST
bug 15839 no longer crashes on TOT, however this still does. Reopening.
Eric Seidel (no email)
Comment 6 2007-11-30 05:39:01 PST
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)
Comment 7 2007-11-30 05:55:42 PST
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 8 2007-11-30 09:30:02 PST
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)
Comment 9 2007-11-30 15:44:19 PST
Note You need to log in before you can comment on or make changes to this bug.