From 9e275423e2f44e37d73d5b7372a3ead3ba95cf75 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Fri, 31 Jul 2026 00:10:32 -0700 Subject: [PATCH] fix(engine): reject stdin input, decode stdout correctly, bound its size MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three issues in runFfprobe's process and stream handling. A filePath of exactly "-" hung for 30 seconds. `--` stops option parsing, so "-intro.mp4" is safe, but ffprobe rewrites "-" to `fd:` AFTER option parsing and reads stdin — and stdin was an inherited pipe the parent never writes to and never ends. The probe ran to the deadline and failed with an empty diagnostic, because ffprobe never errored so stderr was blank: 30010 ms and no message, against 28 ms for a normal missing-file error. Rejected up front, and the child now gets stdio ["ignore", ...] so no future invocation can block on stdin either. stdout was decoded per chunk. `stdout += data.toString()` decodes each 64 KiB pipe chunk independently, so a multi-byte character straddling a boundary became U+FFFD on both sides — verified: 200 KB of 3-byte characters produced 15 replacements and a string 9 characters longer than the source. -show_format output above ~64 KiB with non-ASCII tag text returns silently mangled values, since JSON.parse still succeeds. Now accumulated through StringDecoder. Note on testing that one: U+FFFD is valid JSON string content, and nothing on extractMediaMetadata's public surface exposes a tag value, so there is no assertion that fails against the old implementation. Rather than add a test that cannot fail, it is stated here and the bound below is what the new test covers. stdout was unbounded. stderr is capped by ManagedChildProcess but stdout was not, and analyzeKeyframeIntervals emits one line per frame — an all-intra ProRes proxy can produce an arbitrarily large string. Capped at 8M characters, which real -show_streams JSON is nowhere near. Tests: "-" rejected without spawning, the stdio shape, and the size bound. Reverting the stdin guards fails 1. The first draft of the bound checked before appending, so a single oversized chunk passed — the test caught it. Co-Authored-By: Claude Opus 5 (1M context) --- packages/engine/src/utils/ffprobe.test.ts | 83 ++++++++++++++++++++++- packages/engine/src/utils/ffprobe.ts | 49 ++++++++++++- 2 files changed, 127 insertions(+), 5 deletions(-) diff --git a/packages/engine/src/utils/ffprobe.test.ts b/packages/engine/src/utils/ffprobe.test.ts index 9c6de657e3..ad9727098a 100644 --- a/packages/engine/src/utils/ffprobe.test.ts +++ b/packages/engine/src/utils/ffprobe.test.ts @@ -128,7 +128,15 @@ interface FakeProc extends EventEmitter { type SpawnOutcome = | { kind: "missing" } | { kind: "error"; message: string; code?: string } - | { kind: "exit"; code: number; stdout?: string; stderr?: string }; + | { + kind: "exit"; + code: number; + stdout?: string; + stderr?: string; + /** Emit stdout as these exact byte chunks, to exercise a multi-byte + * character split across a pipe-chunk boundary. */ + stdoutChunks?: Buffer[]; + }; function createSpawnSpy(outcomes: SpawnOutcome[]): { spawn: (command: string, args: readonly string[]) => FakeProc; @@ -159,7 +167,9 @@ function createSpawnSpy(outcomes: SpawnOutcome[]): { proc.emit("error", err); return; } - if (outcome.stdout) proc.stdout.emit("data", Buffer.from(outcome.stdout)); + if (outcome.stdoutChunks) { + for (const chunk of outcome.stdoutChunks) proc.stdout.emit("data", chunk); + } else if (outcome.stdout) proc.stdout.emit("data", Buffer.from(outcome.stdout)); if (outcome.stderr) proc.stderr.emit("data", Buffer.from(outcome.stderr)); proc.emit("close", outcome.code); }); @@ -961,3 +971,72 @@ describe("AAC duration refinement must never fail or distort the call", () => { expect(meta.durationSeconds).toBeCloseTo((861 * 1024) / 44100, 5); }); }); + +describe("runFfprobe process and stream handling", () => { + afterEach(() => { + vi.resetModules(); + vi.doUnmock("child_process"); + }); + + // Regression: `--` protects "-intro.mp4" but not a path of exactly "-", + // which ffprobe rewrites to fd: AFTER option parsing and then reads stdin. + // With stdin left as an unwritten pipe the probe hung for the full 30s + // deadline and failed with an empty diagnostic. + it("rejects a filePath of '-' immediately instead of hanging on stdin", async () => { + const { spawn, calls } = createSpawnSpy([{ kind: "exit", code: 0, stdout: "{}" }]); + vi.resetModules(); + vi.doMock("child_process", () => ({ spawn })); + const { extractMediaMetadata } = await import("./ffprobe.js"); + + await expect(extractMediaMetadata("-")).rejects.toThrow(/stdin is not a supported input path/); + expect(calls).toHaveLength(0); + }); + + it("never leaves the child's stdin as a writable pipe", async () => { + const stdios: unknown[] = []; + const spawn = (_c: string, _a: readonly string[], opts?: { stdio?: unknown }) => { + stdios.push(opts?.stdio); + const proc = new EventEmitter() as FakeProc; + proc.stdout = new EventEmitter(); + proc.stderr = new EventEmitter(); + process.nextTick(() => { + proc.stdout.emit( + "data", + Buffer.from( + JSON.stringify({ + streams: [{ codec_type: "video", codec_name: "h264", width: 2, height: 2 }], + format: { duration: "1" }, + }), + ), + ); + proc.emit("close", 0); + }); + return proc; + }; + vi.resetModules(); + vi.doMock("child_process", () => ({ spawn })); + const { extractMediaMetadata } = await import("./ffprobe.js"); + + await extractMediaMetadata("/tmp/stdio-shape.mp4"); + expect(stdios[0]).toEqual(["ignore", "pipe", "pipe"]); + }); + + // NOTE on the StringDecoder change: a per-chunk toString() corrupts a + // multi-byte character split across a pipe boundary into U+FFFD, but + // U+FFFD is valid JSON string content, so JSON.parse still succeeds and + // extractMediaMetadata's public surface returns nothing that exposes the + // mangled tag value. There is no assertion here that fails on the old + // implementation, so rather than ship a test that cannot fail, the + // corruption is stated in the commit and this covers the bound instead. + it("refuses to parse stdout that exceeds the size bound", async () => { + const huge = "x".repeat(8_000_001); + const { spawn } = createSpawnSpy([{ kind: "exit", code: 0, stdout: huge }]); + vi.resetModules(); + vi.doMock("child_process", () => ({ spawn })); + const { extractMediaMetadata } = await import("./ffprobe.js"); + + await expect(extractMediaMetadata("/tmp/unbounded-output.mov")).rejects.toThrow( + /exceeded 8000000 characters/, + ); + }); +}); diff --git a/packages/engine/src/utils/ffprobe.ts b/packages/engine/src/utils/ffprobe.ts index acc47bfb98..da10672fb7 100644 --- a/packages/engine/src/utils/ffprobe.ts +++ b/packages/engine/src/utils/ffprobe.ts @@ -2,6 +2,7 @@ import { spawn } from "child_process"; import { readFileSync } from "fs"; import * as zlib from "node:zlib"; +import { StringDecoder } from "node:string_decoder"; import { basename, extname } from "path"; import { redactTelemetryString } from "@hyperframes/core"; import { FFPROBE_PATH_ENV, getFfprobeBinary } from "./ffmpegBinaries.js"; @@ -9,6 +10,10 @@ import { ManagedChildProcess } from "./managedChildProcess.js"; import { trackChildProcess } from "./processTracker.js"; const FFPROBE_STDERR_MAX_BYTES = 8 * 1024; +/** Bound on collected stdout. Generous — real -show_streams JSON is well + * under this — but finite, unlike the previous unbounded accumulation. */ +const FFPROBE_STDOUT_MAX_CHARS = 8_000_000; + const FFPROBE_ERROR_MAX_CHARS = 4 * 1024; function redactFfprobeInput(stderr: string, filePath: string): string { @@ -48,12 +53,44 @@ async function runFfprobe( argsWithoutInput: string[], signal?: AbortSignal, ): Promise { + // `--` stops option parsing so a path like "-intro.mp4" is a filename, but + // it does NOT cover a path of exactly "-": ffprobe rewrites that to `fd:` + // AFTER option parsing and then reads stdin. Since stdin here is a pipe the + // parent never writes to and never ends, the probe hangs for the full 30s + // deadline and fails with an empty diagnostic (ffprobe never errored, so + // stderr is blank). Reject it up front with something a caller can read. + if (filePath === "-") { + throw new Error('[FFmpeg] Refusing to probe "-": stdin is not a supported input path.'); + } + const command = getFfprobeBinary(); - const proc = spawn(command, ["-v", "error", ...argsWithoutInput, "--", filePath]); + const proc = spawn(command, ["-v", "error", ...argsWithoutInput, "--", filePath], { + // Nothing is ever written to the child's stdin; leaving it as a pipe is + // what lets a stdin-reading invocation block indefinitely. + stdio: ["ignore", "pipe", "pipe"], + }); trackChildProcess(proc); + // Decoded through StringDecoder rather than per-chunk toString(): a + // multi-byte character split across a 64 KiB pipe boundary decodes to U+FFFD + // on both sides. -show_format output above ~64 KiB with non-ASCII tag text + // (an MKV with many chapters, or title/artist tags) came back silently + // mangled — JSON.parse still succeeds, so nothing surfaced it, and tag + // lookups like alpha_mode could miss. + const decoder = new StringDecoder("utf8"); let stdout = ""; - proc.stdout.on("data", (data) => { - stdout += data.toString(); + let stdoutTruncated = false; + proc.stdout.on("data", (data: Buffer) => { + // stderr is capped by ManagedChildProcess; stdout had no bound at all, and + // analyzeKeyframeIntervals emits one line per frame — an all-intra ProRes + // proxy can produce an unbounded string. + if (stdoutTruncated) return; + stdout += decoder.write(data); + // Checked AFTER appending: a single chunk can already exceed the bound, + // so a pre-append check only ever stops the second one. + if (stdout.length > FFPROBE_STDOUT_MAX_CHARS) { + stdoutTruncated = true; + stdout = ""; + } }); const managed = new ManagedChildProcess(proc, { signal, @@ -61,6 +98,12 @@ async function runFfprobe( stderrMaxBytes: FFPROBE_STDERR_MAX_BYTES, }); const outcome = await managed.wait(); + stdout += decoder.end(); + if (stdoutTruncated) { + throw new Error( + `[FFmpeg] ffprobe output exceeded ${FFPROBE_STDOUT_MAX_CHARS} characters; refusing to parse a truncated result.`, + ); + } if (outcome.reason === "spawn_error") { if ((outcome.error as NodeJS.ErrnoException | undefined)?.code === "ENOENT") { const configured = process.env[FFPROBE_PATH_ENV]?.trim();