Fix leak on panic in insert_many.
#213
Conversation
lib.rs
Outdated
| @@ -898,10 +901,17 @@ impl<A: Array> SmallVec<A> { | |||
| ptr = self.as_mut_ptr().add(index); | |||
| cur = ptr.add(num_added); | |||
| ptr::copy(cur, cur.add(1), old_len - index); | |||
|
|
|||
| guard.ptr = self.as_mut_ptr(); | |||
There was a problem hiding this comment.
I am actually somewhat surprised that this does not invalidate ptr... doesn't self.as_mut_ptr() create a mutable reference that invalidates all other pointers? I suppose this function is called via the DerefMut impl, so it even creates an intermediate &mut [Item] that covers all elements currently marked as initialized.
There was a problem hiding this comment.
self.len() has been set to zero at this point, so self currently derefs to an empty slice.
However, MIRI also does not seem to flag use use of vec.deref().as_mut_ptr() to create and use aliasing raw pointers to elements of non-empty Vecs, so maybe this is just a limitation of MIRI.
There was a problem hiding this comment.
Vec has a pointer indirection, so &mut Vec just demands exclusive access to the fields of that struct. But I thought smallvec stores (some) elements inline, thus &mut SmallVec does demand exclusive access to those elements.
self.len() has been set to zero at this point, so self currently derefs to an empty slice.
Well, but &mut self is still being passed fully to deref_mut, which covers all inline elements.
Also, an empty slice turned into a raw pointer should only be used to access elements of that slice (the pointer "comes from" the slice and this is restricted to the memory the slice covers). Probably there are just so many raw pointers everywhere here that Miri is fine, but that might change once we experiment with tracking raw pointers more precisely.
Even if nothing goes wrong here (I'd need to dig deeper to find out why), maybe it would be better to follow Vec and have raw pointer methods that do not go through slices?
There was a problem hiding this comment.
Thanks, that makes sense. I'll try converting this to replace unique borrows with raw-pointer-only methods.
There was a problem hiding this comment.
Updated the code to avoid invalidating raw pointers before using them, and shadowed the as_ptr and as_mut_ptr methods (as in std::vec::Vec) to avoid dereferencing to slice while the contents may be invalid.
There was a problem hiding this comment.
I don't have time to review this in detail right now but avoiding the intermediate slice is certainly an improvement. At some point I'll make Miri's raw ptr checking more strict and then we'll see what happens here and all over the rest of the ecosystem. ;)
|
r? @SimonSapin |
mbrubeck
force-pushed
the
leak
branch
2 times, most recently
from
31a4e89
to
82eeece
Compare
|
☔ The latest upstream changes (presumably #229) made this pull request unmergeable. Please resolve the merge conflicts. |
mbrubeck
force-pushed
the
leak
branch
2 times, most recently
from
13408f6
to
6e3453e
Compare
|
Rebased and added some more test cases. r? @SimonSapin or @jdm |
|
@bors-servo r+ |
|
📌 Commit 8ddf613 has been approved by |
|
☀️ Test successful - checks-travis |
Version 1.4.2 Changelog: * `insert_many` no longer leaks elements if the provided iterator panics (#213). * The unstable `const_generics` and `specialization` features are updated to work with the most recent nightly Rust toolchain (#232). * Internal code cleanup (#229, #231). This PR also changes the `author` field in `Cargo.toml` to "The Servo Project Developers".
Fixes #208 and reverts the workaround added in #209. CC @RalfJung.