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.

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

Conversation

ratmice
Copy link
Contributor

@ratmice ratmice commented Mar 8, 2024

The test added here was failing, this adds manual bounds checking on has_element_at,
for reasons I don't yet understand this manual bounds checking is not equivalent to self.data.get(idx).is_some(),
which appears to actually just break everything.

@ratmice
Copy link
Contributor Author

ratmice commented Mar 8, 2024

worth mentioning this doesn't manage to fix all the UB from cargo +nightly miri test but the addition to the test failed with cargo +nightly miri test clear, and now that passes.

Adding something like for foo in self.data { foo.take() } to clear() should actually fix the behavior (in this case), but it should also still trigger miri due to the uninitialized read.

@ratmice
Copy link
Contributor Author

ratmice commented Mar 8, 2024

I spent some time digging into the other miri failure I was seeing compact_tiny.
It looks like it is just because we take 2 mutable (non-overlapping) pointers out of a single mutable reference.
Which is UB in the stacked-borrows model. If we run it with tree-borrows however all tests now pass.

MIRIFLAGS='-Zmiri-tree-borrows' cargo +nightly miri test

@@ -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.

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

Copy link
Contributor Author

@ratmice ratmice Mar 9, 2024

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.

@LukasKalbertodt
Copy link
Owner

Ouch, thanks a lot for catching and fixing this. I somehow never considered miri getting better over time and that I should rerun the tests.

Disappointing to see that despite me cautions, there is another memory unsafety issue with this crate. Meh.

@LukasKalbertodt LukasKalbertodt merged commit 1e48278 into LukasKalbertodt:master Mar 10, 2024
1 of 5 checks passed
@ratmice
Copy link
Contributor Author

ratmice commented Mar 10, 2024

This one actually was what caused me to look into miri, because I ran into the UB from entirely safe code.
It was caught one of the existing tests in Lox, when switching one of the StableVecs there to use InlineStableVec when I was playing with an implementation of Lox #38

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.

2 participants