Decoder hardening: overflow, arena, and codebook fixes#16
Merged
Conversation
A crafted setup header can point up to 64 modes * 16 submaps at a single residue whose decodemap is hundreds of MB. Each per-mode term fits a long, but their sum can exceed 2^31 and wrap a 32-bit long on the ESP32 target to a small value. _ogg_malloc then succeeds undersized and res0_look's unchecked arena allocations write past the buffer. Sum the size in ogg_int64_t and reject any header whose total exceeds DSP_ARENA_MAX_BYTES (2^31-1, the largest arena a long offset can address) through the existing -1 init-failure path, before the allocation.
mapping0_inverse calls the residue inverse once per submap and res2_inverse allocates its partword array each call, all live within one packet (the block arena resets only between packets). The sizing reserved just one max_partwords_2 array. When info->end is the binding cap, each res2 submap allocates the full array rather than a channel-scaled share, so the reservation undercounts by up to the submap count and _vorbis_block_alloc returns NULL, which res012.c dereferences unchecked. Scale the res2 term by min(channels, 16), the most res2 submaps that can be live at once (each needs >=1 channel; submaps is a 4-bit field).
oggpack_read returns its -1 EOP sentinel on a truncated identification packet, so 1<<oggpack_read(opb,4) evaluated 1<<-1 (a negative shift count, UB) before the existing EOP check could fire. Read both blocksize nibbles into locals and reject a negative value before shifting, matching the rangebits guard in floor1.c. Only reachable through the exported vorbis_synthesis_headerin API; the C++ wrapper's 30-byte id-header minimum keeps in-tree paths clear of it.
decode_packed_entry_number chases the decode tree through dec_maxlength bits and, on a fall-off (no leaf reached), returned the 0xffffffff sentinel while trusting oggpack_adv(b, read+1) to trip end-of-packet. That advance's overflow check is byte-granular, so slack in the final partial byte could leave EOP unset, letting the sentinel reach decode_map_apply past its oggpack_eop guard: garbage output for dec_type 1 and an out-of-bounds q_val read for dec_type 2 (quantvals not a power of two). Only a one-used-entry book reaches this path, and only on corrupt input. Set the overrun sentinel directly (oggpack_seteop) so every caller's EOP check rejects the vector, covering all dec_types and the scalar vorbis_book_decode path in one place.
entries==0 is a legal 24-bit codebook field, and _make_decode_table already handles 0-sized books. But the two lengthlist allocations and the maptype-2 dec_type-3 q_val allocation size themselves from entries/used_entries, so a zero-entry book requests 0 bytes. heap_caps_malloc(0) returns NULL on ESP-IDF (glibc returns non-NULL), so the if(!ptr) checks rejected such a book on-target while the host decoded it. Clamp the three sizes to at least one byte; the read/pack loops that follow are empty when the count is zero.
There was a problem hiding this comment.
Pull request overview
This PR hardens the forked Tremor Vorbis decoder against crafted/truncated input by addressing undefined behavior, malformed codebook decoding, and arena-sizing corner cases, with corresponding documentation updates in src/tremor/CHANGES.md.
Changes:
- Prevents UB in
_vorbis_unpack_infoby rejecting truncated headers before shifting blocksize nibbles. - Hardens codebook unpack/decoding by (a) forcing EOP on decode-tree fall-through and (b) allowing legal zero-entry codebooks on ESP-IDF by avoiding 0-byte allocations.
- Fixes arena sizing: accounts for multiple res2 submaps in the block arena and computes DSP setup arena totals in 64-bit with an enforced cap.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/tremor/info.c | Rejects truncated id headers before shifting to avoid negative-shift UB. |
| src/tremor/codebook.c | Adds explicit EOP forcing on decode-tree fall-through and clamps allocations for zero-entry codebooks on ESP-IDF. |
| src/tremor/block.c | Adjusts arena sizing (res2 multi-submap) and moves DSP setup arena sizing to 64-bit with a maximum cap. |
| src/tremor/CHANGES.md | Documents the hardening fixes and related behavioral details. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Five correctness/memory-safety fixes in the forked Tremor decoder, each hardening a path reachable only through crafted or corrupt input. All touch only
src/tremor/and are documented inCHANGES.md._ogg_mallocsucceeded undersized andres0_lookwrote past the buffer. Sums inogg_int64_tand rejects totals overDSP_ARENA_MAX_BYTESbefore allocating.res2_inverseallocates its partword array per submap, all live within one packet, but the sizing reserved only one array. Wheninfo->endis the binding cap, the arena undercounts and_vorbis_block_allocreturnsNULL(dereferenced unchecked). Scales theres2term bymin(channels, 16)._vorbis_unpack_info:oggpack_readreturns its −1 EOP sentinel on a truncated packet, so1 << oggpack_read(...)evaluated1 << -1(UB) before the EOP check could fire. Reads the blocksize nibbles into locals and rejects a negative value before shifting.0xffffffffsentinel while trusting a byte-granularoggpack_advto trip end-of-packet; slack in the final partial byte could leave EOP unset, letting the sentinel reachdecode_map_apply(garbage output / OOBq_valread). Sets the overrun sentinel directly so every caller's EOP check rejects the vector.entries == 0is a legal field, but thelengthlist/q_valallocations sized from it request 0 bytes;heap_caps_malloc(0)returnsNULLon ESP-IDF (non-NULLon glibc), so the target rejected books the host decoded. Clamps the three sizes to at least one byte.