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();