Skip to content
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.

Sign up for GitHub

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 UB when calling get() after clear() for InlineStableVec #45

Merged
merged 2 commits into from
Mar 10, 2024
Merged

Fix UB when calling get() after clear() for InlineStableVec #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6cbe8a2
Fix UB when calling `get()` after `clear()` for `InlineStableVec`
ratmice Mar 8, 2024
ad995f7
Add another check to the `with_capacity()` tests.
ratmice Mar 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core/option.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ impl<T> Core<T> for OptionCore<T> {
unsafe fn has_element_at(&self, idx: usize) -> bool {
debug_assert!(idx < self.cap());

self.data.get_unchecked(idx).is_some()
idx < self.len() && self.data.get_unchecked(idx).is_some()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think perhaps this should instead be checked in the safe function StableVecFacade::has_element_at by changing the following check to self.len(), however running into some miri failures under tree-borrows with that change in place.

stable-vec/src/lib.rs

Line 550 in 7e6d29c

if index >= self.core.cap() {

Copy link
Contributor Author

@ratmice ratmice Mar 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that my comment above is wrong and the patch as is probably right as-is in that it seems to make it match the behavior of the other *Core implementations, but those can implement has_element_at in a way that don't cause an uninitialized read.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it would be better if the safe method checks for len and we change the pre-condition of the unsafe has_element_at to idx <= len. But that would be a breaking change and I certainly want to release this as a patch version. And the patch as presented isn't bad, the worst case would be that the idx is checked against cap first and then against len.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I made a long attempt at that, between those two comments, the main thing is there are a lot of other cases mostly the find_empty_slot* ones that also call has_element_at, and so all those need to be updated too.
It much more involved at least than I had initially imagined in the first comment.

}

unsafe fn insert_at(&mut self, idx: usize, elem: T) {
Expand Down
2 changes: 2 additions & 0 deletions src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ macro_rules! gen_tests_for {

assert!(sv.capacity() >= 3);
assert_sv_eq!(sv, []: String);
assert_eq!(sv.get(0), None);
}

#[test]
Expand Down Expand Up @@ -896,6 +897,7 @@ macro_rules! gen_tests_for {
let mut sv = $ty::from_iter(vec![1, 3, 5]);
sv.clear();
assert_sv_eq!(sv, []: u32);
assert_eq!(sv.get(0), None);
}

#[test]
Expand Down
Loading