Hello — while researching panic-safety in Rust crates, I found that
AllocRingBuffer::fill_with is not panic-safe.
Summary
fill_with marks the buffer full (writeptr = self.capacity) before running the
filler closure that initializes the slots. If the closure panics partway through,
writeptr still claims all capacity slots are live, but the slots the loop hadn't
reached yet were never written.
A later read — or the buffer's own Drop — then ptr::reads one of those
uninitialized slots. This is reachable from safe Rust (use of uninitialized memory,
CWE-908) and is a soundness issue.
Affected method
AllocRingBuffer::fill_with — src/with_alloc/alloc_ringbuffer.rs:
fn fill_with<F: FnMut() -> T>(&mut self, mut f: F) {
self.clear();
self.readptr = 0;
self.writeptr = self.capacity; // buffer marked full here
for i in 0..self.capacity {
unsafe { ptr::write(get_unchecked_mut(self, i), f()) }; // f() may panic
}
}
writeptr is set to capacity before the loop. If f() panics during the loop,
unwinding leaves writeptr unchanged, so the buffer reports capacity live elements
while the not-yet-written slots hold uninitialized memory.
A subsequent read reaches the unchecked ptr::read at line 272:
unsafe { Some(ptr::read(res)) } // res may point at an uninitialized slot
Impact
Reading an uninitialized slot builds a T out of uninitialized bytes. For a
heap-owning element type such as String, the resulting value has garbage
length/pointer fields, so using or dropping it operates on an invalid allocation.
Reachable from safe Rust: the filler is an ordinary safe closure, and a panic in it
(unwrap(), indexing, assert!, or an explicit panic!) is enough. No unsafe in
the caller.
Verification
Under Miri, a proof-of-concept that calls fill_with with a panicking filler under
catch_unwind and then drops the buffer reports UB at the ptr::read:
error: Undefined Behavior: constructing invalid value of type Item:
encountered uninitialized memory, but expected an integer
--> src/with_alloc/alloc_ringbuffer.rs:272:27
|
272 | unsafe { Some(ptr::read(res)) }
The caller uses no unsafe and a non-panicking element Drop; the only unwinding is
inside the filler closure.
Affected versions
Confirmed on ringbuffer 0.16.0.
Thank you for your time.
Hello — while researching panic-safety in Rust crates, I found that
AllocRingBuffer::fill_withis not panic-safe.Summary
fill_withmarks the buffer full (writeptr = self.capacity) before running thefiller closure that initializes the slots. If the closure panics partway through,
writeptrstill claims allcapacityslots are live, but the slots the loop hadn'treached yet were never written.
A later read — or the buffer's own
Drop— thenptr::reads one of thoseuninitialized slots. This is reachable from safe Rust (use of uninitialized memory,
CWE-908) and is a soundness issue.
Affected method
AllocRingBuffer::fill_with—src/with_alloc/alloc_ringbuffer.rs:writeptris set tocapacitybefore the loop. Iff()panics during the loop,unwinding leaves
writeptrunchanged, so the buffer reportscapacitylive elementswhile the not-yet-written slots hold uninitialized memory.
A subsequent read reaches the unchecked
ptr::readat line 272:Impact
Reading an uninitialized slot builds a
Tout of uninitialized bytes. For aheap-owning element type such as
String, the resulting value has garbagelength/pointer fields, so using or dropping it operates on an invalid allocation.
Reachable from safe Rust: the filler is an ordinary safe closure, and a panic in it
(
unwrap(), indexing,assert!, or an explicitpanic!) is enough. Nounsafeinthe caller.
Verification
Under Miri, a proof-of-concept that calls
fill_withwith a panicking filler undercatch_unwindand then drops the buffer reports UB at theptr::read:The caller uses no
unsafeand a non-panicking elementDrop; the only unwinding isinside the filler closure.
Affected versions
Confirmed on
ringbuffer0.16.0.Thank you for your time.