buffer: validate UTF8 on fast path #54526

ronag · 2024-08-23T14:34:31Z

Fast API handles invalid UTF differently than the slow API.

13:05:25 buffers/buffer-write-string-short.js n=1000000 len=1 encoding='ascii'                            ***    266.52 %       ±7.27%  ±9.75% ±12.84%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=1 encoding='latin1'                           ***    201.16 %       ±4.74%  ±6.33%  ±8.29%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=1 encoding='utf8'                             ***    242.07 %       ±6.29%  ±8.44% ±11.14%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=16 encoding='ascii'                           ***    264.81 %       ±5.88%  ±7.86% ±10.29%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=16 encoding='latin1'                          ***    197.28 %       ±6.96%  ±9.32% ±12.26%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=16 encoding='utf8'                            ***    349.02 %       ±8.80% ±11.84% ±15.68%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=32 encoding='ascii'                           ***    271.75 %       ±8.70% ±11.69% ±15.43%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=32 encoding='latin1'                          ***    214.58 %       ±5.85%  ±7.84% ±10.32%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=32 encoding='utf8'                            ***    351.17 %       ±7.64% ±10.28% ±13.60%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=8 encoding='ascii'                            ***    273.30 %       ±8.65% ±11.63% ±15.38%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=8 encoding='latin1'                           ***    191.62 %       ±5.62%  ±7.52%  ±9.88%
13:05:25 buffers/buffer-write-string-short.js n=1000000 len=8 encoding='utf8'                             ***    224.55 %       ±4.72%  ±6.33%  ±8.33%

Fixes: #54521
PR-URL: #54525

src/node_buffer.cc

lemire · 2024-08-23T14:59:10Z

test/parallel/test-buffer-write.js

+{
+ let i = 0;
+
+ while (i < 1_000_000) {


Having a non-deterministic test is unpleasant.

You can trigger fast api call using the method @targos implemented in his previous pull requests.

lemire

This looks to me. I wish we could use a deterministic test. Having to loop a million times to trigger and issue is both slow and error prone.

codecov · 2024-08-23T16:05:22Z

Codecov Report

Attention: Patch coverage is 88.88889% with 8 lines in your changes missing coverage. Please review.

Project coverage is 87.61%. Comparing base (01f751b) to head (8c1de9b).
Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_buffer.cc	88.88%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54526      +/-   ##
==========================================
+ Coverage   87.31%   87.61%   +0.29%     
==========================================
  Files         649      650       +1     
  Lines      182755   182892     +137     
  Branches    35051    35390     +339     
==========================================
+ Hits       159575   160236     +661     
+ Misses      16452    15925     -527     
- Partials     6728     6731       +3

Files with missing lines	Coverage Δ
src/node_buffer.cc	`71.41% <88.88%> (+1.46%)`	⬆️

... and 85 files with indirect coverage changes

jasnell

Some lint issues but otherwise LGTM

blexrob · 2024-08-24T07:38:59Z

With the current commits, the following test will fail:

let i = 0;
const testStr = "\xc2\x80"; // when stored in 1-byte chars, this is a valid UTF-8 encoding
const expected = Buffer.from(testStr).toString("hex");
for(; i < 1_000_000; i++) {
  const buf = Buffer.from(testStr);
  const ashex = buf.toString("hex");
  if (ashex !== expected) {
    console.log(`Decoding changed in iteration ${i} when changing to FastWriteStringUTF8, got ${ashex}, expected ${expected}`);
    break;
  }
}

if(i<1_000_000) {
  console.error("FAILED after %d iterations",i);
} else
  console.log("PASSED after %d iterations",i);

The source string is stored as UTF16 (compressed to 1 byte), but still needs to be converted to UTF-8. The fast path can only be implemented as memcpy when only characters from the range 0-127 are present.

ronag · 2024-08-24T14:23:39Z

The source string is stored as UTF16 (compressed to 1 byte), but still needs to be converted to UTF-8. The fast path can only be implemented as memcpy when only characters from the range 0-127 are present.

I thought FastOneByteString means that it's only one byte chars?

ronag · 2024-08-24T14:25:11Z

< when only characters from the range 0-127 are present

@lemira does simdutf have this? validate_ascii?

ronag · 2024-08-30T10:10:36Z

@ShenHongFei can you try again with your project. I think I might have found the issue.

ronag · 2024-08-30T10:11:16Z

For future reference:

Simple fuzzing:

const assert = require('node:assert')
const crypto = require('node:crypto')

for (let i = 0; i < 1e6; i++) {
  const len = Math.floor(Math.random() * 128)
  const buf = Buffer.allocUnsafe(len)
  crypto.getRandomValues(buf)

  for (let n = 0; n < 16; n++) {
    const start = Math.floor(Math.random() * len)
    const end = Math.min(len, start + Math.floor(Math.random() * (len - start)))
    const src = buf.subarray(start, end)
    const str = src.toString('utf8')

    let bufNew = Buffer.alloc(buf.length);
    let bufOld = Buffer.alloc(buf.length);

    bufNew = bufNew.subarray(0, bufNew.utf8Write(str, start))
    bufOld = bufOld.subarray(0, bufOld.utf8WriteLegacy(str, start)) // old implementation

    assert.deepStrictEqual(bufNew, bufOld)
  }
}

Adds a "safe" version of convert_latin1_to_utf8 with a maximum output length. Refs: nodejs/node#54526

ronag · 2024-08-30T10:28:48Z

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1626/

ShenHongFei · 2024-08-30T10:38:51Z

@ronag I tried it, and the problem has been fixed. 👍

nodejs-github-bot · 2024-08-30T13:34:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61707/

nodejs-github-bot · 2024-08-30T19:01:12Z

Commit Queue failed

- Loading data for nodejs/node/pull/54526
✔  Done loading data for nodejs/node/pull/54526
----------------------------------- PR info ------------------------------------
Title      buffer: validate UTF8 on fast path  (#54526)
Author     Robert Nagy <[email protected]> (@ronag)
Branch     ronag:fast-utf8-2 -> nodejs:main
Labels     buffer, c++, author ready, needs-ci
Commits    1
 - buffer: validate UTF8 on fast path
Committers 1
 - Robert Nagy <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54526
Fixes: https://github.com/nodejs/node/issues/54521
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54526
Fixes: https://github.com/nodejs/node/issues/54521
Reviewed-By: Daniel Lemire <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 23 Aug 2024 14:34:31 GMT
   ✔  Approvals: 6
   ✔  - Daniel Lemire (@lemire): https://github.com/nodejs/node/pull/54526#pullrequestreview-2257470621
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/54526#pullrequestreview-2273156047
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54526#pullrequestreview-2258976699
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/54526#pullrequestreview-2258958504
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/54526#pullrequestreview-2272343097
   ✔  - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/54526#pullrequestreview-2260113366
   ✘  Last GitHub CI failed
   ℹ  Last Full PR CI on 2024-08-30T13:34:03Z: https://ci.nodejs.org/job/node-test-pull-request/61707/
   ℹ  Last Benchmark CI on 2024-08-30T10:28:48Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1626/
- Querying data for job/node-test-pull-request/61707/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/10637813307

nodejs-github-bot · 2024-08-30T21:32:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61722/

nodejs-github-bot · 2024-08-31T08:37:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61734/

Adds a "safe" version of convert_latin1_to_utf8 with a maximum output length. Refs: nodejs/node#54526

nodejs-github-bot · 2024-08-31T12:45:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61738/

nodejs-github-bot · 2024-08-31T13:39:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61741/

Fast API handles invalid UTF differently than the slow API. Fixes: nodejs#54521 PR-URL: nodejs#54525

nodejs-github-bot · 2024-08-31T15:22:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/61749/

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 23, 2024

ronag commented Aug 23, 2024

View reviewed changes

src/node_buffer.cc Outdated Show resolved Hide resolved

lemire mentioned this pull request Aug 23, 2024

Buffer.from() output breaks after optimizing in nodejs 22.7 #54521

Closed

ronag force-pushed the fast-utf8-2 branch 2 times, most recently from a930ad4 to 0fda693 Compare August 23, 2024 14:45

ronag mentioned this pull request Aug 23, 2024

buffer: don't use Fast API for utf8 #54525

Closed

ronag marked this pull request as ready for review August 23, 2024 14:46

ronag force-pushed the fast-utf8-2 branch 2 times, most recently from 23f1b23 to 8be5c43 Compare August 23, 2024 14:50

lemire reviewed Aug 23, 2024

View reviewed changes

lemire approved these changes Aug 23, 2024

View reviewed changes

benjamingr approved these changes Aug 23, 2024

View reviewed changes

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2024

This comment was marked as outdated.

Sign in to view

ronag force-pushed the fast-utf8-2 branch from 8be5c43 to 79573f4 Compare August 23, 2024 18:43

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 23, 2024

ronag mentioned this pull request Aug 23, 2024

copy_utf8 simdutf/simdutf#550

Closed

dlarocque mentioned this pull request Aug 23, 2024

Compatibility error with Node 22.7 firebase/firebase-js-sdk#8455

Closed

jasnell approved these changes Aug 23, 2024

View reviewed changes

ronag force-pushed the fast-utf8-2 branch from 79573f4 to 8821991 Compare August 24, 2024 07:14

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2024

This comment was marked as outdated.

Sign in to view

ronag force-pushed the fast-utf8-2 branch from fd95e67 to 3ce11da Compare August 30, 2024 10:10

ronag added a commit to ronag/simdutf that referenced this pull request Aug 30, 2024

feat: convert_latin1_to_utf8_s

73e3ecc

Adds a "safe" version of convert_latin1_to_utf8 with a maximum output length. Refs: nodejs/node#54526

ronag added a commit to ronag/simdutf that referenced this pull request Aug 30, 2024

feat: convert_latin1_to_utf8_s

e2f02b8

Adds a "safe" version of convert_latin1_to_utf8 with a maximum output length. Refs: nodejs/node#54526

ronag added a commit to ronag/simdutf that referenced this pull request Aug 30, 2024

feat: convert_latin1_to_utf8_s

210a186

Adds a "safe" version of convert_latin1_to_utf8 with a maximum output length. Refs: nodejs/node#54526

anonrig approved these changes Aug 30, 2024

View reviewed changes

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2024

ronag changed the title ~~buffer: validate UTF8 on fast path~~ buffer: write string with fast api Aug 30, 2024

ronag changed the title ~~buffer: write string with fast api~~ buffer: validate UTF8 on fast path Aug 30, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2024

benjamingr approved these changes Aug 30, 2024

View reviewed changes

ronag added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 30, 2024

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 30, 2024

ronag added a commit to ronag/simdutf that referenced this pull request Aug 31, 2024

feat: convert_latin1_to_utf8_s

f80d5b1

Adds a "safe" version of convert_latin1_to_utf8 with a maximum output length. Refs: nodejs/node#54526

ronag added a commit to ronag/simdutf that referenced this pull request Aug 31, 2024

feat: convert_latin1_to_utf8_s

399649f

Adds a "safe" version of convert_latin1_to_utf8 with a maximum output length. Refs: nodejs/node#54526

ronag added a commit to ronag/simdutf that referenced this pull request Aug 31, 2024

feat: convert_latin1_to_utf8_s

ade5fc0

Adds a "safe" version of convert_latin1_to_utf8 with a maximum output length. Refs: nodejs/node#54526

ronag added a commit to ronag/simdutf that referenced this pull request Aug 31, 2024

feat: convert_latin1_to_utf8_s

eff6b01

Adds a "safe" version of convert_latin1_to_utf8 with a maximum output length. Refs: nodejs/node#54526

buffer: validate UTF8 on fast path

8c1de9b

Fast API handles invalid UTF differently than the slow API. Fixes: nodejs#54521 PR-URL: nodejs#54525

ronag force-pushed the fast-utf8-2 branch from 3ce11da to 8c1de9b Compare August 31, 2024 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: validate UTF8 on fast path #54526

buffer: validate UTF8 on fast path #54526

ronag commented Aug 23, 2024 •

edited

Loading

lemire Aug 23, 2024

anonrig Aug 23, 2024

lemire left a comment

codecov bot commented Aug 23, 2024 •

edited

Loading

This comment was marked as outdated.

jasnell left a comment

This comment was marked as outdated.

blexrob commented Aug 24, 2024 •

edited by VoltrexKeyva

Loading

ronag commented Aug 24, 2024

ronag commented Aug 24, 2024

ronag commented Aug 30, 2024 •

edited

Loading

ronag commented Aug 30, 2024 •

edited

Loading

ronag commented Aug 30, 2024

ShenHongFei commented Aug 30, 2024

nodejs-github-bot commented Aug 30, 2024

nodejs-github-bot commented Aug 30, 2024

nodejs-github-bot commented Aug 30, 2024

nodejs-github-bot commented Aug 31, 2024

nodejs-github-bot commented Aug 31, 2024

nodejs-github-bot commented Aug 31, 2024

nodejs-github-bot commented Aug 31, 2024

buffer: validate UTF8 on fast path #54526

Are you sure you want to change the base?

buffer: validate UTF8 on fast path #54526

Conversation

ronag commented Aug 23, 2024 • edited Loading

lemire Aug 23, 2024

Choose a reason for hiding this comment

anonrig Aug 23, 2024

Choose a reason for hiding this comment

lemire left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 23, 2024 • edited Loading

Codecov Report

This comment was marked as outdated.

jasnell left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

blexrob commented Aug 24, 2024 • edited by VoltrexKeyva Loading

ronag commented Aug 24, 2024

ronag commented Aug 24, 2024

ronag commented Aug 30, 2024 • edited Loading

ronag commented Aug 30, 2024 • edited Loading

ronag commented Aug 30, 2024

ShenHongFei commented Aug 30, 2024

nodejs-github-bot commented Aug 30, 2024

nodejs-github-bot commented Aug 30, 2024

nodejs-github-bot commented Aug 30, 2024

nodejs-github-bot commented Aug 31, 2024

nodejs-github-bot commented Aug 31, 2024

nodejs-github-bot commented Aug 31, 2024

nodejs-github-bot commented Aug 31, 2024

ronag commented Aug 23, 2024 •

edited

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

blexrob commented Aug 24, 2024 •

edited by VoltrexKeyva

Loading

ronag commented Aug 30, 2024 •

edited

Loading

ronag commented Aug 30, 2024 •

edited

Loading