Bug 178207 – UTF-8 decoding produces one replacement character per byte; Encoding standard requires one replacement character per illegal sequence instead

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Marja Hölttä]

See https://bugs.chromium.org/p/chromium/issues/detail?id=773320 for the same bug in Blink.

In that bug, we saw a script where an invalid byte sequence occurs and is split between two chunks (of size 4096):

0b11100000 << lead << 0xe0
0b10100101 << cont << 0xa5
0b00111111 << ascii << 0x3f

The bug is that TextCodecUTF8::HandlePartialSequence calls TextCodecUTF8::HandleError which assumes that each error consumes one byte from the byte stream and produces an invalid char. However, that ignores the fact that we need to consume multiple bytes (i.e., the maximal subpart).

[Radar WebKit Bug Importer]

<rdar://problem/34979603>

[Alexey Proskuryakov]

Test case and patch available in the Chromium bug. Thanks!

[David Kilzer (:ddkilzer)]

Blink commit:
https://chromium.googlesource.com/chromium/src.git/+/79806338f4762c005b99808c8e97edf4d176b621

[Darin Adler]

I think I’ll fix this.

This might be better tested by writing a unit test for TextCodecUTF8 rather than trying to use a layout test. That would avoid the dependency on buffer sizes. I don’t know where the knowledge that a buffer is 4kB comes from and I would not want the test to stop being effective if we switched to, say, an 8kB buffer size.

I also think a better fix would be to get rid of the handleError function; it’s not really working well.

[Darin Adler]

To be thorough need to test both the case where the replacement character is the first 16-bit character and the case where there is a preceding 16-bit character.

[Darin Adler]

And also the "flush" and "no flush" variants.

[Darin Adler]

There is not a bug here that is specific to chunk boundaries. The code handles chunk boundaries correctly.

The issue is that the UTF-8 decoder produces too many replacement characters. The Encoding standard and "Best Practices for Using U+FFFD" from the Unicode specification require one replacement character for each illegal sequence, not one per byte in an illegal sequence.

We weren’t trying to implement that before, but now know that we should.

[Darin Adler]

Henri Sivonen wrote a paper about this topic <https://hsivonen.fi/broken-utf-8/>.

[Darin Adler]

I believe that the Chromium comments about this being a "very old bug" are incorrect. I think someone implemented new behavior in Chromium and didn’t handle chunk boundaries correctly. The very old code was correct for the old desired behavior, and became incorrect only recently when the behavior of the non-chunk-boundaries code was changed.

[Darin Adler]

This code is not in WTF; it’s in the platform layer. And a tiny bit of trivia: Henri’s paper incorrectly claims that it is in WTF.

[Darin Adler]

Created attachment 323840 [details]
Patch

[Darin Adler]

Created attachment 323841 [details]
Patch

[Darin Adler]

Created attachment 323843 [details]
Patch

[Darin Adler]

WTF has a different set of UTF-8 functions. Not sure how they handle replacement characters.

[Sam Weinig]

Comment on attachment 323843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323843&action=review

> Source/WebCore/platform/text/TextCodecUTF8.cpp:441
> +    Vector<char, 3000> bytes(length * 3);

How did you decide on 3000?

> Source/WebCore/platform/text/TextCodecUTF8.cpp:448
> +    return CString { bytes.data(), bytesWritten };

It's too bad we don't have a way to avoid this copy at least in some instances.  Perhaps at some point, we should rework CStringBuffer to allow for this.

[WebKit Commit Bot]

Comment on attachment 323843 [details]
Patch

Clearing flags on attachment: 323843

Committed r223329: <https://trac.webkit.org/changeset/223329>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 323843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323843&action=review

>> Source/WebCore/platform/text/TextCodecUTF8.cpp:441
>> +    Vector<char, 3000> bytes(length * 3);
> 
> How did you decide on 3000?

I just kind of thought "1000 * 3", but it’s so random.

I am getting a little annoyed at Vector<uint8_t> vs. Vector<char> vs. CString in different places. For the same kinds of purposes. We really need to standardize on Vector<uint8_t> vs. Vector<char>. I think that encode should probably return a Vector and not a CString.

>> Source/WebCore/platform/text/TextCodecUTF8.cpp:448
>> +    return CString { bytes.data(), bytesWritten };
> 
> It's too bad we don't have a way to avoid this copy at least in some instances.  Perhaps at some point, we should rework CStringBuffer to allow for this.

I think we should minimize the use of CString in the future. Not sure we need that class.