Skip to content

fix: guard against late chunk after consume teardown#5357

Open
cesarvspr wants to merge 1 commit into
nodejs:mainfrom
cesarvspr:fix/consume-push-null-guard
Open

fix: guard against late chunk after consume teardown#5357
cesarvspr wants to merge 1 commit into
nodejs:mainfrom
cesarvspr:fix/consume-push-null-guard

Conversation

@cesarvspr
Copy link
Copy Markdown
Contributor

This relates to...

Fixes #5356

Rationale

When a body is consumed (body.json()/text()/etc.) and the stream is then torn down (close/abort/timeout/error), consumeFinish sets consume.body = null but leaves stream[kConsume] in place. If a chunk arrives afterwards — e.g. flushed by a decompress interceptor after teardown — push still routes it to consumePush, which did consume.body.push(chunk) on null and crashed synchronously with:

TypeError: Cannot read properties of null (reading 'push')
    at consumePush (lib/api/readable.js:549)

instead of dropping the late chunk and letting the consume promise reject normally.

The same teardown race is already guarded in two sibling code paths — consumeStart (if (consume.body === null) return) and the 'close' handler (if (this[kConsume].body !== null)). consumePush was simply missing the equivalent guard. This parallels the HTTP/2 teardown guards added in #4880.

Changes

  • lib/api/readable.js: add a consume.body === null guard at the top of consumePush so late chunks are dropped instead of crashing.

Features

N/A

Bug Fixes

  • Late chunks arriving after consume teardown no longer crash; .json()/.text() reject with UND_ERR_ABORTED as expected.

Breaking Changes and Deprecations

N/A

Status

  • I have read and agreed to the Developer's Certificate of Origin
  • Tested
  • [S] Benchmarked (optional)
  • Documented (code comment; no public API change)
  • Review ready
  • In review
  • Merge ready

Test

Added a regression test in test/readable.js (late chunk after consume teardown does not crash). It fails on main with the null-deref and passes with the fix:

not ok 1 - late chunk after consume teardown does not crash   # before fix
        Actual message: "Cannot read properties of null (reading 'push')"

ok  - late chunk after consume teardown does not crash         # after fix

node --test test/readable.js → 10/10 passing; npm run lint clean.

`consumeFinish` sets `consume.body = null` on teardown (close/abort/error)
but leaves `stream[kConsume]` in place. A chunk arriving afterwards - for
example flushed by a decompress interceptor - reached `consumePush` and
crashed synchronously with "Cannot read properties of null (reading
'push')" instead of being dropped.

Add a null guard in `consumePush`, consistent with the existing guards in
`consumeStart` and the 'close' handler, so late chunks are dropped and the
consume promise still rejects with the expected AbortError.

Fixes: nodejs#5356
Signed-off-by: cesarvspr <vinicius_spr@hotmail.com>
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.24%. Comparing base (dd1e50a) to head (70e6d62).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5357   +/-   ##
=======================================
  Coverage   93.23%   93.24%           
=======================================
  Files         110      110           
  Lines       36668    36676    +8     
=======================================
+ Hits        34189    34197    +8     
  Misses       2479     2479           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 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
Copy Markdown
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.

I think the bug is elsewhere. That function should not be called.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

consumePush crashes when chunk arrives after body.json() teardown

4 participants