Skip to content

test_runner: do not crash on stdout that mimics a v8 frame - #66273

Open
faizanu94 wants to merge 2 commits into
nodejs:mainfrom
faizanu94:test-runner-stdout-frame-guard
Open

faizanu94 wants to merge 2 commits into
nodejs:mainfrom
faizanu94:test-runner-stdout-frame-guard

Conversation

@faizanu94

Copy link
Copy Markdown

Summary

node --test can crash the parent runner when a test writes bytes to stdout that look like a v8 report frame. A single console.log of the wrong bytes aborts the whole run with an internal error that does not point at any test:

Error: Unable to deserialize cloned data due to invalid or unsupported version.
    at #processRawBuffer (node:internal/test_runner/runner:...)

Fixes: #66164

Root cause

The child test process multiplexes framed report messages and raw user stdout on one pipe. A frame starts with the two magic bytes FF 0F followed by a 4 byte size. #processRawBuffer scans for that magic, reads the size and hands the payload to the v8 deserializer. User output can contain those same bytes, so stray stdout with a plausible size reaches the deserializer, which throws. The call had no error handling, so the exception aborted the entire run. The earlier >>> 0 fix in #64706 hardened the size read only. This case has a valid looking size and fails one step later at the deserialize.

Fix

Two small changes in #processRawBuffer:

  1. Advance the buffer past a frame only after it has been read successfully, so a failed read cannot drop real frames that follow.
  2. Wrap readHeader() and readValue() in try/catch. On failure, leave the buffer untouched and stop parsing frames. The existing #drainRawBuffer no progress path then emits the stray byte as stdout and rescans for the next real header.

This turns a fatal crash into recoverable stdout, keeps any real frames that follow the stray bytes and matches how the runner already treats false headers with an oversized size.

Tests

Added four cases to test/parallel/test-runner-v8-deserializer.mjs:

  • stdout that mimics a frame with a plausible size becomes stdout and does not crash
  • the same false frame followed by a real message: the parser resyncs and still reports the real event
  • a real message, then the false frame, then another real message: both real messages survive
  • the false frame split across two chunks still recovers as stdout

Known limitation (possible follow-up, out of scope here)

If stray bytes ever form a valid v8 payload, the deserialize would succeed and report a bogus item. This is pre-existing and extremely unlikely. Fully removing the ambiguity needs an escape mechanism or a separate channel for user stdout, which is a larger change. This PR removes the crash, which is the reported bug.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Sep 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

@inoway46

Copy link
Copy Markdown
Contributor

This seems reasonable as a mitigation, but I'm concerned that swallowing deserialization errors could hide real regressions in the internal report protocol.

Could we make the frame validation stricter before deserializing instead? For example, checking for the expected inner V8 header after the length field may let us reject false positives as stdout without hiding genuine deserialization failures.

Separating the report stream from user stdout still seems like the cleanest long-term fix, but stricter validation might be a smaller alternative.

@codecov

codecov Bot commented Sep 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.34%. Comparing base (a3bb551) to head (ff40068).
⚠️ Report is 50 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66273      +/-   ##
==========================================
- Coverage   90.35%   90.34%   -0.01%     
==========================================
  Files         789      789              
  Lines      273493   273506      +13     
  Branches    52281    52280       -1     
==========================================
- Hits       247101   247091      -10     
- Misses      16853    16891      +38     
+ Partials     9539     9524      -15     
Files with missing lines Coverage Δ
lib/internal/test_runner/runner.js 95.24% <100.00%> (+0.05%) ⬆️

... and 32 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.

@faizanu94

Copy link
Copy Markdown
Author

Thanks for the sharp review. You are right that a blanket catch could hide a real regression in the report protocol. I pushed a commit that validates the inner v8 header before deserializing, so stray stdout that only mimics a frame is rejected as output, while a genuine frame that fails to deserialize is left to surface instead of being swallowed. I also added a test for that second case. I agree that separating the report stream from user stdout is the cleanest long-term fix.

@faizanu94
faizanu94 force-pushed the test-runner-stdout-frame-guard branch from b5c91f3 to 3a25bf3 Compare September 26, 2026 01:05
The child test process sends framed report messages and raw user stdout
over one pipe, using the bytes FF 0F to mark the start of a frame. User
output can contain those same bytes, so #processRawBuffer could read a
plausible size from stray stdout and hand the bytes to the v8
deserializer. The deserializer then threw. Because the call had no error
handling, the exception aborted the whole test run.

Read the frame before advancing the buffer and wrap the deserialize in a
try/catch. When the read fails, leave the buffer untouched and stop
parsing frames so #drainRawBuffer emits the stray byte as stdout and
rescans for the next real header. This turns a fatal crash into
recoverable stdout and preserves any real frames that follow the stray
bytes.

Fixes: nodejs#66164
Signed-off-by: Muhammad Faizan Uddin <faizan.uddin94@gmail.com>
@faizanu94
faizanu94 force-pushed the test-runner-stdout-frame-guard branch from 3a25bf3 to 8120db3 Compare September 26, 2026 01:09
Check that a framed payload starts with the inner v8 header before
handing it to the deserializer, so stray stdout that mimics a frame is
rejected as output while genuine deserialize failures still surface.

Signed-off-by: Muhammad Faizan Uddin <faizan.uddin94@gmail.com>
@faizanu94
faizanu94 force-pushed the test-runner-stdout-frame-guard branch from 8120db3 to 642807f Compare September 26, 2026 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test_runner: parent runner still crashes on child stdout bytes that mimic an event frame (survives the #64706 fix)

3 participants