ESP32-P4: fix ROM miniz collision, three malformed-input UB fixes, faster CRC/ECC - #180
Open
rtissera wants to merge 14 commits into
Open
ESP32-P4: fix ROM miniz collision, three malformed-input UB fixes, faster CRC/ECC#180rtissera wants to merge 14 commits into
rtissera wants to merge 14 commits into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
absolute linker symbols outrank ours, so
tinfl_decompressbound to the ROMcopy 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.
repeated builds leaked subtables and eventually decoded against a stale one.
a
uint8_tpromoted tointand shifted by 24 in the header parser;unvalidated
lengthbits/selfbits/parentbitsfrom the v5 map headerreaching
bitstream_read()and shifting by a negative amount; and a refillshifting by 32 once a stream has been over-consumed. Found by fuzzing under
ASan/UBSan.
drflac__realloc_from_callbacks()shrinks a buffer and a discarded subframedecode result that let a failed frame report success.
CHDR_WANT_RAW_DATA_SECTOR=OFFwithCHDR_VERIFY_BLOCK_CRC=ONis nowrefused 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.
crc16slice-by-4. The byte-at-a-time loop is 12 instructions per byte onRV32 and runs over every decoded hunk under
VERIFY_BLOCK_CRC- about0.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.
ecclow[]is exactlyxtime()in GF(2^8) with poly 0x11d and
poffsets[r][c]is exactlyr + 86c, so the86 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):
Memory and API
chd_set_cache_budget()adds an opt-in read-ahead window and aself-reference cache, both off by default - how much memory is available
is a property of the embedding system, not of libchdr.
tinfldirectly with a non-wrapping output buffer,dropping the 32KB LZ dictionary
mz_inflateallocates per instance.LOWRAM_TARGET, cdzs shares one ~93KB zstd context between sector dataand subcode, which are never in flight at once.
CHDERR_OUT_OF_MEMORYinstead of beingindistinguishable from a corrupt stream.
VERIFY_BLOCK_CRCon, dr_flac's per-frame CRC is skipped since the hunkCRC 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
with
VERIFY_BLOCK_CRCchecking every hunk against chdman's own CRC.orders; across
LOWRAM_TARGETon/off, system zlib, system zstd and LTO; andbetween 32-bit and 64-bit builds.
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.
Notes for review
contrib/esp32p4/is a test harness, not library code, and can be droppedwithout 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.