From 0d503f6d15234b62b67a5453ee42be44bbfd4285 Mon Sep 17 00:00:00 2001 From: Muhammad Faizan Uddin Date: Thu, 24 Sep 2026 22:05:38 -0600 Subject: [PATCH 1/3] test_runner: do not crash on stdout that mimics a v8 frame 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: https://github.com/nodejs/node/issues/66164 Signed-off-by: Muhammad Faizan Uddin --- lib/internal/test_runner/runner.js | 17 +++++- test/parallel/test-runner-v8-deserializer.mjs | 57 +++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index 67c78e03c83..f76af79db99 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -512,12 +512,25 @@ class FileTest extends Test { TypedArrayPrototypeSubarray(concatenatedBuffer, kSerializedSizeHeader, fullMessageSize), ); + let item; + try { + deserializer.readHeader(); + item = deserializer.readValue(); + } catch { + // The bytes begin with the v8 header magic and a plausible size, but + // the payload is not a real serialized message. This happens when a + // test writes raw bytes to stdout that look like a frame. Leave the + // buffer untouched and stop parsing frames here. #drainRawBuffer then + // emits the stray byte as stdout and rescans for the next real header. + break; + } + + // Only advance past the frame once it has been read successfully, so a + // failed read above cannot drop real frames that follow the stray bytes. bufferHead = TypedArrayPrototypeSubarray(concatenatedBuffer, fullMessageSize); this.#rawBufferSize = TypedArrayPrototypeGetLength(bufferHead); this.#rawBuffer = this.#rawBufferSize !== 0 ? [bufferHead] : []; - deserializer.readHeader(); - const item = deserializer.readValue(); this.addToReport(item); } } diff --git a/test/parallel/test-runner-v8-deserializer.mjs b/test/parallel/test-runner-v8-deserializer.mjs index 3a4db367ca6..01af7b6fff9 100644 --- a/test/parallel/test-runner-v8-deserializer.mjs +++ b/test/parallel/test-runner-v8-deserializer.mjs @@ -39,6 +39,17 @@ const oversizedLengthStdout = String.fromCharCode(oversizedLengthHeader[0]) + Buffer.from(oversizedLengthHeader.subarray(1)).toString('utf-8'); const unsignedOversizedLengthStdout = String.fromCharCode(unsignedOversizedLengthHeader[0]) + Buffer.from(unsignedOversizedLengthHeader.subarray(1)).toString('utf-8'); +// FF 0F followed by a small, plausible size (8) and 8 payload bytes. Unlike the +// oversized headers above, this passes the size check and reaches the +// deserializer, which throws because the payload is not a real message. +// Regression fixture for https://github.com/nodejs/node/issues/66164 +const plausibleSizeFalseHeader = Buffer.from([ + 0xff, 0x0f, // v8 serializer header magic + 0x00, 0x00, 0x00, 0x08, // payload size of 8 bytes + 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, // "ABCDEFGH", not a real payload +]); +const plausibleSizeFalseHeaderStdout = String.fromCharCode(plausibleSizeFalseHeader[0]) + + Buffer.from(plausibleSizeFalseHeader.subarray(1)).toString('utf-8'); function collectStdout(reported) { return reported @@ -169,6 +180,52 @@ describe('v8 deserializer', common.mustCall(() => { assert.strictEqual(collectStdout(reported), oversizedLengthStdout); }); + it('should not crash when stdout mimics a v8 frame with a plausible size', async () => { + // Regression test for https://github.com/nodejs/node/issues/66164 + // The bytes reach the deserializer and it throws. The parser must emit + // them as stdout instead of letting the error abort the whole run. + const reported = await collectReported([plausibleSizeFalseHeader]); + assert(reported.every((event) => event.type === 'test:stdout')); + assert.strictEqual(collectStdout(reported), plausibleSizeFalseHeaderStdout); + }); + + it('should resync and parse a real message after a plausible-size false frame', async () => { + // The poison bytes followed by a real serialized message. The parser must + // recover from the failed deserialize and still report the real event. + const reported = await collectReported([ + plausibleSizeFalseHeader, + ...chunks, + ]); + assert.deepStrictEqual(reported.at(-1), reportedDiagnosticEvent); + assert.strictEqual(reported.filter((event) => event.type === 'test:diagnostic').length, 1); + assert.strictEqual(collectStdout(reported), plausibleSizeFalseHeaderStdout); + }); + + it('should preserve real messages on both sides of a plausible-size false frame', async () => { + // A real message, then the poison bytes, then another real message. Both + // real messages must survive and the poison bytes must become stdout. + const reported = await collectReported([ + ...chunks, + plausibleSizeFalseHeader, + ...chunks, + ]); + const diagnostics = reported.filter((event) => event.type === 'test:diagnostic'); + assert.strictEqual(diagnostics.length, 2); + diagnostics.forEach((event) => assert.deepStrictEqual(event, reportedDiagnosticEvent)); + assert.strictEqual(collectStdout(reported), plausibleSizeFalseHeaderStdout); + }); + + it('should recover from a plausible-size false frame split across chunks', async () => { + // The same poison bytes arriving in two chunks must still be treated as + // stdout without crashing. + const reported = await collectReported([ + plausibleSizeFalseHeader.subarray(0, 3), + plausibleSizeFalseHeader.subarray(3), + ]); + assert(reported.every((event) => event.type === 'test:stdout')); + assert.strictEqual(collectStdout(reported), plausibleSizeFalseHeaderStdout); + }); + const headerPosition = headerLength * 2 + 4; for (let i = 0; i < headerPosition + 5; i++) { const message = `should deserialize a serialized message split into two chunks {...${i},${i + 1}...}`; From c63573ec26269d4cd528baf4eabb8249b3b994e7 Mon Sep 17 00:00:00 2001 From: Muhammad Faizan Uddin Date: Fri, 25 Sep 2026 19:19:22 -0600 Subject: [PATCH 2/3] test_runner: validate inner v8 header before deserializing 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 --- lib/internal/test_runner/runner.js | 28 +++++----- test/parallel/test-runner-v8-deserializer.mjs | 52 ++++++++++++++++--- 2 files changed, 58 insertions(+), 22 deletions(-) diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index f76af79db99..eedf123ea1d 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -508,25 +508,23 @@ class FileTest extends Test { const concatenatedBuffer = this.#rawBuffer.length === 1 ? this.#rawBuffer[0] : Buffer.concat(this.#rawBuffer, this.#rawBufferSize); + // A real frame repeats the v8 header at the start of its payload, right + // before the serialized value. If that inner header is missing, these + // are stray stdout bytes that only look like a frame, so stop here and + // let #drainRawBuffer emit them as stdout and resync on the next real + // header. A genuine frame that fails to deserialize is left to throw, so + // real report-protocol regressions are not silently hidden. + if (fullMessageSize - kSerializedSizeHeader < kV8HeaderLength || + concatenatedBuffer.indexOf(v8Header, kSerializedSizeHeader) !== kSerializedSizeHeader) { + break; + } + const deserializer = new DefaultDeserializer( TypedArrayPrototypeSubarray(concatenatedBuffer, kSerializedSizeHeader, fullMessageSize), ); + deserializer.readHeader(); + const item = deserializer.readValue(); - let item; - try { - deserializer.readHeader(); - item = deserializer.readValue(); - } catch { - // The bytes begin with the v8 header magic and a plausible size, but - // the payload is not a real serialized message. This happens when a - // test writes raw bytes to stdout that look like a frame. Leave the - // buffer untouched and stop parsing frames here. #drainRawBuffer then - // emits the stray byte as stdout and rescans for the next real header. - break; - } - - // Only advance past the frame once it has been read successfully, so a - // failed read above cannot drop real frames that follow the stray bytes. bufferHead = TypedArrayPrototypeSubarray(concatenatedBuffer, fullMessageSize); this.#rawBufferSize = TypedArrayPrototypeGetLength(bufferHead); this.#rawBuffer = this.#rawBufferSize !== 0 ? [bufferHead] : []; diff --git a/test/parallel/test-runner-v8-deserializer.mjs b/test/parallel/test-runner-v8-deserializer.mjs index 01af7b6fff9..5258eefd2f0 100644 --- a/test/parallel/test-runner-v8-deserializer.mjs +++ b/test/parallel/test-runner-v8-deserializer.mjs @@ -40,16 +40,37 @@ const oversizedLengthStdout = String.fromCharCode(oversizedLengthHeader[0]) + const unsignedOversizedLengthStdout = String.fromCharCode(unsignedOversizedLengthHeader[0]) + Buffer.from(unsignedOversizedLengthHeader.subarray(1)).toString('utf-8'); // FF 0F followed by a small, plausible size (8) and 8 payload bytes. Unlike the -// oversized headers above, this passes the size check and reaches the -// deserializer, which throws because the payload is not a real message. +// oversized headers above, this passes the size check, but its payload does not +// begin with the inner v8 header a real frame carries, so it is treated as +// stdout instead of reaching the deserializer. // Regression fixture for https://github.com/nodejs/node/issues/66164 const plausibleSizeFalseHeader = Buffer.from([ - 0xff, 0x0f, // v8 serializer header magic - 0x00, 0x00, 0x00, 0x08, // payload size of 8 bytes + 0xff, 0x0f, // V8 serializer header magic + 0x00, 0x00, 0x00, 0x08, // Payload size of 8 bytes 0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, // "ABCDEFGH", not a real payload ]); const plausibleSizeFalseHeaderStdout = String.fromCharCode(plausibleSizeFalseHeader[0]) + Buffer.from(plausibleSizeFalseHeader.subarray(1)).toString('utf-8'); +// FF 0F, a valid size, then the inner v8 header a real frame repeats, followed +// by a byte that is not a valid serialized value. This passes the inner header +// check and reaches the deserializer, which throws. This is what a genuine +// report-protocol regression looks like, so the parser must let the error +// surface instead of hiding it as stdout. +const headeredCorruptFrame = Buffer.from([ + 0xff, 0x0f, // Outer v8 serializer header magic + 0x00, 0x00, 0x00, 0x03, // Payload size of 3 bytes + 0xff, 0x0f, // Inner v8 header that a real frame repeats + 0xee, // Not a valid serialized value +]); +// FF 0F with a declared size of 1, then more header bytes. The payload is +// shorter than the inner v8 header a real frame carries, so it can never be a +// real frame. The length guard must reject it as stdout without reaching the +// deserializer. +const shortPayloadFalseHeader = Buffer.from([ + 0xff, 0x0f, // Outer v8 serializer header magic + 0x00, 0x00, 0x00, 0x01, // Payload size of 1 byte, too short for a header + 0xff, 0x0f, // Trailing bytes that also look like a header +]); function collectStdout(reported) { return reported @@ -182,8 +203,9 @@ describe('v8 deserializer', common.mustCall(() => { it('should not crash when stdout mimics a v8 frame with a plausible size', async () => { // Regression test for https://github.com/nodejs/node/issues/66164 - // The bytes reach the deserializer and it throws. The parser must emit - // them as stdout instead of letting the error abort the whole run. + // The payload does not start with the inner v8 header that a real frame + // carries, so the parser treats the bytes as stdout instead of handing + // them to the deserializer and aborting the whole run. const reported = await collectReported([plausibleSizeFalseHeader]); assert(reported.every((event) => event.type === 'test:stdout')); assert.strictEqual(collectStdout(reported), plausibleSizeFalseHeaderStdout); @@ -191,7 +213,7 @@ describe('v8 deserializer', common.mustCall(() => { it('should resync and parse a real message after a plausible-size false frame', async () => { // The poison bytes followed by a real serialized message. The parser must - // recover from the failed deserialize and still report the real event. + // reject the poison as stdout and still report the real event. const reported = await collectReported([ plausibleSizeFalseHeader, ...chunks, @@ -226,6 +248,22 @@ describe('v8 deserializer', common.mustCall(() => { assert.strictEqual(collectStdout(reported), plausibleSizeFalseHeaderStdout); }); + it('should surface a genuinely corrupt frame instead of hiding it', () => { + // A frame with both v8 headers and a valid size but an invalid value is + // what a real report-protocol regression looks like, not stray stdout. + // The parser must let the deserialize error surface instead of silently + // turning it into stdout. + assert.throws(() => fileTest.parseMessage(headeredCorruptFrame), /deserialize/); + }); + + it('should treat a frame whose payload is shorter than the header as stdout', async () => { + // The declared size is smaller than the inner v8 header, so the length + // guard must reject the bytes as stdout instead of reaching the + // deserializer. + const reported = await collectReported([shortPayloadFalseHeader]); + assert(reported.every((event) => event.type === 'test:stdout')); + }); + const headerPosition = headerLength * 2 + 4; for (let i = 0; i < headerPosition + 5; i++) { const message = `should deserialize a serialized message split into two chunks {...${i},${i + 1}...}`; From 2899307f5171564c21ced504934aa1e82c8e3d4a Mon Sep 17 00:00:00 2001 From: Muhammad Faizan Uddin Date: Sat, 26 Sep 2026 03:30:49 -0600 Subject: [PATCH 3/3] test_runner: resync live after a stray v8 frame Recovery from stray stdout that mimics a frame happened only at shutdown, inside #drainRawBuffer, so a real frame that followed the stray bytes was reported late. Move the recovery into #processRawBuffer. When the payload does not start with the inner v8 header, emit one byte as stdout and restart the header search right away, so the next real frame is reported live. Assert before drain() in the test so it verifies live recovery instead of recovery at shutdown. Signed-off-by: Muhammad Faizan Uddin --- lib/internal/test_runner/runner.js | 140 +++++++++++------- test/parallel/test-runner-v8-deserializer.mjs | 29 +++- 2 files changed, 106 insertions(+), 63 deletions(-) diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index eedf123ea1d..18451ade271 100644 --- a/lib/internal/test_runner/runner.js +++ b/lib/internal/test_runner/runner.js @@ -467,69 +467,97 @@ class FileTest extends Test { #processRawBuffer() { // This method is called when it is known that there is at least one message let bufferHead = this.#rawBuffer[0]; - let headerIndex = bufferHead.indexOf(v8Header); - let nonSerialized = new FastBuffer(); - - while (bufferHead && headerIndex !== 0) { - const nonSerializedData = headerIndex === -1 ? - bufferHead : - bufferHead.slice(0, headerIndex); - nonSerialized = Buffer.concat([nonSerialized, nonSerializedData]); - this.#rawBufferSize -= TypedArrayPrototypeGetLength(nonSerializedData); - if (headerIndex === -1) { - ArrayPrototypeShift(this.#rawBuffer); - } else { - this.#rawBuffer[0] = TypedArrayPrototypeSubarray(bufferHead, headerIndex); + + while (bufferHead) { + // Emit any leading bytes that are not the start of a frame as stdout, + // until a v8 header sits at the front of the buffer. + let headerIndex = bufferHead.indexOf(v8Header); + let nonSerialized = new FastBuffer(); + + while (bufferHead && headerIndex !== 0) { + const nonSerializedData = headerIndex === -1 ? + bufferHead : + bufferHead.slice(0, headerIndex); + nonSerialized = Buffer.concat([nonSerialized, nonSerializedData]); + this.#rawBufferSize -= TypedArrayPrototypeGetLength(nonSerializedData); + if (headerIndex === -1) { + ArrayPrototypeShift(this.#rawBuffer); + } else { + this.#rawBuffer[0] = TypedArrayPrototypeSubarray(bufferHead, headerIndex); + } + bufferHead = this.#rawBuffer[0]; + headerIndex = bufferHead?.indexOf(v8Header); } - bufferHead = this.#rawBuffer[0]; - headerIndex = bufferHead?.indexOf(v8Header); - } - if (TypedArrayPrototypeGetLength(nonSerialized) > 0) { + if (TypedArrayPrototypeGetLength(nonSerialized) > 0) { + this.addToReport({ + __proto__: null, + type: 'test:stdout', + data: { __proto__: null, file: this.name, message: nonSerialized.toString('utf-8') }, + }); + } + + let mimicsFrame = false; + while (bufferHead?.length >= kSerializedSizeHeader) { + // We call `readUInt32BE` manually here, because this is faster than first converting + // it to a buffer and using `readUInt32BE` on that. + const fullMessageSize = (( + bufferHead[kV8HeaderLength] << 24 | + bufferHead[kV8HeaderLength + 1] << 16 | + bufferHead[kV8HeaderLength + 2] << 8 | + bufferHead[kV8HeaderLength + 3] + ) >>> 0) + kSerializedSizeHeader; + + if (this.#rawBufferSize < fullMessageSize) break; + + const concatenatedBuffer = this.#rawBuffer.length === 1 ? + this.#rawBuffer[0] : Buffer.concat(this.#rawBuffer, this.#rawBufferSize); + + // A real frame repeats the v8 header at the start of its payload, right + // before the serialized value. If that inner header is missing, these + // bytes only mimic a frame, so stop deserializing and fall through to + // the resync below. A genuine frame that fails to deserialize is left + // to throw, so real report-protocol regressions are not hidden. + if (fullMessageSize - kSerializedSizeHeader < kV8HeaderLength || + concatenatedBuffer.indexOf(v8Header, kSerializedSizeHeader) !== kSerializedSizeHeader) { + mimicsFrame = true; + break; + } + + const deserializer = new DefaultDeserializer( + TypedArrayPrototypeSubarray(concatenatedBuffer, kSerializedSizeHeader, fullMessageSize), + ); + deserializer.readHeader(); + const item = deserializer.readValue(); + + bufferHead = TypedArrayPrototypeSubarray(concatenatedBuffer, fullMessageSize); + this.#rawBufferSize = TypedArrayPrototypeGetLength(bufferHead); + this.#rawBuffer = this.#rawBufferSize !== 0 ? [bufferHead] : []; + + this.addToReport(item); + } + + if (!mimicsFrame) { + // The head is either exhausted or an incomplete frame we must wait for. + break; + } + + // The head only mimics a frame. Emit its first byte as stdout and restart + // the header search, so a real frame that follows is reported right away + // instead of waiting for #drainRawBuffer at shutdown. This mirrors the no + // progress path in #drainRawBuffer, so the stdout bytes come out the same. this.addToReport({ __proto__: null, type: 'test:stdout', - data: { __proto__: null, file: this.name, message: nonSerialized.toString('utf-8') }, + data: { __proto__: null, file: this.name, message: StringFromCharCode(bufferHead[0]) }, }); - } - - while (bufferHead?.length >= kSerializedSizeHeader) { - // We call `readUInt32BE` manually here, because this is faster than first converting - // it to a buffer and using `readUInt32BE` on that. - const fullMessageSize = (( - bufferHead[kV8HeaderLength] << 24 | - bufferHead[kV8HeaderLength + 1] << 16 | - bufferHead[kV8HeaderLength + 2] << 8 | - bufferHead[kV8HeaderLength + 3] - ) >>> 0) + kSerializedSizeHeader; - - if (this.#rawBufferSize < fullMessageSize) break; - - const concatenatedBuffer = this.#rawBuffer.length === 1 ? - this.#rawBuffer[0] : Buffer.concat(this.#rawBuffer, this.#rawBufferSize); - - // A real frame repeats the v8 header at the start of its payload, right - // before the serialized value. If that inner header is missing, these - // are stray stdout bytes that only look like a frame, so stop here and - // let #drainRawBuffer emit them as stdout and resync on the next real - // header. A genuine frame that fails to deserialize is left to throw, so - // real report-protocol regressions are not silently hidden. - if (fullMessageSize - kSerializedSizeHeader < kV8HeaderLength || - concatenatedBuffer.indexOf(v8Header, kSerializedSizeHeader) !== kSerializedSizeHeader) { - break; + if (TypedArrayPrototypeGetLength(bufferHead) === 1) { + ArrayPrototypeShift(this.#rawBuffer); + } else { + this.#rawBuffer[0] = TypedArrayPrototypeSubarray(bufferHead, 1); } - - const deserializer = new DefaultDeserializer( - TypedArrayPrototypeSubarray(concatenatedBuffer, kSerializedSizeHeader, fullMessageSize), - ); - deserializer.readHeader(); - const item = deserializer.readValue(); - - bufferHead = TypedArrayPrototypeSubarray(concatenatedBuffer, fullMessageSize); - this.#rawBufferSize = TypedArrayPrototypeGetLength(bufferHead); - this.#rawBuffer = this.#rawBufferSize !== 0 ? [bufferHead] : []; - - this.addToReport(item); + this.#rawBufferSize--; + bufferHead = this.#rawBuffer[0]; } } } diff --git a/test/parallel/test-runner-v8-deserializer.mjs b/test/parallel/test-runner-v8-deserializer.mjs index 5258eefd2f0..38a7feae059 100644 --- a/test/parallel/test-runner-v8-deserializer.mjs +++ b/test/parallel/test-runner-v8-deserializer.mjs @@ -211,13 +211,16 @@ describe('v8 deserializer', common.mustCall(() => { assert.strictEqual(collectStdout(reported), plausibleSizeFalseHeaderStdout); }); - it('should resync and parse a real message after a plausible-size false frame', async () => { - // The poison bytes followed by a real serialized message. The parser must - // reject the poison as stdout and still report the real event. - const reported = await collectReported([ - plausibleSizeFalseHeader, - ...chunks, - ]); + it('should resync live and report a real message after a false frame', async () => { + // Feed the poison bytes then a real message but never call drain(). Recovery + // must happen live, so the real event is reported right away. If resync only + // ran at shutdown, the diagnostic would still be buffered and missing here. + // The reporter is a stream, so flush it with end() and finished() before + // asserting, rather than reading it synchronously. + fileTest.parseMessage(plausibleSizeFalseHeader); + chunks.forEach((chunk) => fileTest.parseMessage(chunk)); + fileTest.reporter.end(); + await finished(fileTest.reporter); assert.deepStrictEqual(reported.at(-1), reportedDiagnosticEvent); assert.strictEqual(reported.filter((event) => event.type === 'test:diagnostic').length, 1); assert.strictEqual(collectStdout(reported), plausibleSizeFalseHeaderStdout); @@ -248,6 +251,18 @@ describe('v8 deserializer', common.mustCall(() => { assert.strictEqual(collectStdout(reported), plausibleSizeFalseHeaderStdout); }); + it('should resync through several stray frames in a row', async () => { + // Two false frames back to back in one read, then a real one. The parser + // must peel each stray frame off as stdout and still report the real event. + const reported = await collectReported([ + Buffer.concat([plausibleSizeFalseHeader, plausibleSizeFalseHeader, ...chunks]), + ]); + assert.deepStrictEqual(reported.at(-1), reportedDiagnosticEvent); + assert.strictEqual(reported.filter((event) => event.type === 'test:diagnostic').length, 1); + assert.strictEqual(collectStdout(reported), + plausibleSizeFalseHeaderStdout + plausibleSizeFalseHeaderStdout); + }); + it('should surface a genuinely corrupt frame instead of hiding it', () => { // A frame with both v8 headers and a valid size but an invalid value is // what a real report-protocol regression looks like, not stray stdout.