Verify the hunk CRC32 that v3/v4 map entries carry

.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

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
 )
 

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,13 +3155,23 @@ 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 */
 			case V34_MAP_ENTRY_TYPE_MINI:
 				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 */

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

tests/corpus/.gitignore

@@ -3,3 +3,4 @@
 !README.md
 !generate.sh
 !fetch.sh
+!mklegacy.py

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

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 '.')