Skip to content

Comments

stream: fix decoded fromList chunk boundary check#61884

Merged
nodejs-github-bot merged 2 commits intonodejs:mainfrom
watson:fix-readable-stream-bug
Feb 22, 2026
Merged

stream: fix decoded fromList chunk boundary check#61884
nodejs-github-bot merged 2 commits intonodejs:mainfrom
watson:fix-readable-stream-bug

Conversation

@watson
Copy link
Member

@watson watson commented Feb 19, 2026

Correct fromList() in decoded string mode to compare n against the current chunk length, not the buffer array length.

This prevents over-consuming chunks, which can corrupt readable state and crash with TypeError when mixing setEncoding() and read(n).

I reproduced this in all non-EoL release lines.

For reference, here's the script I used to make read() throw (usually it throws almost instantly, but a few times it can take a little while):

'use strict'

const https = require('node:https')

const url = process.argv[2] || 'https://www.google.com'
const maxRuns = Number(process.argv[3] || 2000)
let runs = 0

function runOnce () {
  runs++
  https.get(url, (res) => {
    res.setEncoding('utf8') // If this line is removed, the crash doesn't happen.

    res.on('readable', () => {
      // Race condition: The call to `res.read(100)` might throw
      while (res.read(100) !== null) {}
    })

    res.on('end', () => {
      if (runs >= maxRuns) {
        console.log(`completed ${runs} runs without crash`)
      } else {
        runOnce()
      }
    })
  })
}

console.log(`reproducing with ${url} for up to ${maxRuns} runs...`)
runOnce()

And here's the output:

reproducing with https://www.google.com for up to 2000 runs...
node:internal/streams/readable:1650
  } else if (n < buf[idx].length) {
                          ^

TypeError: Cannot read properties of undefined (reading 'length')
    at fromList (node:internal/streams/readable:1650:27)
    at Readable.read (node:internal/streams/readable:756:11)
    at IncomingMessage.<anonymous> (/Users/thomas.watson/go/src/github.com/DataDog/dd-trace-js/foo.js:16:18)
    at IncomingMessage.emit (node:events:508:20)
    at emitReadable_ (node:internal/streams/readable:837:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:89:21)

Node.js v25.6.1

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels Feb 19, 2026
Correct `fromList()` in decoded string mode to compare `n` against the
current chunk length, not the buffer array length.

This prevents over-consuming chunks, which can corrupt readable state
and crash with `TypeError` when mixing `setEncoding()` and `read(n)`.
@watson watson force-pushed the fix-readable-stream-bug branch from 8a962b7 to 5ef134f Compare February 19, 2026 10:30
@watson watson changed the title fix(stream): fix decoded fromList chunk boundary check stream: fix decoded fromList chunk boundary check Feb 19, 2026
@BridgeAR BridgeAR added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 19, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2026
@nodejs-github-bot
Copy link
Collaborator

@codecov
Copy link

codecov bot commented Feb 19, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.73%. Comparing base (d5279e5) to head (2984f13).
⚠️ Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/streams/readable.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61884      +/-   ##
==========================================
- Coverage   91.81%   89.73%   -2.08%     
==========================================
  Files         338      675     +337     
  Lines      140073   204855   +64782     
  Branches    22081    39372   +17291     
==========================================
+ Hits       128605   183829   +55224     
- Misses      11242    13287    +2045     
- Partials      226     7739    +7513     
Files with missing lines Coverage Δ
lib/internal/streams/readable.js 97.16% <0.00%> (+0.88%) ⬆️

... and 456 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added request-ci Add this label to start a Jenkins CI on a PR. commit-queue Add this label to land a pull request using GitHub Actions. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Feb 19, 2026
@nodejs-github-bot 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 Feb 21, 2026
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/61884
✔  Done loading data for nodejs/node/pull/61884
----------------------------------- PR info ------------------------------------
Title      stream: fix decoded fromList chunk boundary check (#61884)
Author     Thomas Watson <w@tson.dk> (@watson)
Branch     watson:fix-readable-stream-bug -> nodejs:main
Labels     stream, author ready, needs-ci
Commits    2
 - stream: fix decoded fromList chunk boundary check
 - fix lint
Committers 1
 - Thomas Watson <w@tson.dk>
PR-URL: https://github.com/nodejs/node/pull/61884
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ilyas Shabi <ilyasshabi94@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/61884
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ilyas Shabi <ilyasshabi94@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 19 Feb 2026 10:28:10 GMT
   ✔  Approvals: 9
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/61884#pullrequestreview-3824865677
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/61884#pullrequestreview-3824921277
   ✔  - Ilyas Shabi (@IlyasShabi): https://github.com/nodejs/node/pull/61884#pullrequestreview-3825273620
   ✔  - Gürgün Dayıoğlu (@gurgunday): https://github.com/nodejs/node/pull/61884#pullrequestreview-3825732569
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/61884#pullrequestreview-3828304463
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/61884#pullrequestreview-3828525776
   ✔  - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/61884#pullrequestreview-3830500467
   ✔  - Colin Ihrig (@cjihrig): https://github.com/nodejs/node/pull/61884#pullrequestreview-3832438069
   ✔  - Ethan Arrowood (@Ethan-Arrowood): https://github.com/nodejs/node/pull/61884#pullrequestreview-3832584001
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2026-02-19T11:05:46Z: https://ci.nodejs.org/job/node-test-pull-request/71377/
- Querying data for job/node-test-pull-request/71377/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 61884
From https://github.com/nodejs/node
 * branch                  refs/pull/61884/merge -> FETCH_HEAD
✔  Fetched commits as 94df36a12199..2984f1316238
--------------------------------------------------------------------------------
[main 555cb3c22c] stream: fix decoded fromList chunk boundary check
 Author: Thomas Watson <w@tson.dk>
 Date: Thu Feb 19 11:25:19 2026 +0100
 2 files changed, 16 insertions(+), 1 deletion(-)
 create mode 100644 test/parallel/test-stream2-read-correct-num-bytes-in-utf8.js
[main 78184b3a60] fix lint
 Author: Thomas Watson <w@tson.dk>
 Date: Thu Feb 19 11:43:24 2026 +0100
 1 file changed, 1 insertion(+)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
(node:458) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated.
(Use `node --trace-deprecation ...` to show where the warning was created)
Rebasing (2/4)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
stream: fix decoded fromList chunk boundary check

Correct fromList() in decoded string mode to compare n against the
current chunk length, not the buffer array length.

This prevents over-consuming chunks, which can corrupt readable state
and crash with TypeError when mixing setEncoding() and read(n).

PR-URL: #61884
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ilyas Shabi <ilyasshabi94@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>

[detached HEAD ca615c81bd] stream: fix decoded fromList chunk boundary check
Author: Thomas Watson <w@tson.dk>
Date: Thu Feb 19 11:25:19 2026 +0100
2 files changed, 16 insertions(+), 1 deletion(-)
create mode 100644 test/parallel/test-stream2-read-correct-num-bytes-in-utf8.js
Rebasing (3/4)
Rebasing (4/4)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fix lint

PR-URL: #61884
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ilyas Shabi <ilyasshabi94@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>

[detached HEAD 403f5bd46a] fix lint
Author: Thomas Watson <w@tson.dk>
Date: Thu Feb 19 11:43:24 2026 +0100
1 file changed, 1 insertion(+)
Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

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

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Feb 22, 2026
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 22, 2026
@nodejs-github-bot nodejs-github-bot merged commit d10104b into nodejs:main Feb 22, 2026
86 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in d10104b

aduh95 pushed a commit that referenced this pull request Feb 22, 2026
Correct `fromList()` in decoded string mode to compare `n` against the
current chunk length, not the buffer array length.

This prevents over-consuming chunks, which can corrupt readable state
and crash with `TypeError` when mixing `setEncoding()` and `read(n)`.

PR-URL: #61884
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ilyas Shabi <ilyasshabi94@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
@watson watson deleted the fix-readable-stream-bug branch February 22, 2026 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.