Fix apultra safe_dist under-reporting for multi-block inputs#898
Open
meeq wants to merge 1 commit into
Open
Conversation
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.
Problem
For inputs larger than
BLOCK_SIZE(1 MiB),apultra_compresscan write astats.safe_distthat is too small for safe in-place decompression. Consumers that size their in-place buffer fromsafe_dist(e.g.,bufsize = orig_size + (safe_dist + cmp_size - orig_size)) will see the decoder's output pointer overtake the unread compressed input, corrupting subsequent reads.The bitstream itself is valid — streaming/non-in-place decoders decode correctly. Only in-place decoders are affected, and only on multi-block inputs.
Symptom
libdragon's
mkasset -c 2(vendored apultra) on a 2,282,582-byte input producedcmp_size = 1,621,797,safe_dist = 660,785(i.e. emittedinplace_margin = 0). libdragon's in-place asm decoder returned at byte 1,619,454 (premature accidental EOS in corrupted-input territory). Traced byte-exact decode showed the actual worst-case (output_pos − input_pos) reached ~672,860 (safe_distwas under-reported by ~12 KB).Root cause
In apultra_write_block:
i - nStartOffsetis bytes consumed from the current block's input.nOutOffsetis bytes written to the current block's output (reset to 0 at the top ofapultra_write_block;pOutDatais already advanced by the caller).So
nCurSafeDistmeasures the per-block local lead of output over input. For a single-block compression the local lead equals the global lead and the bug is invisible. For multi-block compression the local lead resets to 0 at each block boundary but the global lead, which is what an in-place decoder actually experiences, does not. The accumulated lead from earlier blocks is lost.Concretely, if block 0 finishes with
delta_0_end = block_0_input − block_0_outputbytes of lead, then during block 1 the actual global_delta isdelta_0_end + local_block_1_delta.stats.safe_distmisses thedelta_0_endterm and any later additions.Fix
Thread nPriorDelta — the cumulative (decoder-output − decoder-input) at block start — through
apultra_compressor_shrink_block→apultra_optimize_and_write_block→apultra_write_block, and add it tonCurSafeDist. The top-level loop inapultra_compressalready has the cumulativenOriginalSizeandnCompressedSize, so the value is just:Dictionary bytes are not emitted by the decoder, so they're excluded from the input side.
Also submitted upstream as emmanuel-marty/apultra#26