diff --git a/packages/engine/src/utils/ffprobe.test.ts b/packages/engine/src/utils/ffprobe.test.ts index 4795d6376..172418fc9 100644 --- a/packages/engine/src/utils/ffprobe.test.ts +++ b/packages/engine/src/utils/ffprobe.test.ts @@ -642,3 +642,70 @@ describe("extractPngMetadataFromBuffer cICP ordering", () => { expect(extractPngMetadataFromBuffer(onlyCicp)).toBeNull(); }); }); + +describe("PNG chunk walk — integrity of the fallback itself", () => { + const IHDR_4K = [0, 0, 0x0f, 0, 0, 0, 0x08, 0x70, 16, 2, 0, 0, 0]; + const CICP_PQ = [9, 16, 0, 1]; + + // Regression: the walk used to continue past cICP to IEND, which made + // whole-file integrity a precondition for returning anything. A damaged + // trailing chunk in an otherwise-good HDR PNG nulled the whole result, and + // extractMediaMetadata then re-throws the ffprobe error it had swallowed + // rather than using the fallback it just computed. + it("returns metadata even when a chunk AFTER cICP is corrupt", () => { + const bad = pngChunk("tEXt", [65, 66]); + bad[bad.length - 1] ^= 0xff; // break the CRC + const png = buildPngWithChunks([ + pngChunk("IHDR", IHDR_4K), + pngChunk("cICP", CICP_PQ), + pngChunk("IDAT", [0x78, 0x9c, 0x63, 0x00, 0x00, 0x00, 0x02, 0x00, 0x01]), + bad, + pngChunk("IEND", []), + ]); + expect(extractPngMetadataFromBuffer(png)).toEqual({ + width: 3840, + height: 2160, + colorSpace: { colorPrimaries: "bt2020", colorTransfer: "smpte2084", colorSpace: "gbr" }, + }); + }); + + it("survives outright truncation after cICP", () => { + const png = buildPngWithChunks([pngChunk("IHDR", IHDR_4K), pngChunk("cICP", CICP_PQ)]); + const truncated = Buffer.concat([png, Buffer.from([0, 0, 0x7f, 0xff, 73, 68, 65, 84])]); + expect(extractPngMetadataFromBuffer(truncated)?.width).toBe(3840); + }); + + // Regression: IHDR had no first-chunk anchor, so a later one overwrote the + // real dimensions and the producer laid out a 1-pixel image. + it("ignores a second IHDR", () => { + const png = buildPngWithChunks([ + pngChunk("IHDR", IHDR_4K), + pngChunk("cICP", CICP_PQ), + pngChunk("IDAT", [0x78, 0x9c, 0x63, 0x00, 0x00, 0x00, 0x02, 0x00, 0x01]), + pngChunk("IHDR", [0, 0, 0, 1, 0, 0, 0, 1, 16, 2, 0, 0, 0]), + pngChunk("IEND", []), + ]); + const meta = extractPngMetadataFromBuffer(png); + expect(meta?.width).toBe(3840); + expect(meta?.height).toBe(2160); + }); + + // A truncated 8-byte IHDR used to be accepted, reading height out of the + // CRC bytes; the spec length is 13. + it("rejects a short IHDR rather than reading garbage dimensions", () => { + const png = buildPngWithChunks([ + pngChunk("IHDR", [0, 0, 0, 7, 0, 0, 0, 9]), + pngChunk("cICP", CICP_PQ), + pngChunk("IEND", []), + ]); + expect(extractPngMetadataFromBuffer(png)).toBeNull(); + }); + + it("still rejects a PNG whose IHDR or cICP itself is corrupt", () => { + const badIhdr = pngChunk("IHDR", IHDR_4K); + badIhdr[badIhdr.length - 1] ^= 0xff; + expect( + extractPngMetadataFromBuffer(buildPngWithChunks([badIhdr, pngChunk("IEND", [])])), + ).toBeNull(); + }); +}); diff --git a/packages/engine/src/utils/ffprobe.ts b/packages/engine/src/utils/ffprobe.ts index 4e59de1a7..b02adfb71 100644 --- a/packages/engine/src/utils/ffprobe.ts +++ b/packages/engine/src/utils/ffprobe.ts @@ -1,6 +1,7 @@ // fallow-ignore-file code-duplication complexity import { spawn } from "child_process"; import { readFileSync } from "fs"; +import { crc32 } from "node:zlib"; import { basename, extname } from "path"; import { redactTelemetryString } from "@hyperframes/core"; import { FFPROBE_PATH_ENV, getFfprobeBinary } from "./ffmpegBinaries.js"; @@ -167,16 +168,13 @@ interface StillImageMetadata { colorSpace: VideoColorSpace | null; } -function crc32(buf: Buffer): number { - let crc = 0xffffffff; - for (let i = 0; i < buf.length; i++) { - crc ^= buf[i] ?? 0; - for (let bit = 0; bit < 8; bit++) { - const mask = -(crc & 1); - crc = (crc >>> 1) ^ (0xedb88320 & mask); - } - } - return (crc ^ 0xffffffff) >>> 0; +// node:zlib's crc32 is native and takes a running seed, so the chunk type and +// the chunk data can be CRC'd in sequence without concatenating them into a +// throwaway buffer. The hand-rolled bit-at-a-time loop this replaces cost +// ~210 ms on a 12 MiB PNG; this is ~1.3 ms. Available on this repo's +// "node": ">=22". +function chunkCrc32(chunkType: string, chunkData: Buffer): number { + return crc32(chunkData, crc32(Buffer.from(chunkType, "ascii"))); } export function extractPngMetadataFromBuffer(buf: Buffer): StillImageMetadata | null { @@ -205,10 +203,14 @@ export function extractPngMetadataFromBuffer(buf: Buffer): StillImageMetadata | if (pos + 12 + chunkLen > buf.length) return null; const chunkData = buf.subarray(pos + 8, pos + 8 + chunkLen); const chunkCrc = buf.readUInt32BE(pos + 8 + chunkLen); - const chunkBytes = Buffer.concat([Buffer.from(chunkType, "ascii"), chunkData]); - if (crc32(chunkBytes) !== chunkCrc) return null; + if (chunkCrc32(chunkType, chunkData) !== chunkCrc) return null; - if (chunkType === "IHDR" && chunkLen >= 8) { + // First IHDR only. PNG permits exactly one and it must come first, but a + // malformed file can carry more — without this anchor a trailing + // [IHDR 1x1] silently replaced the real 4K dimensions, and the producer + // laid out a one-pixel image. `>= 13` is the spec length; the old `>= 8` + // accepted a truncated header and read height out of the CRC bytes. + if (chunkType === "IHDR" && chunkLen >= 13 && width === 0 && height === 0) { width = buf.readUInt32BE(pos + 8); height = buf.readUInt32BE(pos + 12); } @@ -242,6 +244,16 @@ export function extractPngMetadataFromBuffer(buf: Buffer): StillImageMetadata | }; } + // Everything this parser extracts has been found, so stop walking. + // + // Not just an optimisation: cICP must precede IDAT (enforced above), so + // continuing only ever visits chunks we ignore — while making whole-file + // integrity a precondition for returning anything. A truncated or + // bad-CRC trailing chunk in an otherwise-good HDR PNG used to null the + // entire result, and the caller then re-throws the swallowed ffprobe + // error instead of using the fallback it just computed. + if (width > 0 && height > 0 && colorSpaceFromCicp !== null) break; + if (chunkType === "IEND") break; pos += 12 + chunkLen; }