[webkit-reviews] review denied: [Bug 70107] Custom written CSS lexer : [Attachment 118748] core patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 70107: Custom written CSS lexer
https://bugs.webkit.org/show_bug.cgi?id=70107

Attachment 118748: core patch
https://bugs.webkit.org/attachment.cgi?id=118748&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=118748&action=review


There seems to be a lot of code in this that assumes null-termination of all
strings passed to the lexer -- is this guaranteed?  For instance if i do var
foo = "some big string"; someElement.style=foo.substring(2, 8);  (not sure if
this exact statement is valid), we may return a StringImpl that is referencing
the middle of another string so null termination won't be guaranteed.

At the very least i'd like assertions that m_data never exceeds the buffer
length.

> Source/WebCore/css/CSSParser.cpp:7388
> +    return chr | 0x20;

Can you add an assertion that chr is in fact an ascii letter?

> Source/WebCore/css/CSSParser.cpp:7394
> +	   || (data[0] == '\\' && isCSSEscape(data[1]));

How do we guarantee data is at least 2 characters long?

> Source/WebCore/css/CSSParser.cpp:7430
> +	   } while (isASCIIHexDigit(*data) && --length);

I'd like assertions that data doesn't extend beyond the end of the buffer.

> Source/WebCore/css/CSSParser.cpp:7533
> +	   if (UNLIKELY(*m_data == quote)) {
> +	       // String parsing is done.
> +	       ++m_data;
> +	       return;
> +	   }

What happens if we have an unterminated string? afaict we just walk off the end
of the buffer.