Merge rust-bitcoin/rust-bitcoin#6946: Fix integer overflow in `get_array`
What the developer wrote
Authored by Andrew Poelstra
Merge rust-bitcoin/rust-bitcoin#6946: Fix integer overflow in `get_array`
56fb1287d8f590f1a739b5b23f0d621efa89dc33 Fix integer overflow in `get_array` (Martin Habovstiak)
Pull request description:
The `get_array` method promised to return `None` in case of out-of-bounds access but internally tried to add caller-controlled values which would result in integer overflow. In case overflow happens the attempted read is certainly out of bounds - goes beyond address space. Therefore this commit treats it as such and fixes it to return `None`.
**This bug was found by creusot.** :tada: :tada: :tada:
Though now that I think about it, it would only cause crashes with debug assertions but ironically, without them it would work fine because the garbage `end` value would make the range invalid causing `get` to return `None.` Still, I believe this fix is better than changing it to `wrapping_add` because it's less confusing for both humans and creusot and there's a good chance the compiler will optimize-out the check.
ACKs for top commit:
apoelstra:
ACK 56fb1287d8f590f1a739b5b23f0d621efa89dc33; successfully ran local tests
tcharding:
ACK 56fb1287d8f590f1a739b5b23f0d621efa89dc33
Tree-SHA512: 970ed2cbbf7a8510854a307ba85929221fefb8b18fe34beed560e116cbe9d111b14150579ee329fdcb97ef0f2da1b8da6bc58e45133f2b1573fa9d7d2f8bd177
What changed, and why it matters
This commit fixes a small but real bug in a Rust helper that reads fixed-size chunks from a data slice. The helper was supposed to safely return 'nothing' when asked to read past the end of the data, but it accidentally added two numbers together in a way that could overflow. In Rust, that overflow can crash a program in debug builds. In release builds it would not crash, but the code was still doing the wrong thing logically. The fix uses safe arithmetic so the overflow is caught and the helper returns 'nothing' as documented.
No urgent action required beyond applying the patch. Users running debug builds of code that passes untrusted offsets to `get_array` could experience panics; upgrading removes that risk. Review other slice helpers for similar unchecked arithmetic.
Security signals we found
Integer overflow in bounds-checking helper
Contract violation: method documented to return None on out-of-bounds access could panic instead
Debug-build panic (denial of service) possible
Found by formal verification tool (creusot)
Evidence from the diff
The get_array method in internals/src/slice.rs computes the end of a requested sub-slice as offset + ARRAY_LEN. Both values are caller-controlled/usize-sized, so the addition can panic on integer overflow in debug builds (and is defined to wrap in release builds). The method’s contract is to return None on out-of-bounds access, and an overflowing end is necessarily out of bounds, so the patch replaces the unchecked addition with checked_add(...)?, propagating None on overflow. This is a correctness/defensive fix; the reachable consequences are at most a denial-of-service panic in debug builds, because the wrapped value in release builds produces an invalid range that slice::get already treats as None.
Changed components
internals/src/slice.rsSliceExt::get_arrayInspect captured patch +2 / −1
### internals/src/slice.rs
@@ -96,7 +96,8 @@ impl<T> SliceExt for [T] {
}
fn get_array<const ARRAY_LEN: usize>(&self, offset: usize) -> Option<&[Self::Item; ARRAY_LEN]> {
- self.get(offset..(offset + ARRAY_LEN)).map(|slice| {
+ let end = offset.checked_add(ARRAY_LEN)?;
+ self.get(offset..end).map(|slice| {
slice
.try_into()
.expect("the arguments to `get` evaluate to the same length the return type uses")Why this scored 33/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.