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

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted 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 97489: Patch
https://bugs.webkit.org/attachment.cgi?id=97489&action=review

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

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

four cases?

>> Source/WebCore/dom/XMLDocumentParser.h:61
>> +	    static PassRefPtr<XMLParserContext>
createStringParser(xmlSAXHandlerPtr handlers, void* userData);
> 
> The handlers name doesn't add anything (it's already described well by the
type), so I wouldn't add it.  But I appreciate you adding the void*.

Yup, "handlers" needn't be here.

>> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:514
>> +	ASSERT(chunk.length() <= INT_MAX);
> 
> What happens if we do pass a length over INT_MAX to
xmlCreateMemoryParserCtxt?

This assertion is not particularly useful. It won't help us uncover any
problems earlier, because we will never have chunks that long in practice.

Perhaps a comment explaining where the length check is made would be more
appropriate.