From d31d8b9c7acda69936fa484750a153dc78f19747 Mon Sep 17 00:00:00 2001 From: Romain TISSERAND Date: Thu, 10 Sep 2026 21:39:19 +0200 Subject: [PATCH] Stop rejecting sparse CHDv5 images 5cc52fd bounded totalhunks by the file's own size in bits, on the reasoning that "every hunk map entry consumes at least one bit in the compressed on-disk map". That is not true of CHDv5: its map is run-length and Huffman coded, so a run of identical, uncompressed or self-referenced hunks costs far less than one bit each. 64 MiB of zeros becomes a 203-byte file carrying 16384 hunks - eighty hunks per byte where the check allowed eight - and chd_open() refused it while chdman verified it. Reported in #190 against a 50 GB image of about 23 KB, and the better an image compresses the more certainly it is rejected. The guard is still needed: totalhunks sizes the map allocation, and a malformed header could ask for gigabytes against a tiny file. Replaced with a bound on each version's actual constraint. v1-v4 have an exact one. Their map is a plain array of fixed-size entries stored immediately after the header, so header->length + totalhunks * entrysize has to fit in the file. Tighter than the old check, and true. v5 has no such invariant, so bound what is really allocated instead: map_size_v5() now caps the materialized map at CHD_MAX_MAP_SIZE. 256 MiB covers over 400 GB at CD geometry and about 89 GB at 4 KB hunks, which is past anything this library claims to handle, while keeping a corrupt hunkcount from requesting more than a host would give. LOWRAM_TARGET never materializes the map at all and is unaffected. tests/sparse_map.c opens the seed and decodes every hunk; it fails before this change with "invalid data". The seed comes from generate.sh, built with chdman from a zero-filled file, so it needs no third-party data. Checked in six build configurations - default, LOWRAM_TARGET, CD_SCRATCH_BUFFER off, system zlib, the no-subcode/no-CRC combination, and LOWRAM with system zlib. All accept the sparse image and all still reject three hand-corrupted headers (v5 logicalbytes at 1 TiB and at INT64_MAX, v4 totalhunks at 2^30) under a hard 300 MB address-space limit, so nothing slipped through by allocating first. Decoded output is unchanged across the corpus - 36 files, no difference in data or error codes - and ASan/UBSan is clean on the targeted cases and the mutant set. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01846EhHuAFk5qvxwEA5Gq6y --- src/libchdr_chd.c | 44 +++++++++++++++++++----- tests/CMakeLists.txt | 9 +++++ tests/corpus/generate.sh | 10 ++++++ tests/sparse_map.c | 73 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 128 insertions(+), 8 deletions(-) create mode 100644 tests/sparse_map.c diff --git a/src/libchdr_chd.c b/src/libchdr_chd.c index b5981c2..6be528d 100644 --- a/src/libchdr_chd.c +++ b/src/libchdr_chd.c @@ -108,6 +108,10 @@ * (#147). */ #define CHD_MAX_FILE_SIZE (1024ULL * 1024 * 1024 * 1024) +/* Ceiling on a materialized CHDv5 hunk map. Not a format limit - it only + * bounds one allocation. LOWRAM_TARGET never materializes the map at all. */ +#define CHD_MAX_MAP_SIZE (256ULL * 1024 * 1024) + #define COOKIE_VALUE 0xbaadf00d #define END_OF_LIST_COOKIE "EndOfListCookie" @@ -861,10 +865,22 @@ static CHDR_INLINE int map_size_v5(chd_header* header, size_t *size) { /* Avoid overflow due to corrupted data. */ const size_t max_hunkcount = ((size_t)-1 / header->mapentrybytes); + uint64_t want; + if (header->hunkcount > max_hunkcount) return FALSE; - *size = (size_t)header->hunkcount * header->mapentrybytes; + /* A compressed v5 map is entropy-coded, so its on-disk size says nothing + * about how many hunks it holds and cannot be used to bound this. Cap the + * decompressed map directly: CHD_MAX_MAP_SIZE covers every image size + * this library claims to support (over 400GB at CD geometry, ~89GB at + * 4KB hunks) while keeping a malformed hunkcount from asking for more + * memory than any host would hand out. */ + want = (uint64_t)header->hunkcount * header->mapentrybytes; + if (want > CHD_MAX_MAP_SIZE) + return FALSE; + + *size = (size_t)want; return TRUE; } @@ -2945,13 +2961,25 @@ static chd_error header_read(chd_file *chd) if (header->hunkbytes >= CHD_MAX_HUNK_SIZE || ((uint64_t)header->hunkbytes * (uint64_t)header->totalhunks) >= CHD_MAX_FILE_SIZE) return CHDERR_INVALID_DATA; - /* totalhunks is used to size the map allocation; a malformed header - * can otherwise request multi-GB allocations for map[] even when the - * file itself is tiny. Every hunk map entry consumes at least one bit - * in the compressed on-disk map, so totalhunks cannot legitimately - * exceed file_size * 8. */ - if ((uint64_t)header->totalhunks > chd->file_size * 8) - return CHDERR_INVALID_DATA; + /* totalhunks sizes the map allocation, so a malformed header could + * otherwise ask for multi-GB of map[] against a tiny file. + * + * v1-v4 give an exact bound: their map is a plain array of fixed-size + * entries stored right after the header, so it has to fit in the file. + * v5 gets no equivalent - its map is run-length and Huffman coded, and a + * run of identical, uncompressed or self-referenced hunks costs far less + * than one bit each, so a valid sparse image really can carry far more + * hunks than its own size in bits. Bound what is actually allocated + * instead; see map_size_v5(). */ + if (header->version < 5) + { + uint64_t entrysize = (header->version < 3) ? OLD_MAP_ENTRY_SIZE : MAP_ENTRY_SIZE; + uint64_t mapend = (uint64_t)header->length + + (uint64_t)header->totalhunks * entrysize; + + if (mapend < (uint64_t)header->length || mapend > chd->file_size) + return CHDERR_INVALID_DATA; + } /* guess it worked */ return CHDERR_NONE; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8e6557b..f025e5d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -52,6 +52,15 @@ chdr_test_defines(chdr-legacy-decode) add_test(NAME legacy-decode COMMAND chdr-legacy-decode "${CMAKE_CURRENT_SOURCE_DIR}/corpus/legacy") +# A sparse CHDv5 whose map carries more hunks than the file has bits. Its own +# test rather than a seeds/ walk, because what is under test is chd_open() +# accepting it at all - a header check can reject it before any decode. +add_executable(chdr-sparse-map sparse_map.c) +target_link_libraries(chdr-sparse-map PRIVATE chdr-static) +chdr_test_defines(chdr-sparse-map) +add_test(NAME sparse-map + COMMAND chdr-sparse-map "${CMAKE_CURRENT_SOURCE_DIR}/corpus/seeds/rle_sparse.chd") + # 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/generate.sh b/tests/corpus/generate.sh index c9985b0..f2d83b6 100755 --- a/tests/corpus/generate.sh +++ b/tests/corpus/generate.sh @@ -82,6 +82,16 @@ create_hd hd_multi.chd -c zlib,lzma,huff,zstd # decoder holds the largest per-instance buffer of any codec here. create_hd hd_flac.chd -c flac +# A CHDv5 whose map compresses far below one bit per hunk: 64 MiB of zeros +# becomes a couple of hundred bytes carrying 16384 hunks, because a run of +# identical hunks costs almost nothing in the run-length and Huffman coded v5 +# map. Any header check that assumes a lower bound on map size per hunk +# rejects it - the regression case for issue #190. Its own file, since the +# shared RAW_HD is far too small to produce a run worth compressing. +dd if=/dev/zero of="$TMP/sparse.raw" bs=1M count=64 status=none +chdman createraw -f -o "$CORPUS/rle_sparse.chd" -i "$TMP/sparse.raw" \ + -hs 4096 -us 2048 -c zlib >/dev/null 2>&1 || true + # CD-ROM: default + per-codec. create_cd cd_default.chd create_cd cd_none.chd -c none diff --git a/tests/sparse_map.c b/tests/sparse_map.c new file mode 100644 index 0000000..414be56 --- /dev/null +++ b/tests/sparse_map.c @@ -0,0 +1,73 @@ +/* license:BSD-3-Clause + * copyright-holders:Romain Tisserand + * + * A CHDv5 map is run-length and Huffman coded, so a run of identical hunks + * costs far less than one bit each: 64 MiB of zeros compresses to a couple of + * hundred bytes carrying 16384 hunks. Header sanity checks that bound + * totalhunks by the file's own size in bits therefore reject perfectly valid + * sparse images - issue #190, where a 50 GB image of ~23 KB was refused. + * + * Skips when the corpus is absent, as the other corpus tests do. + */ +#include +#include + +#include + +int main(int argc, char **argv) +{ + const char *path = (argc > 1) ? argv[1] : "tests/corpus/seeds/rle_sparse.chd"; + chd_file *chd = NULL; + const chd_header *header; + unsigned char *buf; + chd_error err; + uint32_t i; + FILE *probe; + + probe = fopen(path, "rb"); + if (probe == NULL) + { + printf("sparse seed not generated (%s) - skipping\n", path); + printf("run tests/corpus/generate.sh to build it\n"); + return 0; + } + fclose(probe); + + err = chd_open(path, CHD_OPEN_READ, NULL, &chd); + if (err != CHDERR_NONE) + { + fprintf(stderr, "open failed: %s\n", chd_error_string(err)); + fprintf(stderr, "a sparse v5 map holds more hunks than the file has bits;" + " a header check must not assume otherwise\n"); + return 1; + } + + header = chd_get_header(chd); + printf("sparse map: %u hunks, %u hunkbytes\n", + (unsigned)header->totalhunks, (unsigned)header->hunkbytes); + + buf = (unsigned char *)malloc(header->hunkbytes); + if (buf == NULL) + { + chd_close(chd); + return 1; + } + + /* every hunk has to decode, not just the header parse */ + for (i = 0; i < header->totalhunks; i++) + { + err = chd_read(chd, i, buf); + if (err != CHDERR_NONE) + { + fprintf(stderr, "hunk %u: %s\n", i, chd_error_string(err)); + free(buf); + chd_close(chd); + return 1; + } + } + + printf("all %u hunks decoded\n", (unsigned)header->totalhunks); + free(buf); + chd_close(chd); + return 0; +}