Merge pull request #2916 from heygen-com/ffprobe-5-invocation

fix(engine): reject stdin input, decode stdout correctly, bound its size

Author: vanceingalls

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/,
+    );
+  });
+});

packages/engine/src/utils/ffprobe.ts

@@ -2,13 +2,18 @@
 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";
 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,19 +53,57 @@ async function runFfprobe(
   argsWithoutInput: string[],
   signal?: AbortSignal,
 ): Promise<string> {
+  // `--` 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,
     deadlineAtMs: Date.now() + 30_000,
     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();