[webkit-reviews] review granted: [Bug 54862] REGRESSION (new UTF-8 decoder): Reproducible crash on alltommac.se : [Attachment 83508] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 54862: REGRESSION (new UTF-8 decoder): Reproducible crash on alltommac.se
https://bugs.webkit.org/show_bug.cgi?id=54862

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

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

Without test coverage, I had to take most logic changes on trust. It would be
good to have:
-  fuzzer-like tests (an XMLHttpRequest for a .mov or .aiff file in media
resources would be nice, like the one in bug 54519);
- behavior tests that can run on other browsers for comparison (handling of
trailing incomplete sequence comes to mind, as well as decoding the URL from
this bug);
- unit tests for things that cannot be tested otherwise.

> Source/WebCore/platform/text/TextCodecUTF8.cpp:156
> +    memmove(m_partialSequence, m_partialSequence + 1,
m_partialSequenceSize);

Is memmove normally fast enough to efficiently move a byte or two? Sounds like
an job for arithmetic shift to me.

> Source/WebCore/platform/text/TextCodecUTF8.cpp:171
> +void TextCodecUTF8::handlePartialSequence(UChar*& destination, const
uint8_t*& source, const uint8_t* end, bool flush, bool stopOnError, bool&
sawError)
> +{
> +    do {

Can you ASSERT(m_partialSequenceSize) here?

> Source/WebCore/platform/text/TextCodecUTF8.cpp:230
> +	       // Explicitly copy destination and source to avoid taking a
pointer, which
> +	       // harm code generation.

Did you mean "which can harm"?

> Source/WebCore/platform/text/TextCodecUTF8.h:50
> +    uint8_t m_partialSequence[U8_MAX_LENGTH];

So, it's not necessarily partial any more - should the variable be renamed?