Skip to content

Commit

Permalink
fix(cordyceps): fix use-after-free in List iterators
Browse files Browse the repository at this point in the history
The `cordyceps::List` `Iter` and `Cursor` types currently have incorrect
`Item` types that can result in a UAF if used incorrectly. These
iterators return `T::Handle` as their `Item` types...but a `Handle`
*owns* an element, and dropping the handle can drop the element. If, for
example, the element is `Box`, then dropping the `Handle` will
deallocate the memory that the entry lives in. Similarly, dropping an
`Arc` entry would perform an invalid reference count decrement.

If an iterator over a list is created twice, and the list contains a
handle type such as `Box<T>`, then the second iterator will cause a
double free. More worryingly, accessing any entries from the list will
cause a use-after-free.

We missed this issue since

1. there weren't any tests for the iterators, and
2. all the tests currently use `&'a Entry` as the handle type, so
   dropping an entry doesn't free memory.

This branch fixes the use-after-free by changing the `Iter` and `Cursor`
types to return *references* to the element, rather than `Handle`s. We
will add an additional `Drain` iterator that actually removes elements
from the list and returns the `Handle` type, in a follow-up PR.

I've added a test which fails against the current `main` branch.

BREAKING CHANGE:

This changes the type signature of the `list::Iter` and `list::Cursor`
types.
  • Loading branch information
hawkw committed Jun 6, 2022
1 parent 3493cd0 commit 29b7d6c
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 5 deletions.
2 changes: 1 addition & 1 deletion alloc/src/buddy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ where
match list.try_lock() {
Some(list) => {
for entry in list.iter() {
tracing::debug!("entry={:?}", unsafe { entry.as_ref() });
tracing::debug!("entry={entry:?}");
}
}
None => {
Expand Down
19 changes: 15 additions & 4 deletions cordyceps/src/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -610,9 +610,16 @@ unsafe impl<T: Sync> Sync for Links<T> {}
// === impl Cursor ====

impl<'a, T: Linked<Links<T>> + ?Sized> Iterator for Cursor<'a, T> {
type Item = T::Handle;
type Item = &'a mut T;
fn next(&mut self) -> Option<Self::Item> {
self.next_ptr().map(|ptr| unsafe { T::from_ptr(ptr) })
self.next_ptr().map(|mut ptr| unsafe {
// safety: it is safe for us to mutate `curr`, because the cursor
// mutably borrows the `List`, ensuring that the list will not be dropped
// while the iterator exists. the returned item will not outlive the
// cursor, and no one else can mutate it, as we have exclusive
// access to the list..
ptr.as_mut()
})
}
}

Expand Down Expand Up @@ -650,12 +657,16 @@ impl<'a, T: Linked<Links<T>> + ?Sized> Cursor<'a, T> {
// === impl Iter ====

impl<'a, T: Linked<Links<T>> + ?Sized> Iterator for Iter<'a, T> {
type Item = T::Handle;
type Item = &'a T;
fn next(&mut self) -> Option<Self::Item> {
let curr = self.curr.take()?;
unsafe {
// safety: it is safe for us to borrow `curr`, because the iterator
// borrows the `List`, ensuring that the list will not be dropped
// while the iterator exists. the returned item will not outlive the
// iterator.
self.curr = T::links(curr).as_ref().next();
Some(T::from_ptr(curr))
Some(curr.as_ref())
}
}
}

0 comments on commit 29b7d6c

Please sign in to comment.