[webkit-reviews] review denied: [Bug 61053] Using null bytes when setting innerHTML in XTHML results in assertion and a crash due to null-pointer dereference : [Attachment 96828] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Jeffrey Pfau
<jeffrey at endrift.com>'s request for review:
Bug 61053: Using null bytes when setting innerHTML in XTHML results in
assertion and a crash due to null-pointer dereference
https://bugs.webkit.org/show_bug.cgi?id=61053

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=96828&action=review

Looks good to me. Please address Eric's comments too.

> LayoutTests/ChangeLog:8
> +	   Added test cases covering four cases of using innerHTML and
outerHTML with null bytes in XHTML

I don't really like that the tests check assigning strings that are not valid
fragments even if nulls are ignored, so checking these is confusing at best. I
would have tested strings such as "<p>Test fails</p>\x00" or "\x00<p>Test
fails</p>". 

Please change the tests to fail if the null bytes are manually removed, so that
they actually check what happens with these. Please double-check Firefox
compatibility once you make this change.

> LayoutTests/fast/parser/xhtml-innerhtml-null-byte-first.xhtml:14
> +	   // Base64 says: \0

Is there a reason to use Base64, not \x00 in a string literal? The latter would
be more readable, and wouldn't require comments.

> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:1447
> +    if (chunkAsUtf8.length() > 0x7FFFFFFFL) {

Why not INT_MAX? Or you could even check for something like
"chunkAsUtf8.length() == static_cast<int>(chunkAsUtf8.length())", although the
latter is not a common WebKit style.