Potential unsoundness in DrainProducer around drop_in_place on mutable reference

[Manishearth]

rayon/src/vec.rs

Lines 226 to 231 in 7069695

	impl<'data, T: 'data + Send> Drop for DrainProducer<'data, T> {
	fn drop(&mut self) {
	// use `Drop for [T]`
	unsafe { ptr::drop_in_place(self.slice) };
	}
	}

This code takes &mut Self, which itself contains an &mut [T], and runs drop glue for the elements in the slice.

While ptr::drop_in_place() does not explicitly say that the resultant value is invalid, it says it is "invalid to read". My understanding of the Rust unsafe model is that it is UB to have a live reference to a type that is invalid to read, in this case we are violating the validity constraints of &mut [T], which in turn violates validity of &mut Self.

The type does come from ManuallyDrop, but it's still UB to invalidate that whilst it has live references to it.

I think this should probably be a raw pointer + phantomdata?

[Manishearth]

The usage of IterMut could have similar problems were IterMut implemented by wrapping an &mut T and an index, but it's not, so it gets away on a technicality.

[cuviper]

Well this sounds a little familiar -- see #851 / #852 (and later #907), and the cited UCG#77 still isn't resolved. I think this case is actually on more solid ground, because we're not dealing with uninitialized data (dropping is different), but I'm willing to try a more conservative approach here.

The usage of IterMut could have similar problems were IterMut implemented by wrapping an &mut T and an index, but it's not, so it gets away on a technicality.

I don't see how -- the references we get from IterMut are then independent of it, as this isn't a GAT iterator. But that borrow does still tie back to wherever we got it from in the first place.

[cuviper]

Do you think this suffices?

diff --git a/src/vec.rs b/src/vec.rs
index c804b0f339bb..900b3b9785bd 100644
--- a/src/vec.rs
+++ b/src/vec.rs
@@ -226,7 +226,8 @@ impl<'data, T: 'data + Send> Producer for DrainProducer<'data, T> {
 impl<'data, T: 'data + Send> Drop for DrainProducer<'data, T> {
     fn drop(&mut self) {
         // use `Drop for [T]`
-        unsafe { ptr::drop_in_place(self.slice) };
+        let slice = mem::take(&mut self.slice) as *mut [T];
+        unsafe { ptr::drop_in_place(slice) };
     }
 }
 

[Manishearth]

not dealing with uninitialized data (dropping is different)

The docs on drop_in_place are kinda squirrelly about this, they say that the dropped pointer should never be read from, but it does seem like in the current plans for the unsafe model there is no difference between having a reference and reading from it.

I plan to file an issue upstream about what precisely drop_in_place does when it comes to validity.

Do you think this suffices?

No, because ultimately &mut self is still left in a potentially invalid situation: I think the field needs to be a raw pointer.

[Manishearth]

https://rust-lang.zulipchat.com/#narrow/stream/136281-t-opsem/topic/is.20drop_in_place.20on.20.26mut.20T.20ever.20valid.3F

[cuviper]

Why is &mut self invalid? After the mem::take, it has no connection to the pointer we're about to drop.

[Manishearth]

Oh! Yes, that should be fine.

[cuviper]

See #1030.