fix(engine): stop the PNG walk at cICP, anchor IHDR, use native crc32

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) <noreply@anthropic.com>
This commit is contained in:
Vance Ingalls
2026-07-30 23:57:17 -07:00
co-authored by Claude Opus 5
parent 3a6b7f0612
commit 2af3f4d0ed
2 changed files with 92 additions and 13 deletions
+67
View File
@@ -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();
});
});
+25 -13
View File
@@ -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;
}