NEW 87980
Fix access beyond array end when parsing the empty string
https://bugs.webkit.org/show_bug.cgi?id=87980
Summary Fix access beyond array end when parsing the empty string
Andy Wingo
Reported 2012-05-31 08:05:27 PDT
The patch to be attached fixes an OOB array access in the lexer.
Attachments
Patch (2.65 KB, patch)
2012-05-31 08:10 PDT, Andy Wingo
no flags
Patch (5.35 KB, patch)
2012-06-04 07:20 PDT, Andy Wingo
no flags
Patch (6.09 KB, patch)
2012-06-04 09:01 PDT, Andy Wingo
ggaren: review-
Andy Wingo
Comment 1 2012-05-31 08:10:52 PDT
Andy Wingo
Comment 2 2012-05-31 10:09:54 PDT
Adding more potential reviewers :)
Geoffrey Garen
Comment 3 2012-05-31 10:28:17 PDT
Comment on attachment 145095 [details] Patch Our policy is to require a regression test for a code change that fixes an observable bug (http://www.webkit.org/coding/contributing.html). Can you make a regression test for this?
José Dapena Paz
Comment 4 2012-05-31 11:19:10 PDT
With this patch, I'm getting the assert ==31670== Invalid write of size 4 ==31670== at 0xB4E5578: JSC::Identifier const& JSC::IdentifierArena::makeIdentifier<unsigned short>(JSC::JSGlobalData*, unsigned short const*, unsigned long) (ParserArena.h:74) ==31670== by 0xB4E2FAA: JSC::Lexer<unsigned short>::makeIdentifier(unsigned short const*, unsigned long) (Lexer.h:236) ==31670== by 0xB4E217A: JSC::Lexer<unsigned short>::scanRegExp(JSC::Identifier const*&, JSC::Identifier const*&, unsigned short) (Lexer.cpp:1623) ==31670== by 0xB531EC0: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parsePrimaryExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1456) ==31670== by 0xB52F5B6: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parseMemberExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1519) ==31670== by 0xB52DE05: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parseUnaryExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1608) ==31670== by 0xB52C0DC: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parseBinaryExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1150) ==31670== by 0xB52A20E: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parseConditionalExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1112) ==31670== by 0xB527DCE: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parseAssignmentExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1054) ==31670== by 0xB524B34: JSC::SyntaxChecker::Expression JSC::Parser<JSC::Lexer<unsigned short> >::parseExpression<JSC::SyntaxChecker>(JSC::SyntaxChecker&) (Parser.cpp:1028) Seems similar changes are needed in other usages of makeIdentifier.
Andy Wingo
Comment 5 2012-05-31 12:43:51 PDT
I appear to have borked the patch :) It was a valgrind run from José that caught the bug. Geoffrey; are you asking me to make an exploit for this bug? It seems tricky: the extra byte would have to reside on a page that is not mapped into memory, and it would just yield a segfault. There is of course the possibility of poisoning the recent identifiers cache, but that doesn't seem to be exploitable, merely an efficiency loss. I'll submit an updated patch, but can you please clarify that you think a regression test is necessary in this case.
Geoffrey Garen
Comment 6 2012-05-31 13:45:57 PDT
> Geoffrey; are you asking me to make an exploit for this bug? I'm not sure -- maybe you can explain what the symptom is that you're trying to fix, and what conditions might cause it, and then we can talk about the best way to add a test. If this is a read past the end of a malloc buffer, one option is to create a test that crashes if and only if you are running under GuardMalloc.
Andy Wingo
Comment 7 2012-05-31 13:59:16 PDT
(In reply to comment #6) > > Geoffrey; are you asking me to make an exploit for this bug? > > I'm not sure -- maybe you can explain what the symptom is that you're trying to fix, and what conditions might cause it, and then we can talk about the best way to add a test. The story is that José was doing a valgrind run and noticed this read beyond the end of the array. It seems to be worth fixing, as it could cause a segfault at some point. Admittedly, we have not seen a crash due to this. Though, hummm: what if the OOB byte is greater than 127. In that case we would be writing beyond the bounds of m_recentIdentifiers. > If this is a read past the end of a malloc buffer, one option is to create a test that crashes if and only if you are running under GuardMalloc. I will look into this, thanks.
Darin Adler
Comment 8 2012-06-01 10:45:44 PDT
(In reply to comment #7) > (In reply to comment #6) > > If this is a read past the end of a malloc buffer, one option is to create a test that crashes if and only if you are running under GuardMalloc. > > I will look into this, thanks. Another good option is to add an assertion that we aren’t reading past the end of the buffer, then write a test that hits the assertion, then fix the bug.
Andy Wingo
Comment 9 2012-06-04 07:20:59 PDT
Andy Wingo
Comment 10 2012-06-04 07:23:37 PDT
The patch I pushed does include tests for the codepaths that hit this assertion. Without the fix they do indeed abort.
Andy Wingo
Comment 11 2012-06-04 08:07:21 PDT
Comment on attachment 145582 [details] Patch Clearing review flag, as I am seeing some test suite failures.
Andy Wingo
Comment 12 2012-06-04 09:01:16 PDT
Andy Wingo
Comment 13 2012-06-04 09:04:40 PDT
Attachment 145582 [details] had a bug. Sorry for the noise. I fixed it and ran all the layout tests on the one I just uploaded, attachment 145596 [details].
Geoffrey Garen
Comment 14 2012-06-04 11:49:22 PDT
Comment on attachment 145596 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145596&action=review > Source/JavaScriptCore/parser/Lexer.cpp:605 > + if (!length) > + return; This function and all its callees seem safe vs zero length. What happens if you remove this check? If something bad happens, why doesn't append16 need this check? > Source/JavaScriptCore/parser/ParserArena.h:74 > + ASSERT(length); I'm not a fan of the design pattern that has a callee ASSERT a condition and then requires all callers to ensure the condition with extra lines of code. This is only a good tradeoff if it's critical to performance in some way. But even then, I prefer to have a "fast path" function that ASSERTs the condition and a "slow path" convenience function that ensures the condition. That way, there's only one place in the code that needs to get this right, instead of many. I'd suggest the following design change: (1) Remove all instances of "stringStart != currentCharacter() && ", "currentCharacter() != stringStart &&", "identifierStart != currentCharacter()", etc. (2) Add a test for zero length inside Lexer::makeIdentifier and Lexer::makeIdentifierLCharFromUChar, which returns the empty identifier. (3) Put an ASSERT(length) inside IdentifierArena::makeIdentifier and IdentifierArena::makeIdentifierLCharFromUChar. To test the *16 conditions, you need a separate .js file encoded as UTF-16.
Note You need to log in before you can comment on or make changes to this bug.