Skip to content

Commit 73261bd

Browse files
committed
spsc: fix potential overflows
Thanks Jannic for the find: #652 (comment) If N == usize::MAX, there is the possibility of a panic in len() If N >= usize::MAX, then in the iterator code, self.index + self.head could overflow The operations are now slightly more complex and a bit slower, but thanks to compiler optimization they don't introduce branches, only conditional instructions (cmov, csel, it). All added tests fail without the fix
1 parent 0950363 commit 73261bd

File tree

2 files changed

+107
-10
lines changed

2 files changed

+107
-10
lines changed

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ of panicking drop implementations.
1414
- Added `pop_front_if` and `pop_back_if` to `Deque`
1515
- Made `Vec::from_array` const.
1616
- Fixed long division being instroduced by the const-erasure in spsc
17+
- spsc: Fix integer overflow in iterators when N > usize::MAX/2 and the queue loops.
18+
- spsc: Fix integer overflow leading to a panic in `len` when N == usize::MAX and debug assertions are enabled.
1719

1820
## [v0.9.2] 2025-11-12
1921

src/spsc.rs

Lines changed: 105 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,10 @@ impl<T, S: Storage> QueueInner<T, S> {
176176

177177
#[inline]
178178
fn increment(&self, val: usize) -> usize {
179-
let val = val + 1;
179+
// We know that self.n() <= usize::MAX
180+
// So this can only overflow if N == usize::MAX
181+
// and in this case the overflow will be equivalent to the modulo N operation
182+
let val = val.wrapping_add(1);
180183
if val >= self.n() {
181184
val - self.n()
182185
} else {
@@ -628,8 +631,11 @@ impl<'a, T> Iterator for Iter<'a, T> {
628631
if self.index < self.len {
629632
let head = self.rb.head.load(Ordering::Relaxed);
630633

631-
let i = head + self.index;
632-
let i = if i >= self.rb.n() { i - self.rb.n() } else { i };
634+
let i = match head.checked_add(self.index) {
635+
Some(i) if i >= self.rb.n() => i - self.rb.n(),
636+
Some(i) => i,
637+
None => head.wrapping_add(self.index).wrapping_sub(self.rb.n()),
638+
};
633639
self.index += 1;
634640

635641
Some(unsafe { &*(self.rb.buffer.borrow().get_unchecked(i).get() as *const T) })
@@ -646,8 +652,11 @@ impl<'a, T> Iterator for IterMut<'a, T> {
646652
if self.index < self.len {
647653
let head = self.rb.head.load(Ordering::Relaxed);
648654

649-
let i = head + self.index;
650-
let i = if i >= self.rb.n() { i - self.rb.n() } else { i };
655+
let i = match head.checked_add(self.index) {
656+
Some(i) if i >= self.rb.n() => i - self.rb.n(),
657+
Some(i) => i,
658+
None => head.wrapping_add(self.index).wrapping_sub(self.rb.n()),
659+
};
651660
self.index += 1;
652661

653662
Some(unsafe { &mut *self.rb.buffer.borrow().get_unchecked(i).get().cast::<T>() })
@@ -663,8 +672,11 @@ impl<T> DoubleEndedIterator for Iter<'_, T> {
663672
let head = self.rb.head.load(Ordering::Relaxed);
664673

665674
// self.len > 0, since it's larger than self.index > 0
666-
let i = head + self.len - 1;
667-
let i = if i >= self.rb.n() { i - self.rb.n() } else { i };
675+
let i = match head.checked_add(self.len - 1) {
676+
Some(i) if i >= self.rb.n() => i - self.rb.n(),
677+
Some(i) => i,
678+
None => head.wrapping_add(self.len - 1).wrapping_sub(self.rb.n()),
679+
};
668680
self.len -= 1;
669681
Some(unsafe { &*(self.rb.buffer.borrow().get_unchecked(i).get() as *const T) })
670682
} else {
@@ -679,8 +691,11 @@ impl<T> DoubleEndedIterator for IterMut<'_, T> {
679691
let head = self.rb.head.load(Ordering::Relaxed);
680692

681693
// self.len > 0, since it's larger than self.index > 0
682-
let i = head + self.len - 1;
683-
let i = if i >= self.rb.n() { i - self.rb.n() } else { i };
694+
let i = match head.checked_add(self.len - 1) {
695+
Some(i) if i >= self.rb.n() => i - self.rb.n(),
696+
Some(i) => i,
697+
None => head.wrapping_add(self.len - 1).wrapping_sub(self.rb.n()),
698+
};
684699
self.len -= 1;
685700
Some(unsafe { &mut *self.rb.buffer.borrow().get_unchecked(i).get().cast::<T>() })
686701
} else {
@@ -888,9 +903,19 @@ impl<T> Producer<'_, T> {
888903

889904
#[cfg(test)]
890905
mod tests {
891-
use std::hash::{Hash, Hasher};
906+
use std::{
907+
cell::UnsafeCell,
908+
hash::{Hash, Hasher},
909+
mem::MaybeUninit,
910+
};
892911

893912
use super::{Consumer, Producer, Queue};
913+
#[cfg(not(feature = "portable-atomic"))]
914+
use core::sync::atomic;
915+
#[cfg(feature = "portable-atomic")]
916+
use portable_atomic as atomic;
917+
918+
use atomic::AtomicUsize;
894919

895920
use static_assertions::assert_not_impl_any;
896921

@@ -1310,4 +1335,74 @@ mod tests {
13101335
};
13111336
assert_eq!(hash1, hash2);
13121337
}
1338+
1339+
// Test for some integer overflow bugs. See
1340+
// https://github.com/rust-embedded/heapless/pull/652#discussion_r3046630717
1341+
// for more info
1342+
#[test]
1343+
#[cfg_attr(miri, ignore)] // too slow
1344+
fn test_len_overflow() {
1345+
let mut queue = Queue::<(), { usize::MAX }> {
1346+
head: AtomicUsize::new(usize::MAX),
1347+
tail: AtomicUsize::new(2),
1348+
buffer: [const { UnsafeCell::new(MaybeUninit::new(())) }; usize::MAX],
1349+
};
1350+
queue.enqueue(()).unwrap();
1351+
queue.enqueue(()).unwrap();
1352+
1353+
let collected: Vec<_> = queue.iter().collect();
1354+
assert_eq!(&collected, &[&(); 4]);
1355+
}
1356+
1357+
#[test]
1358+
#[cfg_attr(miri, ignore)] // too slow
1359+
fn test_usize_overflow_iter() {
1360+
let queue = Queue::<(), { usize::MAX - 1 }> {
1361+
head: AtomicUsize::new(usize::MAX - 3),
1362+
tail: AtomicUsize::new(2),
1363+
buffer: [const { UnsafeCell::new(MaybeUninit::new(())) }; usize::MAX - 1],
1364+
};
1365+
1366+
let collected: Vec<_> = queue.iter().collect();
1367+
assert_eq!(&collected, &[&(); 4]);
1368+
}
1369+
1370+
#[test]
1371+
#[cfg_attr(miri, ignore)] // too slow
1372+
fn test_usize_overflow_iter_mut() {
1373+
let mut queue = Queue::<(), { usize::MAX - 1 }> {
1374+
head: AtomicUsize::new(usize::MAX - 3),
1375+
tail: AtomicUsize::new(2),
1376+
buffer: [const { UnsafeCell::new(MaybeUninit::new(())) }; usize::MAX - 1],
1377+
};
1378+
1379+
let collected: Vec<_> = queue.iter_mut().collect();
1380+
assert_eq!(&collected, &[&(); 4]);
1381+
}
1382+
1383+
#[test]
1384+
#[cfg_attr(miri, ignore)] // too slow
1385+
fn test_usize_overflow_iter_rev() {
1386+
let queue = Queue::<(), { usize::MAX - 1 }> {
1387+
head: AtomicUsize::new(usize::MAX - 3),
1388+
tail: AtomicUsize::new(2),
1389+
buffer: [const { UnsafeCell::new(MaybeUninit::new(())) }; usize::MAX - 1],
1390+
};
1391+
1392+
let collected: Vec<_> = queue.iter().rev().collect();
1393+
assert_eq!(&collected, &[&(); 4]);
1394+
}
1395+
1396+
#[test]
1397+
#[cfg_attr(miri, ignore)] // too slow
1398+
fn test_usize_overflow_iter_mut_rev() {
1399+
let mut queue = Queue::<(), { usize::MAX - 1 }> {
1400+
head: AtomicUsize::new(usize::MAX - 3),
1401+
tail: AtomicUsize::new(2),
1402+
buffer: [const { UnsafeCell::new(MaybeUninit::new(())) }; usize::MAX - 1],
1403+
};
1404+
1405+
let collected: Vec<_> = queue.iter_mut().rev().collect();
1406+
assert_eq!(&collected, &[&(); 4]);
1407+
}
13131408
}

0 commit comments

Comments
 (0)