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: #66164
Signed-off-by: Muhammad Faizan Uddin <faizan.uddin94@gmail.com>
PR-URL: #66273
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Author: faizanu94

lib/internal/test_runner/runner.js

@@ -84,8 +84,6 @@ const {
   kTestTimeoutFailure,
   Test,
 } = require('internal/test_runner/test');
-const { FastBuffer } = require('internal/buffer');
-
 const {
   createRandomSeed,
   convertStringToRegExp,
@@ -466,59 +464,87 @@ 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);
+    // This method is called when it is known that there is at least one message.
+    // Each pass looks at the head of the buffer and handles exactly one of four
+    // cases, then re-checks the new head. Recovery from stray stdout that only
+    // mimics a frame happens inline here, so no separate resync pass is needed.
+    while (this.#rawBuffer.length > 0) {
+      const bufferHead = this.#rawBuffer[0];
+      const headerIndex = bufferHead.indexOf(v8Header);
+
+      // 1. The head does not start with a frame header. Emit the bytes before
+      //    the next header, or the whole head when there is none, as stdout and
+      //    advance to the next header.
+      if (headerIndex !== 0) {
+        const nonSerialized = headerIndex === -1 ?
+          bufferHead : TypedArrayPrototypeSubarray(bufferHead, 0, headerIndex);
+        this.addToReport({
+          __proto__: null,
+          type: 'test:stdout',
+          data: { __proto__: null, file: this.name, message: nonSerialized.toString('utf-8') },
+        });
+        this.#rawBufferSize -= TypedArrayPrototypeGetLength(nonSerialized);
+        if (headerIndex === -1) {
+          ArrayPrototypeShift(this.#rawBuffer);
+        } else {
+          this.#rawBuffer[0] = TypedArrayPrototypeSubarray(bufferHead, headerIndex);
+        }
+        continue;
       }
-      bufferHead = this.#rawBuffer[0];
-      headerIndex = bufferHead?.indexOf(v8Header);
-    }
-
-    if (TypedArrayPrototypeGetLength(nonSerialized) > 0) {
-      this.addToReport({
-        __proto__: null,
-        type: 'test:stdout',
-        data: { __proto__: null, file: this.name, message: nonSerialized.toString('utf-8') },
-      });
-    }
 
-    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.
+      // 2. The head starts with a header but the whole frame has not arrived
+      //    yet. Stop and wait for more data.
+      if (TypedArrayPrototypeGetLength(bufferHead) < kSerializedSizeHeader) {
+        break;
+      }
+      // 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;
+      if (this.#rawBufferSize < fullMessageSize) {
+        break;
+      }
 
       const concatenatedBuffer = this.#rawBuffer.length === 1 ?
-        this.#rawBuffer[0] : Buffer.concat(this.#rawBuffer, this.#rawBufferSize);
+        bufferHead : Buffer.concat(this.#rawBuffer, this.#rawBufferSize);
+
+      // 3. The head only mimics a frame. A real frame repeats the v8 header at
+      //    the start of its payload, right before the serialized value, so a
+      //    payload too short for that header, or one that does not start with
+      //    it, is stray stdout. Emit one byte and let the next pass resync on
+      //    the following header. 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) {
+        this.addToReport({
+          __proto__: null,
+          type: 'test:stdout',
+          data: { __proto__: null, file: this.name, message: StringFromCharCode(bufferHead[0]) },
+        });
+        if (TypedArrayPrototypeGetLength(bufferHead) === 1) {
+          ArrayPrototypeShift(this.#rawBuffer);
+        } else {
+          this.#rawBuffer[0] = TypedArrayPrototypeSubarray(bufferHead, 1);
+        }
+        this.#rawBufferSize--;
+        continue;
+      }
 
+      // 4. A real frame. Deserialize it and continue from the remaining bytes.
       const deserializer = new DefaultDeserializer(
         TypedArrayPrototypeSubarray(concatenatedBuffer, kSerializedSizeHeader, fullMessageSize),
       );
-
-      bufferHead = TypedArrayPrototypeSubarray(concatenatedBuffer, fullMessageSize);
-      this.#rawBufferSize = TypedArrayPrototypeGetLength(bufferHead);
-      this.#rawBuffer = this.#rawBufferSize !== 0 ? [bufferHead] : [];
-
       deserializer.readHeader();
       const item = deserializer.readValue();
+
+      const remaining = TypedArrayPrototypeSubarray(concatenatedBuffer, fullMessageSize);
+      this.#rawBufferSize = TypedArrayPrototypeGetLength(remaining);
+      this.#rawBuffer = this.#rawBufferSize !== 0 ? [remaining] : [];
+
       this.addToReport(item);
     }
   }

test/parallel/test-runner-v8-deserializer.mjs

@@ -39,6 +39,38 @@ 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, 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
+  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
@@ -169,6 +201,84 @@ 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 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);
+  });
+
+  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);
+  });
+
+  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);
+  });
+
+  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.
+    // 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}...}`;