[webkit-reviews] review denied: [Bug 62070] XML memory parser handles NULL bytes wrong : [Attachment 95989] 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 62070: XML memory parser handles NULL bytes wrong
https://bugs.webkit.org/show_bug.cgi?id=62070

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

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

Please see bug 61053, which is related or even a duplicate. It it covers what
is being fixed here, please dupe to the older bug.

If this fixes test cases from that bug, please consider adding them to the
patch.

> LayoutTests/ChangeLog:8
> +	   Added test cases for handling NULL bytes as inserted through the
outerHTML property.

Do these actually pass in Firefox? It's always good to have tests that can be
compared to other implementations.

> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:504
> +PassRefPtr<XMLParserContext>
XMLParserContext::createMemoryParser(xmlSAXHandlerPtr handlers, void* userData,
const char* chunk, int len)

Please don't abbreviate. Maybe "chunkLength" or "chunkSize" would be good
names?

> Source/WebCore/dom/XMLDocumentParserLibxml2.cpp:1457
> +	   ASSERT(m_sawError || !chunkAsUtf8.data()[bytesProcessed]);

This will be an out of bounds read if bytesProcessed is -1.

Also, I don't quite understand the logic. If a null byte causes a failure, why
doesn't m_sawError get set? I don't remember this code well enough to know why
a JS exception will be raised without setting m_sawError. Could you please
explain where the exception flies from?