Skip to content

Fix uninitialized read when the filler closure panics - #151

Open
tooson9010-spec wants to merge 1 commit into
NULLx76:mainfrom
tooson9010-spec:fix-panic-safety-uninit-read
Open

Fix uninitialized read when the filler closure panics#151
tooson9010-spec wants to merge 1 commit into
NULLx76:mainfrom
tooson9010-spec:fix-panic-safety-uninit-read

Conversation

@tooson9010-spec

Copy link
Copy Markdown

fill_with sets the write pointer to the full capacity before running the
filler. If the closure panics partway through, the buffer reports every slot as
live while the ones the loop hadn't reached hold uninitialized memory. A later
read — or the buffer's own Drop — then ptr::reads one of them.

Reported in #150 for AllocRingBuffer; ConstGenericRingBuffer has the same
problem. GrowableAllocRingBuffer is unaffected — it pushes into a VecDeque.

Reproducer

The added test fills an 8-slot buffer with a closure that panics on the fourth
call:

buffer reports 8 live slots but only 3 were written

Draining it afterwards trips Miri:

error: Undefined Behavior: constructing invalid value of type Vec<u8>:
at .buf.inner.cap.0, encountered uninitialized memory, but expected an integer
   --> src/with_alloc/alloc_ringbuffer.rs:272:27

The backtrace runs through Drop for AllocRingBufferdequeue, so the caller
doesn't have to touch the buffer for this to fire.

The fix

Advance the write pointer one slot at a time, as each slot is written. A panic
then leaves it covering only what was initialised.

ConstGenericRingBuffer::fill_with delegated to [MaybeUninit<T>; CAP]::fill_with,
which gives no place to commit per slot, so it now uses an explicit loop.

Regression test

src/lib.rs gains a case that runs all three implementations through a
panicking filler. cargo test catches it; cargo miri test confirms the
uninitialized read is gone.

AllocRingBuffer::fill_with and ConstGenericRingBuffer::fill_with set the
write pointer to the full capacity before running the filler. If the closure
panics partway through, the buffer still reports every slot as live while the
ones the loop had not reached hold uninitialized memory. A later read - or the
buffer's own Drop - then ptr::reads one of them.

Commit each slot as it is written instead.
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