From b1246d3a3cfacc71e24bf29d9c83da2d6776b534 Mon Sep 17 00:00:00 2001 From: Romain TISSERAND Date: Wed, 9 Sep 2026 23:38:11 +0200 Subject: [PATCH] Verify the hunk CRC32 that v3/v4 map entries carry Those entries hold a CRC32 of the decoded hunk and a flag to opt out of it. libchdr parsed the field and never used it, so on the pre-v5 path a hunk that decompressed cleanly to the wrong bytes was returned as valid data - the v5 path has checked its CRC16 all along. MAME rejects the same files with a decompression error. That gap also undercut the reasoning behind DR_FLAC_NO_CRC: dr_flac's own frame CRC is dropped because the hunk CRC is said to cover the same data, and on a v3/v4 AV hunk no hunk CRC was being checked at all. Covered by tests/legacy_decode.c against fixtures from tests/corpus/mklegacy.py, which builds the older layouts directly because chdman only writes v5. Every fixture agrees with MAME 0.264: the intact pair verifies, the two whose stored hunk no longer matches the advertised CRC are refused, and the one setting the opt-out flag reads again. Checked against real CHDv4 images too. The fixtures need only a Python interpreter, so the CMake job builds them; the test skips itself where they are absent, as parent-decode does. Costs about 1.1 KB of text on RV32 with bundled miniz, nearly all of it mz_crc32's table; nothing extra against a shared system zlib, and nothing when VERIFY_BLOCK_CRC is off. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/cmake.yml | 13 ++- cmake/EspRomMinizWorkaround.cmake | 11 ++- src/libchdr_chd.c | 41 +++++++- tests/CMakeLists.txt | 10 ++ tests/corpus/.gitignore | 1 + tests/corpus/generate.sh | 9 ++ tests/corpus/mklegacy.py | 137 +++++++++++++++++++++++++++ tests/legacy_decode.c | 149 ++++++++++++++++++++++++++++++ 8 files changed, 363 insertions(+), 8 deletions(-) create mode 100755 tests/corpus/mklegacy.py create mode 100644 tests/legacy_decode.c diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml index 071c6ce4..b5760339 100644 --- a/.github/workflows/cmake.yml +++ b/.github/workflows/cmake.yml @@ -47,8 +47,15 @@ jobs: - name: Build run: cmake --build ${{github.workspace}}/build --config Release - # Known-answer tests for the hand-optimised ECC and CRC16 primitives. - # Runs across the whole matrix so the pinned values are checked on - # x86-64, arm64, macOS and Windows rather than one host. + # The CHDv3/v4 fixtures need nothing but a Python interpreter - unlike + # the CHDv5 corpus, which needs chdman. Without them legacy-decode + # skips itself, so a runner missing python3 loses coverage rather than + # failing. + - name: Generate the CHDv3/v4 fixtures + shell: bash + run: python3 tests/corpus/mklegacy.py tests/corpus/legacy || true + + # Known-answer tests for the hand-optimised ECC and CRC16 primitives, + # plus the decode tests that have their corpus. - name: Known-answer tests run: ctest --test-dir ${{github.workspace}}/build -C Release --output-on-failure diff --git a/cmake/EspRomMinizWorkaround.cmake b/cmake/EspRomMinizWorkaround.cmake index 713c96ee..aa9080b1 100644 --- a/cmake/EspRomMinizWorkaround.cmake +++ b/cmake/EspRomMinizWorkaround.cmake @@ -28,10 +28,12 @@ # looks exactly like corrupt input or a silicon/codegen bug. # # Renaming the colliding symbols keeps miniz.c's own definitions reachable. -# Only miniz.c references these names, so applying the defines to whatever -# target compiles miniz.c is sufficient. mz_free matters independently of the -# decoder mismatch: bound to ROM it would hand ESP-IDF-heap pointers to the -# ROM allocator. mz_adler32 is benign but renamed for consistency. +# Apply the defines to every target that compiles a translation unit naming +# them - miniz.c itself, and libchdr's own sources, which call mz_crc32. +# mz_free matters independently of the decoder mismatch: bound to ROM it would +# hand ESP-IDF-heap pointers to the ROM allocator. mz_adler32 and mz_crc32 are +# benign - pure functions over a caller buffer, with no shared struct to +# disagree about - but renamed so the whole group stays consistent. # # Deliberately NOT patched into deps/miniz-3.1.2/miniz.h - that tree is # vendored verbatim so it can be re-synced from upstream, and a local edit @@ -56,6 +58,7 @@ set(LIBCHDR_ESP_ROM_MINIZ_COLLISIONS tinfl_decompress_mem_to_mem tinfl_decompress_mem_to_callback mz_adler32 + mz_crc32 mz_free ) diff --git a/src/libchdr_chd.c b/src/libchdr_chd.c index 5c507a77..5e7e844a 100644 --- a/src/libchdr_chd.c +++ b/src/libchdr_chd.c @@ -1040,6 +1040,22 @@ uint16_t chd_crc16(const void *data, uint32_t length) return crc16_update(0xffff, data, length); } +#if VERIFY_BLOCK_CRC +/*------------------------------------------------- + chd_crc32 - calculate CRC32 of a decoded + v1-v4 hunk +-------------------------------------------------*/ + +static uint32_t chd_crc32(const void *data, uint32_t length) +{ +#ifdef CHDR_SYSTEM_ZLIB + return (uint32_t)crc32(0, (const Bytef *)data, length); +#else + return (uint32_t)mz_crc32(MZ_CRC32_INIT, (const mz_uint8 *)data, length); +#endif +} +#endif + /*------------------------------------------------- compressed - test if CHD file is compressed +-------------------------------------------------*/ @@ -3096,7 +3112,15 @@ static chd_error hunk_read_into_memory(chd_file *chd, uint32_t hunknum, uint8_t uint32_t bytes; uint8_t* compressed_bytes; - /* switch off the entry type */ + /* v3/v4 map entries carry a CRC32 of the decoded hunk plus a flag to + * opt out of it; v1/v2 entries have no room for one and map_extract_old() + * sets that flag for them. Checking it is the legacy counterpart of the + * CRC16 check the v5 path does below - without it a hunk that decodes + * cleanly to the wrong bytes is handed back as valid data. Self- and + * parent-referenced entries are covered when the hunk they point at is + * read. + * + * switch off the entry type */ switch (entry->flags & MAP_ENTRY_FLAG_TYPE_MASK) { /* compressed data */ @@ -3118,6 +3142,11 @@ static chd_error hunk_read_into_memory(chd_file *chd, uint32_t hunknum, uint8_t err = chd->codecintf[0]->decompress(codec, compressed_bytes, entry->length, dest, chd->header.hunkbytes); if (err != CHDERR_NONE) return err; +#if VERIFY_BLOCK_CRC + if (!(entry->flags & MAP_ENTRY_FLAG_NO_CRC) && + chd_crc32(dest, chd->header.hunkbytes) != entry->crc) + return CHDERR_DECOMPRESSION_ERROR; +#endif break; } @@ -3126,6 +3155,11 @@ static chd_error hunk_read_into_memory(chd_file *chd, uint32_t hunknum, uint8_t err = hunk_read_uncompressed(chd, entry->offset, chd->header.hunkbytes, dest); if (err != CHDERR_NONE) return err; +#if VERIFY_BLOCK_CRC + if (!(entry->flags & MAP_ENTRY_FLAG_NO_CRC) && + chd_crc32(dest, chd->header.hunkbytes) != entry->crc) + return CHDERR_DECOMPRESSION_ERROR; +#endif break; /* mini-compressed data */ @@ -3133,6 +3167,11 @@ static chd_error hunk_read_into_memory(chd_file *chd, uint32_t hunknum, uint8_t put_bigendian_uint64_t(&dest[0], entry->offset); for (bytes = 8; bytes < chd->header.hunkbytes; bytes++) dest[bytes] = dest[bytes - 8]; +#if VERIFY_BLOCK_CRC + if (!(entry->flags & MAP_ENTRY_FLAG_NO_CRC) && + chd_crc32(dest, chd->header.hunkbytes) != entry->crc) + return CHDERR_DECOMPRESSION_ERROR; +#endif break; /* self-referenced data */ diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 85d41272..8e6557b5 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -42,6 +42,16 @@ target_link_libraries(chdr-kat PRIVATE chdr-static) chdr_test_defines(chdr-kat) add_test(NAME known-answer COMMAND chdr-kat) +# v1-v4 hunk CRC32: seeds/ is CHDv5 only (chdman writes nothing older), and +# these fixtures live outside it for the same reason parent/ does - the +# workflows that walk seeds/ expect every file there to decode cleanly, and +# half of these are meant not to. +add_executable(chdr-legacy-decode legacy_decode.c) +target_link_libraries(chdr-legacy-decode PRIVATE chdr-static) +chdr_test_defines(chdr-legacy-decode) +add_test(NAME legacy-decode + COMMAND chdr-legacy-decode "${CMAKE_CURRENT_SOURCE_DIR}/corpus/legacy") + # CHDs with a parent: seeds/ has none, and cannot (a child needs its parent, and # the workflows there open every file standalone), so COMPRESSION_PARENT would # otherwise go untested - including through the in-place CD spread, where a diff --git a/tests/corpus/.gitignore b/tests/corpus/.gitignore index e127e1e0..685c553c 100644 --- a/tests/corpus/.gitignore +++ b/tests/corpus/.gitignore @@ -3,3 +3,4 @@ !README.md !generate.sh !fetch.sh +!mklegacy.py diff --git a/tests/corpus/generate.sh b/tests/corpus/generate.sh index 034f11d3..a872928c 100755 --- a/tests/corpus/generate.sh +++ b/tests/corpus/generate.sh @@ -127,3 +127,12 @@ if [ -f "$PARENT/base.chd" ]; then -op "$PARENT/base.chd" >/dev/null 2>&1 || true fi fi + +# CHDv3/v4 fixtures for tests/legacy_decode.c. chdman only writes v5, so these +# come from mklegacy.py, which builds the older layouts directly. +LEGACY="$(cd "$(dirname "$0")" && pwd)/legacy" +if command -v python3 >/dev/null; then + python3 "$(dirname "$0")/mklegacy.py" "$LEGACY" +else + echo "python3 not found, skipping the CHDv3/v4 fixtures" >&2 +fi diff --git a/tests/corpus/mklegacy.py b/tests/corpus/mklegacy.py new file mode 100755 index 00000000..38ae6e53 --- /dev/null +++ b/tests/corpus/mklegacy.py @@ -0,0 +1,137 @@ +#!/usr/bin/env python3 +# Generate CHDv3/v4 fixtures. chdman only writes v5, so these are built here +# from the format itself; every file is accepted by MAME's own reader, and the +# deliberately-broken ones are rejected by it. +# +# Layouts follow MAME's chd.cpp (parse_v3_header / parse_v4_header) and its +# 16-byte V34 map entry: offset, CRC32 of the decoded hunk, length, flags. +# libchdr additionally requires the end-of-list cookie MAME writes after the +# last entry. +# +# The pairs generated here differ only in that a hunk's stored data no longer +# matches the CRC32 the map advertises: it still decompresses cleanly, so the +# map CRC is the only thing that can tell. One file sets the per-entry +# "no CRC" flag on that same hunk, which must make it readable again. + +import hashlib +import os +import struct +import sys +import zlib + +V3_HEADER_SIZE = 120 +V4_HEADER_SIZE = 108 +MAP_ENTRY_SIZE = 16 + +TYPE_COMPRESSED = 1 +TYPE_UNCOMPRESSED = 2 +FLAG_NO_CRC = 0x10 + +HD_META_TAG = b'GDDD' +META_FLAG_CHECKSUM = 0x01 + + +def build(path, version, hunkbytes, hunks, payload, crc_payload=None, + nocrc_hunks=(), sector=512): + """Write one CHD. crc_payload, when given, is what the map claims.""" + assert version in (3, 4) + assert len(payload) == hunkbytes * hunks + if crc_payload is None: + crc_payload = payload + + hdr_size = V3_HEADER_SIZE if version == 3 else V4_HEADER_SIZE + map_off = hdr_size + data_off = map_off + MAP_ENTRY_SIZE * (hunks + 1) # + end-of-list cookie + + cylinders = max(1, (hunkbytes * hunks) // (sector * 16 * 32)) + meta = b'CYLS:%d,HEADS:16,SECS:32,BPS:%d\x00' % (cylinders, sector) + + entries, blob, off = [], bytearray(), data_off + for i in range(hunks): + raw = payload[i * hunkbytes:(i + 1) * hunkbytes] + comp = zlib.compress(raw, 9)[2:-4] # raw deflate + if len(comp) < hunkbytes: + etype, data = TYPE_COMPRESSED, comp + else: + etype, data = TYPE_UNCOMPRESSED, raw + flags = etype | (FLAG_NO_CRC if i in nocrc_hunks else 0) + claimed = crc_payload[i * hunkbytes:(i + 1) * hunkbytes] + crc = 0 if i in nocrc_hunks else zlib.crc32(claimed) & 0xffffffff + entries.append((off, crc, len(data), flags)) + blob += data + off += len(data) + + rawmap = bytearray() + for offset, crc, length, flags in entries: + rawmap += struct.pack('>QIHBB', offset, crc, length & 0xffff, + (length >> 16) & 0xff, flags) + + rawsha1 = hashlib.sha1(payload).digest() + # compute_overall_sha1(): sha1(rawsha1 || tag || sha1(metadata)) + overall = hashlib.sha1(rawsha1 + HD_META_TAG + + hashlib.sha1(meta).digest()).digest() + + h = bytearray(hdr_size) + h[0:8] = b'MComprHD' + h[8:12] = struct.pack('>I', hdr_size) + h[12:16] = struct.pack('>I', version) + h[16:20] = struct.pack('>I', 0) # no parent, writable + h[20:24] = struct.pack('>I', 1) # zlib + h[24:28] = struct.pack('>I', hunks) + h[28:36] = struct.pack('>Q', hunkbytes * hunks) + h[36:44] = struct.pack('>Q', off) # metadata follows the hunks + if version == 4: + h[44:48] = struct.pack('>I', hunkbytes) + h[48:68] = overall + h[88:108] = rawsha1 + else: + h[76:80] = struct.pack('>I', hunkbytes) + h[80:100] = rawsha1 # v3 keeps only the raw hash + + meta_blk = struct.pack('>4sIQ', HD_META_TAG, + (META_FLAG_CHECKSUM << 24) | len(meta), 0) + meta + + with open(path, 'wb') as f: + f.write(h) + f.write(rawmap) + f.write(b'EndOfListCookie\0') + f.write(blob) + f.write(meta_blk) + + +def main(outdir): + hunkbytes, hunks = 4096, 12 + os.makedirs(outdir, exist_ok=True) + + # Every fourth hunk is incompressible, so both COMPRESSED and + # UNCOMPRESSED entries are exercised. + rnd = os.urandom(hunkbytes * hunks) + payload = bytearray() + for i in range(hunks): + if i % 4 == 3: + payload += rnd[i * hunkbytes:(i + 1) * hunkbytes] + else: + payload += bytes(((i * 7 + j // 13) & 0xff) for j in range(hunkbytes)) + payload = bytes(payload) + + def flip(buf, hunk, off=17): + b = bytearray(buf) + b[hunk * hunkbytes + off] ^= 0x40 + return bytes(b) + + bad_comp = flip(payload, 5) # a COMPRESSED hunk + bad_raw = flip(payload, 7) # an UNCOMPRESSED one + + with open(os.path.join(outdir, 'plain.raw'), 'wb') as f: + f.write(payload) + for v in (3, 4): + p = lambda n: os.path.join(outdir, 'v%d_%s.chd' % (v, n)) + build(p('plain'), v, hunkbytes, hunks, payload) + build(p('badcomp'), v, hunkbytes, hunks, bad_comp, crc_payload=payload) + build(p('badraw'), v, hunkbytes, hunks, bad_raw, crc_payload=payload) + build(p('nocrc'), v, hunkbytes, hunks, bad_comp, crc_payload=payload, + nocrc_hunks=(5,)) + + +if __name__ == '__main__': + main(sys.argv[1] if len(sys.argv) > 1 else '.') diff --git a/tests/legacy_decode.c b/tests/legacy_decode.c new file mode 100644 index 00000000..0d918ca6 --- /dev/null +++ b/tests/legacy_decode.c @@ -0,0 +1,149 @@ +/* license:BSD-3-Clause + * copyright-holders:Romain Tisserand + * + * v1-v4 CHDs carry a CRC32 of each decoded hunk in their map entry, with a + * per-entry flag to opt out. This walks the fixtures tests/corpus/mklegacy.py + * builds: the intact pair must read and match the source data, the two whose + * stored hunk no longer matches the CRC the map advertises must be refused, + * and the one that sets the opt-out flag on that same hunk must read again. + * + * With VERIFY_BLOCK_CRC off nothing is checked, so the broken files are + * expected to read - the point there is only that they still decode. + */ +#include +#include +#include + +#include + +static unsigned char *slurp(const char *path, size_t *len) +{ + FILE *f = fopen(path, "rb"); + unsigned char *buf; + long n; + + if (f == NULL) + return NULL; + fseek(f, 0, SEEK_END); + n = ftell(f); + fseek(f, 0, SEEK_SET); + buf = (unsigned char *)malloc((size_t)n); + if (buf != NULL && fread(buf, 1, (size_t)n, f) != (size_t)n) + { + free(buf); + buf = NULL; + } + fclose(f); + *len = (size_t)n; + return buf; +} + +/* Reads every hunk. Returns the first error, or CHDERR_NONE, and compares + * against expect when the whole file decoded. */ +static chd_error decode_all(const char *path, const unsigned char *expect, + size_t expectlen, int *mismatch) +{ + chd_file *chd = NULL; + const chd_header *header; + unsigned char *buf; + chd_error err; + uint32_t i; + + *mismatch = 0; + err = chd_open(path, CHD_OPEN_READ, NULL, &chd); + if (err != CHDERR_NONE) + return err; + + header = chd_get_header(chd); + buf = (unsigned char *)malloc(header->hunkbytes); + if (buf == NULL) + { + chd_close(chd); + return CHDERR_OUT_OF_MEMORY; + } + + for (i = 0; i < header->totalhunks; i++) + { + err = chd_read(chd, i, buf); + if (err != CHDERR_NONE) + break; + if (expect != NULL && (size_t)(i + 1) * header->hunkbytes <= expectlen && + memcmp(buf, expect + (size_t)i * header->hunkbytes, header->hunkbytes) != 0) + *mismatch = 1; + } + + free(buf); + chd_close(chd); + return err; +} + +int main(int argc, char **argv) +{ + static const char *const versions[] = { "v3", "v4" }; + unsigned char *raw; + size_t rawlen; + char path[1024]; + int failures = 0; + size_t v; + + if (argc < 2) + { + fprintf(stderr, "usage: %s \n", argv[0]); + return 2; + } + + snprintf(path, sizeof(path), "%s/plain.raw", argv[1]); + raw = slurp(path, &rawlen); + if (raw == NULL) + { + printf("legacy corpus not generated (%s) - skipping\n", argv[1]); + printf("run tests/corpus/mklegacy.py to build it\n"); + return 0; + } + + for (v = 0; v < sizeof(versions) / sizeof(versions[0]); v++) + { + struct { const char *name; chd_error want; int check; } cases[] = { + { "plain", CHDERR_NONE, 1 }, +#if VERIFY_BLOCK_CRC + { "badcomp", CHDERR_DECOMPRESSION_ERROR, 0 }, + { "badraw", CHDERR_DECOMPRESSION_ERROR, 0 }, +#else + { "badcomp", CHDERR_NONE, 0 }, + { "badraw", CHDERR_NONE, 0 }, +#endif + { "nocrc", CHDERR_NONE, 0 }, + }; + size_t c; + + for (c = 0; c < sizeof(cases) / sizeof(cases[0]); c++) + { + chd_error err; + int mismatch; + + snprintf(path, sizeof(path), "%s/%s_%s.chd", argv[1], + versions[v], cases[c].name); + err = decode_all(path, cases[c].check ? raw : NULL, rawlen, &mismatch); + if (err != cases[c].want) + { + fprintf(stderr, "%s_%s: got \"%s\", want \"%s\"\n", + versions[v], cases[c].name, chd_error_string(err), + chd_error_string(cases[c].want)); + failures++; + } + else if (mismatch) + { + fprintf(stderr, "%s_%s: decoded data differs from the source\n", + versions[v], cases[c].name); + failures++; + } + } + } + + free(raw); + if (failures != 0) + fprintf(stderr, "%d failure(s)\n", failures); + else + printf("legacy CRC32 checks ok\n"); + return failures != 0; +}