[webkit-reviews] review denied: [Bug 87980] Fix access beyond array end when parsing the empty string : [Attachment 145596] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Andy Wingo <wingo at igalia.com>'s
request for review:
Bug 87980: Fix access beyond array end when parsing the empty string
https://bugs.webkit.org/show_bug.cgi?id=87980

Attachment 145596: Patch
https://bugs.webkit.org/attachment.cgi?id=145596&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
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.