Skip to content

Use checked arithmetic for frame header totals - #133

Open
cf-rhett wants to merge 1 commit into
jamesmunns:mainfrom
cf-rhett:checked-arith-on-main
Open

Use checked arithmetic for frame header totals#133
cf-rhett wants to merge 1 commit into
jamesmunns:mainfrom
cf-rhett:checked-arith-on-main

Conversation

@cf-rhett

Copy link
Copy Markdown

FramedProducer::grant sizes its request as sz + size_of::<H>(), and FramedConsumer::read validates a frame with size_of::<H>() + hdr <= grant_len. Both sums are unchecked. With H = u16 neither can overflow; with H = usize both can.

On the write side, a wrapped total asks the coordinator for a handful of bytes while the returned FramedGrantW still carries the caller's original sz in hdr — which is the length Deref/DerefMut pass to slice::from_raw_parts. The grant hands out a slice far larger than the allocation it covers.

On the read side the wrap lands under grant_len, satisfying the very check that exists to reject an inconsistent header. FramedGrantR::Deref then builds a slice from the unwrapped header value.

Debug builds catch both as an overflow panic. Release builds do not.

Both call sites already return Result, so this needs no API change. Two regression tests are included; both fail without the fix.

`FramedProducer::grant` sizes its request as `sz + size_of::<H>()` and
`FramedConsumer::read` validates a frame as `size_of::<H>() + hdr <= grant_len`.
Both sums are unchecked. `H = u16` cannot overflow either one, but `H = usize`
can, and the two failure modes are worse than a rejected grant.

On the write side the wrapped total asks the coordinator for a handful of bytes
while the returned grant still carries the caller's original `sz` in its header,
which is the length `Deref`/`DerefMut` hand to `slice::from_raw_parts`. On the
read side the wrap lands under `grant_len` and so satisfies the very check that
exists to reject an inconsistent header.

Debug builds catch both as an overflow panic; release builds do not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant