Skip to content

[v20.5.0] Buffer.allocUnsafe and TextDecoder.decode doesn't play well. #48995

Closed
Closed
[v20.5.0] Buffer.allocUnsafe and TextDecoder.decode doesn't play well.#48995
Labels
bufferIssues and PRs related to the buffer subsystem.confirmed-bugIssues and PRs for confirmed bugs.encodingIssues and PRs related to the TextEncoder and TextDecoder APIs.

Description

@JoakimCh
JoakimCh
opened on Aug 2, 2023

Version

v20.5.0

Platform

Linux dell 6.4.0-1-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.4.4-1 (2023-07-23) x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

Here is some code to reproduce and explore the bug.
Both Buffer.from([0x80]) and Buffer.allocUnsafe(1)[0] = 0x80 will cause unstable behaviour in TextDecoder.decode

let error1Count = 0
let error2Count = 0
for (let index = 0; index < 20000; index++) {
  try {
    const buffer = Buffer.from([0x80])   // this is unstable
    // const buffer = Buffer.allocUnsafe(1) // this is unstable
    // const buffer = Buffer.alloc(1)       // this is stable
    buffer[0] = 0x80
    const data = new TextDecoder('utf-8', {fatal: true}).decode(buffer)
    error1Count ++ // then data is the '\uFFFD' replacement character
  } catch {}
  try { // this is stable
    const data = new TextDecoder('utf-8', {fatal: true}).decode(new Uint8Array([0x80]))
    error2Count ++
  } catch {} 
}
console.log(error1Count, error2Count)

How often does it reproduce? Is there a required condition?

Sometimes I have to run the code snippet a few times before the bug is seen.

What is the expected behavior? Why is that the expected behavior?

Since I'm running the TextDecoder with {fatal: true} I expect the invalid input to ALWAYS throw an error.

What do you see instead?

A few times (pretty much random) instead of throwing an error it outputs the \uFFFD replacement character. This should not be possible when {fatal: true} is used.

Additional information

I have only tested on Linux Debian with Node v20.5.0.

Activity

  1. atlowChemi
    added
    bufferIssues and PRs related to the buffer subsystem.
    encodingIssues and PRs related to the TextEncoder and TextDecoder APIs.
    on Aug 3, 2023
  2. lpinca
    added
    confirmed-bugIssues and PRs for confirmed bugs.
    on Aug 3, 2023

  3. lpinca commented on Aug 3, 2023

    @lpinca
    lpinca
    on Aug 3, 2023
    Member

    I can reproduce on macOS. Buffer.from() and Buffer.allocUnsafe() use the internal Buffer pool so it might be related to that.

  4. lpinca commented on Aug 3, 2023

    @lpinca
    lpinca
    on Aug 3, 2023
    Member

    The culprit seems to be this commit 2ef13b8.

    cc: @anonrig

  5. lpinca commented on Aug 3, 2023

    @lpinca
    lpinca
    on Aug 3, 2023
    Member

    FWIW this seems to fix the issue:

    diff --git a/src/encoding_binding.cc b/src/encoding_binding.cc
index b65a4f868e..747df2c40c 100644
--- a/src/encoding_binding.cc
+++ b/src/encoding_binding.cc
@@ -164,9 +164,9 @@ void BindingData::DecodeUTF8(const FunctionCallbackInfo<Value>& args) {
   size_t length = buffer.length();
 
   if (has_fatal) {
-    auto result = simdutf::validate_utf8_with_errors(data, length);
+    bool is_valid = simdutf::validate_utf8(data, length);
 
-    if (result.error) {
+    if (!is_valid) {
       return node::THROW_ERR_ENCODING_INVALID_ENCODED_DATA(
           env->isolate(), "The encoded data was not valid for encoding utf-8");
     }

    cc: @lemire

  6. lemire commented on Aug 3, 2023

    @lemire
    lemire
    on Aug 3, 2023
    Member

    No string that begins with the byte value 0x80 can be valid (does not matter what comes after).

    I have added tests for this upstream, specifically

      const char bad[1] = {(char)0x80};
  size_t length = 1;
  simdutf::result res = implementation.validate_utf8_with_errors(bad, length);
  ASSERT_TRUE(res.error);

    We run tests with sanitizers, so an out-of-bound access or an undefined behavior would be detected.

    simdutf/simdutf#265

    What is the expectation in this routine...?

    if (has_fatal) {
     auto result = simdutf::validate_utf8_with_errors(data, length);

     if (result.error) {
       return node::THROW_ERR_ENCODING_INVALID_ENCODED_DATA(
           env->isolate(), "The encoded data was not valid for encoding utf-8");
     }
   }

    Is the expectation that the data is incorrect? If so, I would encourage validate_utf8_with_errors as it is likely more performant.

  7. lpinca commented on Aug 4, 2023

    @lpinca
    lpinca
    on Aug 4, 2023
    Member

    I think the expectation is that data is correct. Anyway the reproduction script calls validate_utf8_with_errors 20000 times and it fails only a few times on my machine. Sometimes it does not fail. validate_utf8 never fails.

    luigi@imac:Desktop$ cat repro.js 
let error1Count = 0;
let error2Count = 0;
for (let index = 0; index < 20000; index++) {
  try {
    const buffer = Buffer.from([0x80]); // this is unstable
    // const buffer = Buffer.allocUnsafe(1) // this is unstable
    // const buffer = Buffer.alloc(1)       // this is stable
    buffer[0] = 0x80;
    const data = new TextDecoder('utf-8', { fatal: true }).decode(buffer);
    error1Count++; // then data is the '\uFFFD' replacement character
  } catch {}
  try {
    // this is stable
    const data = new TextDecoder('utf-8', { fatal: true }).decode(
      new Uint8Array([0x80])
    );
    error2Count++;
  } catch {}
}
console.log(error1Count, error2Count);
luigi@imac:Desktop$ node repro.js 
5 0
luigi@imac:Desktop$ node repro.js 
0 0
luigi@imac:Desktop$ node repro.js 
4 0
luigi@imac:Desktop$ node repro.js 
108 0
luigi@imac:Desktop$ node repro.js 
73 0
luigi@imac:Desktop$ node repro.js 
2 0

  8. lemire commented on Aug 4, 2023

    @lemire
    lemire
    on Aug 4, 2023
    Member

    I am going to be adding tests upstream today. Give me 24 hours to research this, to see whether there is an issue.

  9. lpinca commented on Aug 4, 2023

    @lpinca
    lpinca
    on Aug 4, 2023
    Member

    Sure, no hurries.

  10. lemire commented on Aug 4, 2023

    @lemire
    lemire
    on Aug 4, 2023
    Member

    I am going to be issue a patch release.

  11. lemire
    added a commit that references this issue on Aug 4, 2023

    Fix for nodejs/node#48995

    9235d29

  12. lemire commented on Aug 4, 2023

    @lemire
    lemire
    on Aug 4, 2023
    Member

    The issue is upstream in simdutf. I have a patch release for it https://github.com/simdutf/simdutf/releases/tag/v3.2.15

    I recommend upgrading to v3.2.15. It is nearly identical to v3.2.14 except for this one issue.

    cc @anonrig

  13. lpinca
    closed this as completedin c9b1d6con Aug 16, 2023

  14. lemire commented on Aug 16, 2023

    @lemire
    lemire
    on Aug 16, 2023
    Member

    Thanks @lpinca and @anonrig for updating.

    Thanks @JoakimCh for reporting the issue.

  15. UlisesGascon
    added a commit that references this issue on Aug 17, 2023

    deps: update simdutf to 3.2.17

    f71e383
  16. targos
    added a commit that references this issue on Oct 28, 2023

    deps: update simdutf to 3.2.17

    11ecd06
  17. panva
    added a commit that references this issue on Feb 18, 2024

    deps: update simdutf to 3.2.17

    c9b1d6c
  18. sercher
    added 2 commits that reference this issue on Apr 25, 2024

    deps: update simdutf to 3.2.17

    390fff9

    deps: update simdutf to 3.2.17

    18373d1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bufferIssues and PRs related to the buffer subsystem.confirmed-bugIssues and PRs for confirmed bugs.encodingIssues and PRs related to the TextEncoder and TextDecoder APIs.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions