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 leak on panic in insert_many.

Merged
merged 1 commit into from Jul 8, 2020
Merged

Conversation

mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented Apr 22, 2020

Fixes #208 and reverts the workaround added in #209. CC @RalfJung.

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();
Copy link
Contributor

@RalfJung RalfJung Apr 23, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mbrubeck mbrubeck Apr 23, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@RalfJung RalfJung Apr 23, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@mbrubeck mbrubeck Apr 23, 2020

Choose a reason for hiding this comment

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

Thanks, that makes sense. I'll try converting this to replace unique borrows with raw-pointer-only methods.

Copy link
Contributor Author

@mbrubeck mbrubeck May 13, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@RalfJung RalfJung May 14, 2020

Choose a reason for hiding this comment

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

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. ;)

@mbrubeck
Copy link
Contributor Author

mbrubeck commented May 14, 2020

r? @SimonSapin

@mbrubeck mbrubeck force-pushed the leak branch 2 times, most recently from 31a4e89 to 82eeece Compare May 14, 2020
@bors-servo
Copy link
Contributor

bors-servo commented Jul 7, 2020

The latest upstream changes (presumably #229) made this pull request unmergeable. Please resolve the merge conflicts.

@mbrubeck mbrubeck force-pushed the leak branch 2 times, most recently from 13408f6 to 6e3453e Compare Jul 7, 2020
@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jul 7, 2020

Rebased and added some more test cases. r? @SimonSapin or @jdm

@mbrubeck mbrubeck requested a review from jdm Jul 7, 2020
@jdm
Copy link
Member

jdm commented Jul 8, 2020

@bors-servo r+
Thanks for fixing this!

@bors-servo
Copy link
Contributor

bors-servo commented Jul 8, 2020

📌 Commit 8ddf613 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jul 8, 2020

Testing commit 8ddf613 with merge cce91ca...

@bors-servo
Copy link
Contributor

bors-servo commented Jul 8, 2020

☀️ Test successful - checks-travis
Approved by: jdm
Pushing cce91ca to master...

@bors-servo bors-servo merged commit cce91ca into servo:master Jul 8, 2020
2 checks passed
mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this issue Aug 11, 2020
* `insert_many` no longer leaks elements if the provided iterator panics (servo#213).
* The unstable `const_generics` and `specialization` features are
  updated to work with the most recent nightly Rust toolchain (servo#232).
* Internal code cleanup (servo#229, servo#231).
@mbrubeck mbrubeck mentioned this pull request Aug 11, 2020
bors-servo added a commit that referenced this issue Aug 11, 2020
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".
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.

4 participants