The patch to be attached fixes an OOB array access in the lexer.
Created attachment 145095 [details] Patch
Adding more potential reviewers :)
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?
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.
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; 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.
(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.
(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.
Created attachment 145582 [details] Patch
The patch I pushed does include tests for the codepaths that hit this assertion. Without the fix they do indeed abort.
Comment on attachment 145582 [details] Patch Clearing review flag, as I am seeing some test suite failures.
Created attachment 145596 [details] Patch
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].
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.