Fix (stacked-borrows-)UB found by Miri #121
Conversation
| @@ -3063,11 +3063,8 @@ pub trait ByteSlice: Sealed { | |||
| // Finally, we are only dealing with u8 data, which is Copy, which | |||
| // means we can copy without worrying about ownership/destructors. | |||
| unsafe { | |||
| ptr::copy( | |||
There was a problem hiding this comment.
Does miri complain here? I can't spot any problem with the existing code.
(Note that I'm aware of stacked borrows and understand the issues here. But that understanding came long after bstr was originally written.)
There was a problem hiding this comment.
Yes, Miri complains here: https://miri.saethlin.dev/ub?crate=bstr&version=1.0.0-pre.2
I would start by disabling all of the quickcheck tests with miri and hope that is good enough... Other than that, maybe the Unicode tests would take too long? I would imagine the rest of the tests should be fine though. |
Yes, I believe everything here is not the flavor that will directly result in surprising code generation by LLVM. I'll try your suggestions for disabling tests. |
We make an absolute mess of our tests so that 'cargo miri test' will complete in reasonable time. I hate this, but Miri is worth it. Ref #121
When using 'get_unchecked' twice where one is a mutable borrow, it ends up creating UB under the "stacked borrows" model. Which isn't adopted yet. Still, it seems likely that it will? So we fix it by deriving both pointers to 'ptr::copy' from the same 'get_unchecked_mut' call. Closes #121
Previously, we were using 'end_ptr' by computing a zero-length pointer by offseting from the end of the haystack. But for pointer provenance reasons, it is more correct to computer the end pointer by adding to the start pointer. I don't believe this is necessary for the forward case, but do it there too "just in case" and also to make the code more similar to the reverse case. Closes #121
We make an absolute mess of our tests so that 'cargo miri test' will complete in reasonable time. I hate this, but Miri is worth it. Ref #121
Previously, we were using 'end_ptr' by computing a zero-length pointer by offseting from the end of the haystack. But for pointer provenance reasons, it is more correct to computer the end pointer by adding to the start pointer. I don't believe this is necessary for the forward case, but do it there too "just in case" and also to make the code more similar to the reverse case. Closes #121
The
ptr::copyis a common footgun in the current formulation of Stacked Borrows: rust-lang/unsafe-code-guidelines#133 Ralf thinks we can be rid of it, but until then it would be nice if MIri passes.The issue with
end_ptris that originally this code turns a slice into a pointer, but then tries to walk the pointer outside of the slice. You can't do that in Stacked Borrows, pointers derived from references can only be used to access the referent, not the whole allocation.(also it might be cool to have Miri in CI, but I think you know better than I what to sculpt with the test cases that run for too long in the interpreter)