Title: [287024] trunk
Revision
287024
Author
[email protected]
Date
2021-12-14 08:29:11 -0800 (Tue, 14 Dec 2021)

Log Message

TextDecoder doesn't detect invalid UTF-8 sequences early enough
https://bugs.webkit.org/show_bug.cgi?id=233921

Patch by Andreu Botella <[email protected]> on 2021-12-14
Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Import WPT tests from
https://github.com/web-platform-tests/wpt/pull/31537.

* web-platform-tests/encoding/textdecoder-eof.any.js:
(test):
* web-platform-tests/encoding/textdecoder-streaming.any-expected.txt:
* web-platform-tests/encoding/textdecoder-streaming.any.js:
(string_appeared_here.forEach.):
(string_appeared_here.forEach.test):
(string_appeared_here.forEach):
* web-platform-tests/encoding/textdecoder-streaming.any.worker-expected.txt:

Source/WebCore/PAL:

In streaming mode, when TextCodecUTF8 found a lead byte for which a
valid sequence would span longer than the currently available bytes, it
used to defer any processing of that sequence until all such bytes were
available, even if errors could be detected earlier. Additionally, if
the stream was flushed at that point, it would emit a single replacement
character, regardless of whether the remaining bytes formed a valid
sequence, even if they had lead bytes, resulting in skipped characters.
Both issues are solved by always checking the validity of partial
sequences.

The approach used in this patch uses `decodeNonASCIISequence` to find
the length of the maximal subpart of a partial sequence, and if the
length is equal to the partial sequence size and we're not at EOF, we
don't emit the error. This is enough to handle the missing characters at
EOF, and when combined with changing the condition of the outer do-while
loops in the `decode` method from `flush && m_partialSequenceSize` to
only `m_partialSequenceSize`, it also fixes the streaming issue.

This patch is a port of
https://chromium-review.googlesource.com/c/chromium/src/+/3263938

Tests: imported/w3c/web-platform-tests/encoding/textdecoder-eof.any.html
       imported/w3c/web-platform-tests/encoding/textdecoder-stream.any.html

* pal/text/TextCodecUTF8.cpp:
(PAL::TextCodecUTF8::handlePartialSequence): Changed to always process
partial sequences.
(PAL::TextCodecUTF8::decode): Changed the loop condition of the outer
do-while loops to not depend on `flush`.

Modified Paths

Diff

Modified: trunk/LayoutTests/imported/w3c/ChangeLog (287023 => 287024)


--- trunk/LayoutTests/imported/w3c/ChangeLog	2021-12-14 16:20:09 UTC (rev 287023)
+++ trunk/LayoutTests/imported/w3c/ChangeLog	2021-12-14 16:29:11 UTC (rev 287024)
@@ -1,3 +1,22 @@
+2021-12-14  Andreu Botella  <[email protected]>
+
+        TextDecoder doesn't detect invalid UTF-8 sequences early enough
+        https://bugs.webkit.org/show_bug.cgi?id=233921
+
+        Reviewed by Darin Adler.
+
+        Import WPT tests from
+        https://github.com/web-platform-tests/wpt/pull/31537.
+
+        * web-platform-tests/encoding/textdecoder-eof.any.js:
+        (test):
+        * web-platform-tests/encoding/textdecoder-streaming.any-expected.txt:
+        * web-platform-tests/encoding/textdecoder-streaming.any.js:
+        (string_appeared_here.forEach.):
+        (string_appeared_here.forEach.test):
+        (string_appeared_here.forEach):
+        * web-platform-tests/encoding/textdecoder-streaming.any.worker-expected.txt:
+
 2021-12-14  Rob Buis  <[email protected]>
 
         Incorrect aspect ratio size

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-eof.any.js (287023 => 287024)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-eof.any.js	2021-12-14 16:20:09 UTC (rev 287023)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-eof.any.js	2021-12-14 16:29:11 UTC (rev 287024)
@@ -1,7 +1,14 @@
 test(() => {
+  // Truncated sequences
   assert_equals(new TextDecoder().decode(new Uint8Array([0xF0])), "\uFFFD");
   assert_equals(new TextDecoder().decode(new Uint8Array([0xF0, 0x9F])), "\uFFFD");
   assert_equals(new TextDecoder().decode(new Uint8Array([0xF0, 0x9F, 0x92])), "\uFFFD");
+
+  // Errors near end-of-queue
+  assert_equals(new TextDecoder().decode(new Uint8Array([0xF0, 0x9F, 0x41])), "\uFFFDA");
+  assert_equals(new TextDecoder().decode(new Uint8Array([0xF0, 0x41, 0x42])), "\uFFFDAB");
+  assert_equals(new TextDecoder().decode(new Uint8Array([0xF0, 0x41, 0xF0])), "\uFFFDA\uFFFD");
+  assert_equals(new TextDecoder().decode(new Uint8Array([0xF0, 0x8F, 0x92])), "\uFFFD\uFFFD\uFFFD");
 }, "TextDecoder end-of-queue handling");
 
 test(() => {
@@ -15,4 +22,19 @@
 
   decoder.decode(new Uint8Array([0xF0, 0x9F]), { stream: true });
   assert_equals(decoder.decode(new Uint8Array([0x92])), "\uFFFD");
+
+  assert_equals(decoder.decode(new Uint8Array([0xF0, 0x9F]), { stream: true }), "");
+  assert_equals(decoder.decode(new Uint8Array([0x41]), { stream: true }), "\uFFFDA");
+  assert_equals(decoder.decode(), "");
+
+  assert_equals(decoder.decode(new Uint8Array([0xF0, 0x41, 0x42]), { stream: true }), "\uFFFDAB");
+  assert_equals(decoder.decode(), "");
+
+  assert_equals(decoder.decode(new Uint8Array([0xF0, 0x41, 0xF0]), { stream: true }), "\uFFFDA");
+  assert_equals(decoder.decode(), "\uFFFD");
+
+  assert_equals(decoder.decode(new Uint8Array([0xF0]), { stream: true }), "");
+  assert_equals(decoder.decode(new Uint8Array([0x8F]), { stream: true }), "\uFFFD\uFFFD");
+  assert_equals(decoder.decode(new Uint8Array([0x92]), { stream: true }), "\uFFFD");
+  assert_equals(decoder.decode(), "");
 }, "TextDecoder end-of-queue handling using stream: true");

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-streaming.any-expected.txt (287023 => 287024)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-streaming.any-expected.txt	2021-12-14 16:20:09 UTC (rev 287023)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-streaming.any-expected.txt	2021-12-14 16:29:11 UTC (rev 287024)
@@ -14,6 +14,7 @@
 PASS Streaming decode: utf-16be, 3 byte window (ArrayBuffer)
 PASS Streaming decode: utf-16be, 4 byte window (ArrayBuffer)
 PASS Streaming decode: utf-16be, 5 byte window (ArrayBuffer)
+PASS Streaming decode: UTF-8 chunk tests (ArrayBuffer)
 PASS Streaming decode: utf-8, 1 byte window (SharedArrayBuffer)
 PASS Streaming decode: utf-8, 2 byte window (SharedArrayBuffer)
 PASS Streaming decode: utf-8, 3 byte window (SharedArrayBuffer)
@@ -29,4 +30,5 @@
 PASS Streaming decode: utf-16be, 3 byte window (SharedArrayBuffer)
 PASS Streaming decode: utf-16be, 4 byte window (SharedArrayBuffer)
 PASS Streaming decode: utf-16be, 5 byte window (SharedArrayBuffer)
+PASS Streaming decode: UTF-8 chunk tests (SharedArrayBuffer)
 

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-streaming.any.js (287023 => 287024)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-streaming.any.js	2021-12-14 16:20:09 UTC (rev 287023)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-streaming.any.js	2021-12-14 16:29:11 UTC (rev 287024)
@@ -28,10 +28,11 @@
                 var decoder = new TextDecoder(encoding);
                 for (var i = 0; i < encoded.length; i += len) {
                     var sub = [];
-                    for (var j = i; j < encoded.length && j < i + len; ++j)
+                    for (var j = i; j < encoded.length && j < i + len; ++j) {
                         sub.push(encoded[j]);
-                        var uintArray = new Uint8Array(createBuffer(arrayBufferOrSharedArrayBuffer, sub.length));
-                        uintArray.set(sub);
+                    }
+                    var uintArray = new Uint8Array(createBuffer(arrayBufferOrSharedArrayBuffer, sub.length));
+                    uintArray.set(sub);
                     out += decoder.decode(uintArray, {stream: true});
                 }
                 out += decoder.decode();
@@ -39,4 +40,50 @@
             }, 'Streaming decode: ' + encoding + ', ' + len + ' byte window (' + arrayBufferOrSharedArrayBuffer + ')');
         }
     });
+
+    test(() => {
+        function bytes(byteArray) {
+            const view = new Uint8Array(createBuffer(arrayBufferOrSharedArrayBuffer, byteArray.length));
+            view.set(byteArray);
+            return view;
+        }
+
+        const decoder = new TextDecoder();
+
+        assert_equals(decoder.decode(bytes([0xC1]), {stream: true}), "\uFFFD");
+        assert_equals(decoder.decode(), "");
+
+        assert_equals(decoder.decode(bytes([0xF5]), {stream: true}), "\uFFFD");
+        assert_equals(decoder.decode(), "");
+
+        assert_equals(decoder.decode(bytes([0xE0, 0x41]), {stream: true}), "\uFFFDA");
+        assert_equals(decoder.decode(bytes([0x42])), "B");
+
+        assert_equals(decoder.decode(bytes([0xE0, 0x80]), {stream: true}), "\uFFFD\uFFFD");
+        assert_equals(decoder.decode(bytes([0x80])), "\uFFFD");
+
+        assert_equals(decoder.decode(bytes([0xED, 0xA0]), {stream: true}), "\uFFFD\uFFFD");
+        assert_equals(decoder.decode(bytes([0x80])), "\uFFFD");
+
+        assert_equals(decoder.decode(bytes([0xF0, 0x41]), {stream: true}), "\uFFFDA");
+        assert_equals(decoder.decode(bytes([0x42]), {stream: true}), "B");
+        assert_equals(decoder.decode(bytes([0x43])), "C");
+
+        assert_equals(decoder.decode(bytes([0xF0, 0x80]), {stream: true}), "\uFFFD\uFFFD");
+        assert_equals(decoder.decode(bytes([0x80]), {stream: true}), "\uFFFD");
+        assert_equals(decoder.decode(bytes([0x80])), "\uFFFD");
+
+        assert_equals(decoder.decode(bytes([0xF4, 0xA0]), {stream: true}), "\uFFFD\uFFFD");
+        assert_equals(decoder.decode(bytes([0x80]), {stream: true}), "\uFFFD");
+        assert_equals(decoder.decode(bytes([0x80])), "\uFFFD");
+
+        assert_equals(decoder.decode(bytes([0xF0, 0x90, 0x41]), {stream: true}), "\uFFFDA");
+        assert_equals(decoder.decode(bytes([0x42])), "B");
+
+        // 4-byte UTF-8 sequences always correspond to non-BMP characters. Here
+        // we make sure that, although the first 3 bytes are enough to emit the
+        // lead surrogate, it only gets emitted when the fourth byte is read.
+        assert_equals(decoder.decode(bytes([0xF0, 0x9F, 0x92]), {stream: true}), "");
+        assert_equals(decoder.decode(bytes([0xA9])), "\u{1F4A9}");
+    }, `Streaming decode: UTF-8 chunk tests (${arrayBufferOrSharedArrayBuffer})`);
 })

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-streaming.any.worker-expected.txt (287023 => 287024)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-streaming.any.worker-expected.txt	2021-12-14 16:20:09 UTC (rev 287023)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-streaming.any.worker-expected.txt	2021-12-14 16:29:11 UTC (rev 287024)
@@ -14,6 +14,7 @@
 PASS Streaming decode: utf-16be, 3 byte window (ArrayBuffer)
 PASS Streaming decode: utf-16be, 4 byte window (ArrayBuffer)
 PASS Streaming decode: utf-16be, 5 byte window (ArrayBuffer)
+PASS Streaming decode: UTF-8 chunk tests (ArrayBuffer)
 PASS Streaming decode: utf-8, 1 byte window (SharedArrayBuffer)
 PASS Streaming decode: utf-8, 2 byte window (SharedArrayBuffer)
 PASS Streaming decode: utf-8, 3 byte window (SharedArrayBuffer)
@@ -29,4 +30,5 @@
 PASS Streaming decode: utf-16be, 3 byte window (SharedArrayBuffer)
 PASS Streaming decode: utf-16be, 4 byte window (SharedArrayBuffer)
 PASS Streaming decode: utf-16be, 5 byte window (SharedArrayBuffer)
+PASS Streaming decode: UTF-8 chunk tests (SharedArrayBuffer)
 

Modified: trunk/Source/WebCore/PAL/ChangeLog (287023 => 287024)


--- trunk/Source/WebCore/PAL/ChangeLog	2021-12-14 16:20:09 UTC (rev 287023)
+++ trunk/Source/WebCore/PAL/ChangeLog	2021-12-14 16:29:11 UTC (rev 287024)
@@ -1,3 +1,40 @@
+2021-12-14  Andreu Botella  <[email protected]>
+
+        TextDecoder doesn't detect invalid UTF-8 sequences early enough
+        https://bugs.webkit.org/show_bug.cgi?id=233921
+
+        Reviewed by Darin Adler.
+
+        In streaming mode, when TextCodecUTF8 found a lead byte for which a
+        valid sequence would span longer than the currently available bytes, it
+        used to defer any processing of that sequence until all such bytes were
+        available, even if errors could be detected earlier. Additionally, if
+        the stream was flushed at that point, it would emit a single replacement
+        character, regardless of whether the remaining bytes formed a valid
+        sequence, even if they had lead bytes, resulting in skipped characters.
+        Both issues are solved by always checking the validity of partial
+        sequences.
+
+        The approach used in this patch uses `decodeNonASCIISequence` to find
+        the length of the maximal subpart of a partial sequence, and if the
+        length is equal to the partial sequence size and we're not at EOF, we
+        don't emit the error. This is enough to handle the missing characters at
+        EOF, and when combined with changing the condition of the outer do-while
+        loops in the `decode` method from `flush && m_partialSequenceSize` to
+        only `m_partialSequenceSize`, it also fixes the streaming issue.
+
+        This patch is a port of
+        https://chromium-review.googlesource.com/c/chromium/src/+/3263938
+
+        Tests: imported/w3c/web-platform-tests/encoding/textdecoder-eof.any.html
+               imported/w3c/web-platform-tests/encoding/textdecoder-stream.any.html
+
+        * pal/text/TextCodecUTF8.cpp:
+        (PAL::TextCodecUTF8::handlePartialSequence): Changed to always process
+        partial sequences.
+        (PAL::TextCodecUTF8::decode): Changed the loop condition of the outer
+        do-while loops to not depend on `flush`.
+
 2021-12-14  Ben Nham  <[email protected]>
 
         Add web push message decryption routines

Modified: trunk/Source/WebCore/PAL/pal/text/TextCodecUTF8.cpp (287023 => 287024)


--- trunk/Source/WebCore/PAL/pal/text/TextCodecUTF8.cpp	2021-12-14 16:20:09 UTC (rev 287023)
+++ trunk/Source/WebCore/PAL/pal/text/TextCodecUTF8.cpp	2021-12-14 16:29:11 UTC (rev 287024)
@@ -189,25 +189,34 @@
         if (!count)
             return true;
 
+        // Copy from `source` until we have `count` bytes.
+        if (count > m_partialSequenceSize && end > source) {
+            size_t additionalBytes = std::min<size_t>(count - m_partialSequenceSize, end - source);
+            memcpy(m_partialSequence + m_partialSequenceSize, source, additionalBytes);
+            source += additionalBytes;
+            m_partialSequenceSize += additionalBytes;
+        }
+
+        // If we still don't have `count` bytes, fill the rest with zeros (any
+        // other lead byte would do), so we can run `decodeNonASCIISequence` to
+        // tell if the chunk that we have is valid. These bytes are not part of
+        // the partial sequence, so don't increment `m_partialSequenceSize`.
+        bool partialSequenceIsTooShort = false;
         if (count > m_partialSequenceSize) {
-            if (count - m_partialSequenceSize > end - source) {
-                if (!flush) {
-                    // The new data is not enough to complete the sequence, so
-                    // add it to the existing partial sequence.
-                    memcpy(m_partialSequence + m_partialSequenceSize, source, end - source);
-                    m_partialSequenceSize += end - source;
-                    return false;
-                }
-                // An incomplete partial sequence at the end is an error, but it will create
-                // a 16 bit string due to the replacementCharacter. Let the 16 bit path handle
-                // the error.
-                return true;
-            }
-            memcpy(m_partialSequence + m_partialSequenceSize, source, count - m_partialSequenceSize);
-            source += count - m_partialSequenceSize;
-            m_partialSequenceSize = count;
+            partialSequenceIsTooShort = true;
+            memset(m_partialSequence + m_partialSequenceSize, 0, count - m_partialSequenceSize);
         }
+
         int character = decodeNonASCIISequence(m_partialSequence, count);
+        if (partialSequenceIsTooShort) {
+            ASSERT(character == nonCharacter);
+            ASSERT(count <= m_partialSequenceSize);
+            // If we're not at the end, and the partial sequence that we have is
+            // incomplete but otherwise valid, a non-character is not an error.
+            if (!flush && count == m_partialSequenceSize)
+                return false;
+        }
+
         if (!isLatin1(character))
             return true;
 
@@ -236,29 +245,35 @@
             consumePartialSequenceByte();
             continue;
         }
+
+        // Copy from `source` until we have `count` bytes.
+        if (count > m_partialSequenceSize && end > source) {
+            size_t additionalBytes = std::min<size_t>(count - m_partialSequenceSize, end - source);
+            memcpy(m_partialSequence + m_partialSequenceSize, source, additionalBytes);
+            source += additionalBytes;
+            m_partialSequenceSize += additionalBytes;
+        }
+
+        // If we still don't have `count` bytes, fill the rest with zeros (any
+        // other lead byte would do), so we can run `decodeNonASCIISequence` to
+        // tell if the chunk that we have is valid. These bytes are not part of
+        // the partial sequence, so don't increment `m_partialSequenceSize`.
+        bool partialSequenceIsTooShort = false;
         if (count > m_partialSequenceSize) {
-            if (count - m_partialSequenceSize > end - source) {
-                if (!flush) {
-                    // The new data is not enough to complete the sequence, so
-                    // add it to the existing partial sequence.
-                    memcpy(m_partialSequence + m_partialSequenceSize, source, end - source);
-                    m_partialSequenceSize += end - source;
-                    return;
-                }
-                // An incomplete partial sequence at the end is an error.
-                sawError = true;
-                if (stopOnError)
-                    return;
-                *destination++ = replacementCharacter;
-                m_partialSequenceSize = 0;
-                source = end;
-                continue;
-            }
-            memcpy(m_partialSequence + m_partialSequenceSize, source, count - m_partialSequenceSize);
-            source += count - m_partialSequenceSize;
-            m_partialSequenceSize = count;
+            partialSequenceIsTooShort = true;
+            memset(m_partialSequence + m_partialSequenceSize, 0, count - m_partialSequenceSize);
         }
+
         int character = decodeNonASCIISequence(m_partialSequence, count);
+        if (partialSequenceIsTooShort) {
+            ASSERT(character == nonCharacter);
+            ASSERT(count <= m_partialSequenceSize);
+            // If we're not at the end, and the partial sequence that we have is
+            // incomplete but otherwise valid, a non-character is not an error.
+            if (!flush && count == m_partialSequenceSize)
+                return;
+        }
+
         if (character == nonCharacter) {
             sawError = true;
             if (stopOnError)
@@ -353,7 +368,7 @@
             source += count;
             *destination++ = character;
         }
-    } while (flush && m_partialSequenceSize);
+    } while (m_partialSequenceSize);
 
     buffer.shrink(destination - buffer.characters());
     if (flush)
@@ -433,7 +448,7 @@
                 continue;
             destination16 = appendCharacter(destination16, character);
         }
-    } while (flush && m_partialSequenceSize);
+    } while (m_partialSequenceSize);
 
     buffer16.shrink(destination16 - buffer16.characters());
     if (flush)
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to