Skip to content

ESP32-P4: fix ROM miniz collision, three malformed-input UB fixes, faster CRC/ECC - #180

Open
rtissera wants to merge 14 commits into
masterfrom
esp32p4-clean
Open

ESP32-P4: fix ROM miniz collision, three malformed-input UB fixes, faster CRC/ECC#180
rtissera wants to merge 14 commits into
masterfrom
esp32p4-clean

Conversation

@rtissera

@rtissera rtissera commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Work from bringing libchdr up on a Waveshare ESP32-P4-NANO with a real SD card.
Three correctness fixes that apply everywhere, two decode optimisations that
apply everywhere, and a hardware benchmark harness under contrib/.

Everything here was measured on the board or found by fuzzing; nothing is
speculative.

Correctness

  • ESP32 ROM miniz symbol collision. ESP ROMs export an older miniz whose
    absolute linker symbols outrank ours, so tinfl_decompress bound to the ROM
    copy and libchdr's zlib/cdzl decoding produced garbage. Verified with a
    direct link test: the ROM address wins and miniz's real decoder is never
    pulled from the archive. Renamed from the build glue, not by patching the
    vendored source.
  • Huffman subtable arena was never reset between lookup-table rebuilds, so
    repeated builds leaked subtables and eventually decoded against a stale one.
  • Three undefined shifts reachable from a malformed CHD, all pre-existing:
    a uint8_t promoted to int and shifted by 24 in the header parser;
    unvalidated lengthbits/selfbits/parentbits from the v5 map header
    reaching bitstream_read() and shifting by a negative amount; and a refill
    shifting by 32 once a stream has been over-consumed. Found by fuzzing under
    ASan/UBSan.
  • dr_flac 0.13.4, which fixes a heap overflow when
    drflac__realloc_from_callbacks() shrinks a buffer and a discarded subframe
    decode result that let a failed frame report success.
  • CHDR_WANT_RAW_DATA_SECTOR=OFF with CHDR_VERIFY_BLOCK_CRC=ON is now
    refused at configure time. The stored CRC covers the reconstituted hunk, so
    the combination can never verify, and it failed content-dependently - a
    hunk holding only audio frames has no ECC to regenerate and still passes -
    which read as sporadic file corruption.

Performance

Both are portable C, no intrinsics, no target-specific paths.

  • crc16 slice-by-4. The byte-at-a-time loop is 12 instructions per byte on
    RV32 and runs over every decoded hunk under VERIFY_BLOCK_CRC - about
    0.76 ms per 19584-byte hunk on the P4, more than the zstd decode it was
    checking. Slice-by-4 takes it to 6.25, for 1536 bytes of extra tables.
  • ECC P parity, four rows per 32-bit word. ecclow[] is exactly xtime()
    in GF(2^8) with poly 0x11d and poffsets[r][c] is exactly r + 86c, so the
    86 independent P rows pack into one accumulator. This is SWAR, not SIMD.

Measured, same 14 files, same configuration, uncapped over 7.32 GB:
2307 s -> 1925 s, 3.17 -> 3.80 MB/s. Per hunk on identical content,
5.087 -> 4.168 ms.

Measured elsewhere (aarch64 under qemu, so ratios rather than absolute times):

x86-64 -O2 x86-64 -O3 aarch64 -O2 aarch64 -O3 RV32
crc16 slice-4 3.55x 3.52x 1.97x 2.42x 1.9x
ECC P SWAR 2.54x 2.70x 4.41x 4.54x 1.47x

Memory and API

  • chd_set_cache_budget() adds an opt-in read-ahead window and a
    self-reference cache, both off by default - how much memory is available
    is a property of the embedding system, not of libchdr.
  • The zlib codec drives tinfl directly with a non-wrapping output buffer,
    dropping the 32KB LZ dictionary mz_inflate allocates per instance.
  • Under LOWRAM_TARGET, cdzs shares one ~93KB zstd context between sector data
    and subcode, which are never in flight at once.
  • FLAC allocation failures report CHDERR_OUT_OF_MEMORY instead of being
    indistinguishable from a corrupt stream.
  • With VERIFY_BLOCK_CRC on, dr_flac's per-frame CRC is skipped since the hunk
    CRC covers the same data - worth ~8% of a CD-FLAC hunk and 13KB of text on
    RV32. Detection is unchanged: over 25 corruption cases the builds with and
    without agree, with no corrupt output escaping.

Validation

  • Decoded output byte-identical over 287 CHDs against the pre-change build,
    with VERIFY_BLOCK_CRC checking every hunk against chdman's own CRC.
  • Identical across sequential, reverse, random and scattered-sample read
    orders; across LOWRAM_TARGET on/off, system zlib, system zstd and LTO; and
    between 32-bit and 64-bit builds.
  • 3412 malformed inputs (mutations of all 17 corpus seed codecs plus
    structure-aware header cases) clean under ASan and UBSan against both map
    implementations and both word sizes; 546 metadata-chain inputs including
    self-referential cycles; API misuse including the OOM path. No leaks.
  • AVHuff regression suite 4/4. Compiles clean for Cortex-M33 and Cortex-M0+.

Notes for review

contrib/esp32p4/ is a test harness, not library code, and can be dropped
without affecting anything else - the last two commits are the benchmark and
its documentation. Every commit builds on its own.

Supersedes #179, which carried the same work across 32 commits.

rtissera and others added 14 commits September 2, 2026 13:57
chd_get_metadata()'s faux hard-disk metadata snprintf() passed uint32_t
header fields against a %d format string (the paired sscanf use already
took int* correctly) - harmless on LP64 desktop builds but a real
-Werror=format= build failure on ILP32 targets. Cast at the call site;
the on-disk MAME metadata text format is untouched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KMYbZzB8mioFmotWGFnAXG
ESP ROMs export an older miniz whose absolute symbols outrank ours at link time; rename the colliding entry points from libchdr's build glue rather than patching the vendored source.
Uses tinfl with a non-wrapping output buffer, which drops the 32KB LZ dictionary that mz_inflate allocates per codec instance and never needs here.
The arena index was never cleared between rebuilds, so repeated builds leaked subtables and eventually decoded against a stale one.
flac_decoder_reset() allocates a fresh ~40KB decoder per hunk, so on small-RAM targets that, not the stream, is what fails.
…ARGET

ZSTD_DStream is a typedef for ZSTD_DCtx, so the two streams can use a single ~93KB context when they are never in flight at once.
Both are off unless the caller sets a budget with chd_set_cache_budget(), since how much memory is available is a property of the embedding system rather than of libchdr.
Keeps the accumulators in registers and computes P parity four rows at a time in one 32-bit word, using the fact that ecclow[] is exactly xtime() in GF(2^8); also refuses WANT_RAW_DATA_SECTOR=OFF with VERIFY_BLOCK_CRC=ON, which can never match the stored CRC.
The byte-at-a-time loop was 12 instructions per byte on RV32 and runs over every decoded hunk under VERIFY_BLOCK_CRC; slice-by-4 takes it to 6.25 for 1536 bytes of extra tables.
Fixes a heap overflow when drflac__realloc_from_callbacks() shrinks a buffer, a discarded subframe decode result that let a failed frame report success, and adds metadata bounds checks.
Gated on VERIFY_BLOCK_CRC, which is what makes it safe: a corrupt frame decodes to garbage that the hunk CRC then rejects with the same error.
Casts before shifting in get_bigendian_uint32_t(), rejects map bit widths above 32 before they reach bitstream_read(), and stops the refill shifting by 32 once a stream has been over-consumed.
Decodes a characterised CHD corpus from flash and SD on real hardware, reporting throughput, latency percentiles, CPU/IO split and heap use.
Records what was measured on hardware, what shipped, and the changes that look correct on paper but are losses in practice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant