From 2af3f4d0ed9fac2fadc1266bcadde04e6513d800 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Thu, 30 Jul 2026 23:57:17 -0700 Subject: [PATCH 1/2] fix(engine): stop the PNG walk at cICP, anchor IHDR, use native crc32 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in the PNG metadata fallback, all introduced when the cICP early return became an accumulator. Corrupt trailing chunk nulls a good result. cICP must precede IDAT, so continuing past it only visits chunks this parser ignores — while making whole-file integrity a precondition for returning anything. A truncated or bad-CRC chunk after cICP in an otherwise-good HDR PNG returned null, and extractMediaMetadata then re-throws the ffprobe error it had swallowed instead of using the fallback it just computed: the render dies on a host without FFmpeg, or grades SDR on a build that does not decode cICP. Now stops once dimensions and colour are known. A second IHDR overwrote the dimensions. PNG permits exactly one, first, but nothing enforced that here — a trailing [IHDR 1x1] replaced a real 3840x2160 and the producer laid out a one-pixel image. Anchored to the first. The length guard was also `>= 8` against a spec length of 13, which accepted a truncated header and read height out of the CRC bytes. crc32 was hand-rolled bit-at-a-time and fed a Buffer.concat per chunk. Since the walk no longer stops early it CRC'd whole files: 210 ms on a 12 MiB PNG, 647 ms on a 35 MiB 4K one, synchronously on the event loop, plus ~11 MB of garbage per parse from concatenating a 4-byte type tag onto every chunk. node:zlib's crc32 is native and takes a running seed, so type and data hash in sequence with no copy. 210.28 ms -> 1.291 ms. Tests: 5 regressions — corrupt-after-cICP, truncation after cICP, second IHDR, short IHDR, and that a corrupt IHDR/cICP still rejects. Reverting the break or the anchor fails 3. Co-Authored-By: Claude Opus 5 (1M context) --- packages/engine/src/utils/ffprobe.test.ts | 67 +++++++++++++++++++++++ packages/engine/src/utils/ffprobe.ts | 40 +++++++++----- 2 files changed, 93 insertions(+), 14 deletions(-) diff --git a/packages/engine/src/utils/ffprobe.test.ts b/packages/engine/src/utils/ffprobe.test.ts index 4795d6376d..172418fc98 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 4e59de1a76..b02adfb710 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 (chunkType === "IHDR" && chunkLen >= 8) { + if (chunkCrc32(chunkType, chunkData) !== chunkCrc) return null; + + // 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; } From 96a6e8bd95410695340b2671b1dc167f14f49999 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Fri, 31 Jul 2026 15:54:46 -0700 Subject: [PATCH 2/2] fix(engine): keep the PNG CRC working on Node 22.0/22.1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit zlib.crc32 landed in Node 22.2.0, but engine and cli both declare `"node": ">=22"` and the runtime gate is major-only, so 22.0 and 22.1 are supported. A NAMED import of a missing export throws at module EVALUATION — ffprobe.ts would have failed to load at all on those runtimes, before any PNG was touched, taking every probe with it. Namespace import plus a capability check, with the previous bit-at-a-time implementation retained as the fallback. Modern runtimes keep the 210ms -> 1.3ms win; older ones keep working. Raising the floor to >=22.2.0 was the alternative, but that is a user-facing support change and does not belong in a PNG bug fix. Tests: the same HDR PNG parses identically with the native export absent, and a corrupt chunk still rejects on the fallback path. Co-Authored-By: Claude Opus 5 (1M context) --- packages/engine/src/utils/ffprobe.test.ts | 41 +++++++++++++++++++++++ packages/engine/src/utils/ffprobe.ts | 33 +++++++++++++++--- 2 files changed, 69 insertions(+), 5 deletions(-) diff --git a/packages/engine/src/utils/ffprobe.test.ts b/packages/engine/src/utils/ffprobe.test.ts index 172418fc98..6c2751f93b 100644 --- a/packages/engine/src/utils/ffprobe.test.ts +++ b/packages/engine/src/utils/ffprobe.test.ts @@ -709,3 +709,44 @@ describe("PNG chunk walk — integrity of the fallback itself", () => { ).toBeNull(); }); }); + +describe("crc32 works on every runtime the package declares", () => { + afterEach(() => { + vi.resetModules(); + vi.doUnmock("node:zlib"); + }); + + /** Load ffprobe.ts as it would evaluate on Node 22.0/22.1. */ + async function loadWithoutNativeCrc32() { + const actual = await vi.importActual("node:zlib"); + vi.resetModules(); + // zlib.crc32 landed in 22.2.0, but engine and cli both declare + // `"node": ">=22"` behind a major-only gate. A NAMED import of a missing + // export throws at module evaluation, so ffprobe.ts would fail to load + // entirely on those runtimes — before any PNG is touched. + vi.doMock("node:zlib", () => ({ ...actual, crc32: undefined })); + return import("./ffprobe.js"); + } + + it("parses an HDR PNG identically with the native crc32 unavailable", async () => { + const png = buildPngWithChunks([ + pngChunk("IHDR", [0, 0, 0x0f, 0, 0, 0, 0x08, 0x70, 16, 2, 0, 0, 0]), + pngChunk("cICP", [9, 16, 0, 1]), + pngChunk("IEND", []), + ]); + const withNative = extractPngMetadataFromBuffer(png); + expect(withNative?.colorSpace?.colorTransfer).toBe("smpte2084"); + + const fresh = await loadWithoutNativeCrc32(); + expect(fresh.extractPngMetadataFromBuffer(png)).toEqual(withNative); + }); + + it("rejects a corrupt chunk on the fallback path too", async () => { + const bad = pngChunk("IHDR", [0, 0, 0x0f, 0, 0, 0, 0x08, 0x70, 16, 2, 0, 0, 0]); + bad[bad.length - 1] ^= 0xff; + const png = buildPngWithChunks([bad, pngChunk("IEND", [])]); + + const fresh = await loadWithoutNativeCrc32(); + expect(fresh.extractPngMetadataFromBuffer(png)).toBeNull(); + }); +}); diff --git a/packages/engine/src/utils/ffprobe.ts b/packages/engine/src/utils/ffprobe.ts index b02adfb710..9b367ffbcf 100644 --- a/packages/engine/src/utils/ffprobe.ts +++ b/packages/engine/src/utils/ffprobe.ts @@ -1,7 +1,7 @@ // fallow-ignore-file code-duplication complexity import { spawn } from "child_process"; import { readFileSync } from "fs"; -import { crc32 } from "node:zlib"; +import * as zlib from "node:zlib"; import { basename, extname } from "path"; import { redactTelemetryString } from "@hyperframes/core"; import { FFPROBE_PATH_ENV, getFfprobeBinary } from "./ffmpegBinaries.js"; @@ -170,11 +170,34 @@ interface StillImageMetadata { // 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". +// throwaway buffer: ~210 ms -> ~1.3 ms on a 12 MiB PNG. +// +// It landed in Node 22.2.0, and this repo declares `"node": ">=22"` with a +// major-only runtime gate, so 22.0 and 22.1 are still supported. A NAMED +// import of a missing export throws at module evaluation — i.e. `ffprobe.ts` +// would fail to load at all on those, long before any PNG is parsed — so the +// namespace import plus this capability check is deliberate. Raising the +// floor to 22.2.0 instead would be a user-facing support change, which does +// not belong in a PNG bug fix. +const nativeCrc32 = typeof zlib.crc32 === "function" ? zlib.crc32 : undefined; + +/** Bit-at-a-time fallback for Node 22.0/22.1. Correct, just slower. */ +function crc32Fallback(data: Buffer, seed: number): number { + let crc = seed ^ 0xffffffff; + for (let i = 0; i < data.length; i++) { + crc ^= data[i] ?? 0; + for (let bit = 0; bit < 8; bit++) { + const mask = -(crc & 1); + crc = (crc >>> 1) ^ (0xedb88320 & mask); + } + } + return (crc ^ 0xffffffff) >>> 0; +} + function chunkCrc32(chunkType: string, chunkData: Buffer): number { - return crc32(chunkData, crc32(Buffer.from(chunkType, "ascii"))); + const typeBytes = Buffer.from(chunkType, "ascii"); + if (nativeCrc32) return nativeCrc32(chunkData, nativeCrc32(typeBytes)); + return crc32Fallback(chunkData, crc32Fallback(typeBytes, 0)); } export function extractPngMetadataFromBuffer(buf: Buffer): StillImageMetadata | null {