cu_aligner: fix a panic on payload-less ticks and bound snapshot decoding - #3
Open
Scofield626 wants to merge 1 commit into
Open
cu_aligner: fix a panic on payload-less ticks and bound snapshot decoding#3Scofield626 wants to merge 1 commit into
Scofield626 wants to merge 1 commit into
Conversation
|
Hi! Thanks for opening this pull request. Because this is your first time contributing to this repository, please read our contributor guide: |
Scofield626
force-pushed
the
fuzz-cu-components
branch
from
August 4, 2026 12:48
41c0e63 to
9794de8
Compare
…ding Two defects found by fuzzing the task with libFuzzer under AddressSanitizer. 1. process() aborted on a message that carries a time-of-validity but no payload. Such a message is normal: a task that had nothing to emit on a tick still produces one, and sparse streams are the whole reason an aligner exists. It was pushed into the buffer unconditionally, iter_window selects on the tov alone, so it came back out and hit payload().unwrap(). A payload-less message carries no data to align, so it is no longer buffered: it should neither occupy a slot in the fixed-size buffer nor advance the alignment window. fill_from_iter now uses filter_map instead of unwrap as well, because TimeboundCircularBuffer::push is public and a caller can still push whatever it likes. 2. thaw() allocated whatever length a snapshot declared. The runtime decodes keyframes with bincode's NoLimit configuration, so a corrupted snapshot aborted the process with "capacity overflow" from raw_vec. The number of messages was already bounded against S; the size of each one was not. decode_buffered_msg now rejects a declared length above a documented per-message cap before allocating, reads the blob in 8 KiB chunks so a length the stream cannot satisfy fails on end-of-input, and gives the inner decode the same limit rather than NoLimit -- without that last part a field inside the snapshot declares its own length and the allocation is unbounded again. encode_buffered_msg refuses to write a snapshot above the cap, so freeze can never produce something thaw would reject. The wire format is unchanged: bincode encodes a Vec<u8> as a u64 varint length followed by the raw bytes, which is exactly what the chunked reader consumes. Adds three regression tests. Both fuzz reproducers now pass, and the aligner targets run clean: 27,745 runs of the task harness and 7,724,360 runs of the buffer harness with the byte limit removed, which used to OOM within seconds.
Scofield626
force-pushed
the
fuzz-cu-components
branch
from
August 4, 2026 13:00
9794de8 to
ae88acf
Compare
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.
Fixes two defects in
cu_aligner, both found by fuzzing the task with libFuzzerunder AddressSanitizer. The fuzz harnesses themselves are not part of this PR.
1. Panic on a timestamped message with no payload
A task that had nothing to emit on a tick still produces a message: it has a
Tovbutpayload == None. Sparse streams are the whole reason an alignerexists — aligning a 10 Hz lidar against a 100 Hz IMU means most ticks have
nothing on one input.
processpushed such a message into the buffer unconditionally.iter_windowselects on the
Tovalone, so the message came back out and hitpayload().unwrap(), aborting the process. A single tick is enough to triggerit: one input with a
Tovand no payload.A payload-less message carries no data to align, so it is no longer buffered — it
should neither occupy a slot in the fixed-size buffer nor advance the alignment
window.
fill_from_iteralso usesfilter_mapinstead ofunwrap, becauseTimeboundCircularBuffer::pushis public and a caller can still push anything.2. Unbounded allocation decoding a snapshot
thawbounded the number of buffered messages againstS, but not the size ofeach one — it called
Vec::<u8>::decode, which allocates whatever length thesnapshot declares. The runtime decodes keyframes with bincode's
NoLimitconfiguration, so a corrupted snapshot aborted the process with
capacity overflowfromraw_vec.decode_buffered_msgnow does three things:(
MAX_MSG_SNAPSHOT_BYTES, 256 MiB) before allocating anything;end-of-input having allocated one chunk rather than the full claim;
MAX_MSG_CLAIM_BYTES) instead ofNoLimit.This part matters: without it the fuzzer went straight back to
malloc(1862396544), because a field inside the snapshot declares its ownlength.
encode_buffered_msgrefuses to write a snapshot above the wire cap, sofreezecannot emit a blob longer than
thawwill read.Caveat on the two caps. They are deliberately different, and not
interchangeable.
bincode's limit counts claimed bytes, not wire bytes:decoding a container claims
len * size_of::<T>(), so aVec<u64>of smallvarints claims roughly eight times what it occupies on the wire.
bincodehas noencoder-side limit to make the two agree, so
MAX_MSG_CLAIM_BYTESis a multipleof the wire cap and a payload holding an enormous collection of multi-byte
elements could in principle be written and then refused on the way back in. Both
caps sit far above any realistic buffered message. This is documented on the
constants.
The wire format is unchanged. Bincode encodes a
Vec<u8>as au64varintlength followed by the raw bytes, which is byte for byte what the chunked reader
consumes.
Residual: a snapshot can still force one allocation up to the claim budget. Bincode's
limit is a const generic, so it cannot be set to the blob's actual length. The
real fix is in the runtime, which should not decode keyframes with
NoLimitatall — the same root cause makes
CuArrayVec::decode(
core/cu29_runtime/src/payload.rs:143) turn 8 bytes of log into a 5 GBallocation, which this PR does not touch.
Verification
cargo test -p cu-aligner— 9 passed, including three new regression tests:test_aligner_tolerates_payload_less_ticks,thaw_rejects_a_bogus_message_length(both the above-cap and the truncatedcase) and
freeze_thaw_round_trips. All three fail against the unfixed code.cargo clippy -p cu-aligner --all-targets— clean.the buffer harness with the byte limit removed, both clean. The latter used to
OOM within seconds.
examples/cu_image_alignerdoes not build, on this branch and on master alike —a pre-existing missing
TypePathimpl for tuples ofCuArray<CuImage<..>>,unrelated to these changes.
One design call worth a second opinion
Dropping payload-less messages means a stream that only sends empty ticks leaves
its buffer empty, and
get_latest_aligned_datatakes.flatten().min()over theper-buffer
most_recent_time()—flatten()dropsNone, so an empty bufferstops constraining the window rather than blocking alignment. The task then emits
an output whose array for that stream is empty.
I kept that behavior rather than blocking, because an empty array is already a
normal outcome of this design:
iter_windowselects on time, so a stream whosedata all falls outside the window yields nothing even when every message it sent
carried a payload. I verified that directly — two streams, both with payloads,
one far ahead of the other, produces an empty array for one of them on today's
code. Consumers must already handle empty arrays.
The alternative is to require every buffer to be non-empty and return no output
otherwise. That is a broader semantic change than this bug fix — it would also
change startup and any slow stream whose data ages out — so I left it alone.
Happy to switch if you would rather the task stayed silent than emitted a partial
alignment.