fix: fallible alloc helpers + incremental ICC profile cap#141
Merged
Conversation
…iant Step-0 follow-up to per_class_signal_probe.rs (commit 2423a8a). The prior probe identified a real screen-vs-photo at-q ΔBA asymmetry in the bottom-right 4×4 HF block, but the perturbation tested (force quant=1, "preserve all HF energy") was the wrong direction — it spent +37% to +283% bytes to gain at-q quality, losing badly at matched bytes. This variant tests the OPPOSITE perturbation per the prior report's explicit Step-0 recommendation: multiply the bottom-right 4×4 quant entries (luma + Cb + Cr) by 2x / 4x / 8x, clamp to [1, 255]. Hypothesis under test: screens have low HF AC energy, so coarsening HF should be ~free for screens and damaging for photos. If true, that's the asymmetric-cost signal the brief required to justify 6-10 hr per-class SA + cloud budget. Result (840 cells across 4 clusters x 3 variants x 3 q anchors): gate fails on every cell. Photo q=85 Bp_8x ΔBA +0.264; screen q=85 Bp_8x ΔBA +0.757 — screens degrade 3x more than photos under the same perturbation. The directional hypothesis is falsified. Companion benchmark report: zenanalyze/benchmarks/per_class_signal_probe_bprime_2026-05-04.md
extract_icc_profile previously accumulated every APP2/ICC chunk into a Vec<(u8, Vec<u8>)> BEFORE checking the cumulative size against MAX_ICC_PROFILE_SIZE. A JPEG with many APP2 chunks (each individually under u16::MAX bytes) could amplify peak memory to roughly the input size before the post-loop cap fired. Move the size check inside the loop so the running total is checked before each chunk is copied. Use try_reserve_exact for the final concatenation buffer. Adds a regression test that constructs ~2x MAX_ICC_PROFILE_SIZE worth of valid APP2/ICC chunks and asserts the function returns None.
The decoder was relying on try_alloc_maybeuninit, try_alloc_zeroed_bytes, and try_alloc_dct_blocks for buffers sized by attacker-controlled (but dimension-validated) image dimensions. Their bodies called vec![T::default(); n] / vec![0u8; n] / vec![[0i16;64]; n] directly, which panic on OOM. The docstrings acknowledged this and described it as acceptable because dimensions had been validated — but dimension validation gates pixel count, not bytes available on the host. A malicious 100 MP grayscale JPEG decoded as f32 RGBA on a memory-limited host (cgroup, container, low-RAM target) would panic the process. Switch all three helpers to Vec::try_reserve_exact + resize. For zero- default types (u8, i16, f32, [i16;64]) the resize step is internally specialized to alloc_zeroed/calloc, so on Linux the kernel still hands out lazy-committed zero-mapped pages without the per-page memset cost on the happy path. OOM is now reported as Error::AllocationFailed instead of unwinding/aborting. H3: replace the remaining vec![64u8; n] and vec![0u64; n] sites in scan.rs / progressive.rs (parallel storage for coefficient counts and nonzero bitmaps) with try_alloc_filled. Without this, a successful try_alloc_dct_blocks (which can succeed via lazy zero pages) followed by an immediately-infallible vec! could still panic at physical commit. Also tighten decode/parser/output.rs: every plain vec![0u8; n] and vec![0.0f32; n] sized by the (validated, but possibly very large) image dimensions is now go through try_alloc_zeroed / try_alloc_zeroed_f32 so OOM propagates as an error.
DecoderConfig::max_memory was stored on the config but never compared against any allocation. Setting `max_memory(64 * 1024 * 1024)` provided zero protection — a 100 MP JPEG decoded as RGBA on a memory-pressured host could panic before any check fired. Wire enforcement into the parser via two new fields, max_memory and max_memory_output_bpp, set by the public Decoder entry points (decode, decode_into, decode_coefficients, decode_coefficients_with_extras, decode_to_ycbcr_f32, scanline_reader). The check fires inside JpegParser::decode() right after read_header() populates dimensions, but before any large allocation runs. Doing the check there (rather than calling read_header from Decoder before parser.decode) means APP markers and preserved metadata (XMP, EXIF, ICC) are only parsed once — calling read_header twice would duplicate extras accumulation and break extended-XMP round-trip tests. Estimation is conservative: max(strip + output, coeff_storage + output) where output uses the configured pixel format's bytes-per-pixel. On exceedance, return Error::AllocationFailed before allocating. Adds three regression tests in decode_api_guide.rs: - max_memory_rejects_obviously_oversize_decode - max_memory_rejects_decode_into - max_memory_zero_is_unlimited Updates docs/SECURITY_ISSUES.md to reflect actual enforcement state (was claiming "Fixed" — now correctly documents the partial state and the dual line of defense with the fallible allocation helpers).
Member
Author
|
@lilith — ready for review. Part of the 2026-05-06 security audit campaign (see /home/lilith/work/feedback/security-audit-2026-05-06/FIX-RESULTS.md and REVIEW-RESULTS.md). |
max_input_bytes + max_pixels already bound JPEG decode memory transitively. Internal allocations are O(input_size) or O(pixels), so MemoryTracker bookkeeping is redundant with the entry-point caps that already fire. This reverts commit 5c6608a. Keeps H2/H3 (fallible try_alloc_* helpers — replace infallible vec! that panicked on OOM) and H4 (incremental ICC cap — real DoS fix).
Member
Author
|
Split: H1 (MemoryTracker parser-side wiring) reverted as YAGNI.
Still merging:
The |
lilith
changed the title
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
1 participant
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.
Hardening based on internal security audit (
/home/lilith/work/feedback/security-audit-2026-05-06/zenjpeg.md). Addresses HIGH findings H2/H3/H4. H1 (MemoryTracker parser-side wiring) was reverted as YAGNI —Limits::max_input_bytes+max_pixelsalready bound JPEG decode memory transitively via the entry-point gates that already fire.Changes
H4 (42b152c):
extract_icc_profilenow checks the 16 MB cap incrementally againstrunning_total + chunk_lenbefore copying each chunk. Previously the full set was accumulated, then capped — peak was 2× cap. The cap fires before any chunk that would exceed it. Added regression test that builds 2× the cap of valid APP2 chunks and assertsNone.H2/H3 (01a165c):
try_alloc_maybeuninit,try_alloc_zeroed_bytes,try_alloc_dct_blocksnow useVec::try_reserve_exact+resize(wasvec![T::default(); n]which panics on OOM). The Linux lazy-zero-page fast path is preserved byresizeon already-allocated capacity. Replaced barevec!withtry_alloc_filledforcoeff_countsand nonzero bitmaps in scan.rs/progressive.rs.revert (81a69bf): drops the H1 MemoryTracker parser-side wiring as YAGNI. The
MemoryTrackerstruct itself remains insrc/decoder/foundation/alloc.rsas dormant code; reverting this revert is one commit if a future need emerges.Tests
cargo test --lib: 951 passed.cargo test --doc: 10 passed. CI green on 18/18 platforms (windows-11-arm, macos-26-intel, i686, wasm32-wasip1).Deferred
MEDIUM and LOW findings deferred to follow-up.