btp: decode chunk headers from unaligned buffers - #79
Open
eastagiletracker wants to merge 1 commit into
Open
Conversation
Chunk::decode read the header with bytemuck::try_from_bytes, which requires the input slice to meet Header's 2-byte alignment. A receiver that passes a sub-slice of a larger packet buffer (an ATT payload sitting at +3, for instance) hands us an odd address, so every otherwise valid chunk was rejected with DecodeError::InvalidHeader and the message never reassembled. Read the header by value with try_pod_read_unaligned instead. Header is Copy and was already copied into the Chunk, so aligned input behaves exactly as before.
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.
This PR proposes fixing
btp::Chunk::decode, which rejects perfectly valid chunks when the caller hands it an unaligned buffer. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/326. You can sign in with your GitHub ID to claim ownership of the project.The defect
Header::from_bytesreads the 8-byte BTP header withbytemuck::try_from_bytes, which is a reference cast and therefore requires the input slice to satisfyHeader's 2-byte alignment.Chunk::decodepasses&data[..HEADER_SIZE]straight through, so whether a chunk decodes depends on the address of the caller's buffer rather than on its contents. A receive path that hands the decoder a sub-slice of a larger packet buffer — say an application payload sitting at offset 3 of anATT_MTUpacket,ATT_MTUbeingAPP_MTU + 3in theconstscrate — lands on an odd address, and then every chunk it delivers is rejected withDecodeError::InvalidHeader. Nothing about those bytes is malformed, so the peer keeps sending,Dechunkernever fills, and the transfer stalls with no useful error. The existing tests miss it because they always decode from the start of a fresh allocation, which is aligned.Reproduced on
mainat a0615c6, by copying a chunk produced bychunk()into a buffer at an odd offset and decoding that sub-slice:The change
Header::from_bytesnow reads the header by value withbytemuck::try_pod_read_unalignedand returnsOption<Header>;Chunk::decodedrops the matching deref and moves the header into theChunk.HeaderisPodandCopyand was already being copied into theChunk, so aligned input decodes byte-for-byte as it did before, and the size validation is unchanged — a slice that is not exactlyHEADER_SIZEstill yieldsDecodeError::InvalidHeader.from_bytesis private, so no public signature moves; the diff outside the tests is two lines.Verification, all on the branch tree: the two regression tests above fail on
mainand pass with the change, covering bothChunk::decodedirectly and a full multi-chunk message reassembled throughDechunker::receiveon odd-addressed slices.cargo test --workspace --all-featureswas run on a clean checkout before the change and again after — 112 tests passing before, 114 after (the two new ones), no test that passed before fails now.cargo fmt --all -- --checkandcargo clippy --all-targets --all-features -- -D warningsare both clean, matching the checks in your Rust workflow. Nothing inapi,backup-shardorquantum-link-macrosis touched, so this should not conflict with the QLV2 work in flight.How this was managed
We tracked this work on a live board imported from this repository's own pull requests and labels — 78 stories, one per pull request, plus the label they carry — and the fix itself is this story:
If you'd rather not receive contributions like this, reply
no-more-prson this pull request and we won't open any further ones on your repositories.Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com