-
-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(cordyceps): fix use-after-free in List
iterators
#203
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hawkw
changed the title
fix(cordyceps): fix use-after-free in List iterators
fix(cordyceps): fix use-after-free in Jun 6, 2022
List
iterators
hawkw
force-pushed
the
eliza/linked-list-iters
branch
from
June 7, 2022 00:09
29b7d6c
to
fe6d69c
Compare
hawkw
added a commit
that referenced
this pull request
Jun 7, 2022
Signed-off-by: Eliza Weisman <[email protected]>
hawkw
added a commit
that referenced
this pull request
Jun 7, 2022
Signed-off-by: Eliza Weisman <[email protected]>
hawkw
added a commit
that referenced
this pull request
Jun 7, 2022
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.
hawkw
added a commit
that referenced
this pull request
Jun 7, 2022
When a linked list's `Handle` type owns the allocation for the item in the list, it is necessary to drain all entries from the list and drop them when the `List` instance is dropped. Otherwise, the entries may be leaked. This fixes a leak reported by Miri. Fixes #165 BREAKING CHANGE: The `List::new` constructor now requires a `T: Linked<list::Links<T>>` bound.
hawkw
force-pushed
the
eliza/linked-list-iters
branch
from
June 7, 2022 00:12
fe6d69c
to
62841c0
Compare
Signed-off-by: Eliza Weisman <[email protected]>
Signed-off-by: Eliza Weisman <[email protected]>
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.
When a linked list's `Handle` type owns the allocation for the item in the list, it is necessary to drain all entries from the list and drop them when the `List` instance is dropped. Otherwise, the entries may be leaked. This fixes a leak reported by Miri. Fixes #165 BREAKING CHANGE: The `List::new` constructor now requires a `T: Linked<list::Links<T>>` bound.
hawkw
force-pushed
the
eliza/linked-list-iters
branch
from
June 7, 2022 00:14
62841c0
to
b607494
Compare
hawkw
added a commit
that referenced
this pull request
Jun 7, 2022
Signed-off-by: Eliza Weisman <[email protected]>
hawkw
added a commit
that referenced
this pull request
Jun 7, 2022
Signed-off-by: Eliza Weisman <[email protected]>
hawkw
added a commit
that referenced
this pull request
Jun 7, 2022
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
cordyceps::List
Iter
andCursor
types currently have incorrectItem
types that can result in a UAF if used incorrectly. Theseiterators return
T::Handle
as theirItem
types...but aHandle
owns an element, and dropping the handle can drop the element. If, for
example, the element is
Box
, then dropping theHandle
willdeallocate 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 adouble free. More worryingly, accessing any entries from the list will
cause a use-after-free.
We missed this issue since
&'a Entry
as the handle type, sodropping an entry doesn't free memory.
This branch fixes the use-after-free by changing the
Iter
andCursor
types to return references to the element, rather than
Handle
s. Wewill add an additional
Drain
iterator that actually removes elementsfrom 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
andlist::Cursor
types.