Potential soundness issue (stacked borrows): Is alloc_fast_path persisting borrows too long?

[Manishearth]

rust-typed-arena/src/lib.rs

Lines 193 to 205 in f1f26c3

	fn alloc_fast_path(&self, value: T) -> Result<&mut T, T> {
	let mut chunks = self.chunks.borrow_mut();
	let len = chunks.current.len();
	if len < chunks.current.capacity() {
	chunks.current.push(value);
	// Avoid going through `Vec::deref_mut`, which overlaps
	// other references we have already handed out!
	debug_assert!(len < chunks.current.len()); // bounds check
	Ok(unsafe { &mut *chunks.current.as_mut_ptr().add(len) })
	} else {
	Err(value)
	}
	}

It's unclear to me if this is sound under stacked/tree borrows.

Essentially, the lifetime of the *chunks mutable place is less than that of the function, and when we call chunks.current.as_mut_ptr() we materialize an &mut Vec<T> for the receiver, but then we return a reference within there that lives longer.

My understanding of how these things interact with cells is that from the purposes of these rules cell types behind references can do things that would not be allowed from the general rules of working with a reference, but that doesn't apply to references obtained from within a cell, which is what's happening here.

As far as I can see there's no actual safe way to do this currently (but it is absolutely something that ought to be safe): we need a way to get the ptr behind a vector without materializing a reference to it, and there is currently no self: *mut Self Vec API for this. (We essentially need the equivalent of addr_of!() but for vector fields). So personally this does not worry me too much since "this is unsafe according to currently-in-flux rules but there is no safe way to handle this use case" to me means that we should raise the use case with the folks making the rules and wait for the right APIs.

I also understand that UCG has been trying to figure out the balance on recursive validity and stuff, and Miri does not currently complain on this.

So this might be fine in the end, but I do think we should talk about it and raise this use case with UCG so we can get a ruling and folks can ensure the stdlib gets the appropriate APIs as needed.

[Manishearth]

Well I guess one safe way to do this is to only store raw pointers, but that does kinda suck

[Manishearth]

Also opened a zulip thread to get feedback from people who understand these rules better than I do.

[thomcc]

I think vec.as_mut_ptr() deliberately doesn't materialize a slice for this reason, but perhaps I'm misunderstanding the issue.

[Manishearth]

@thomcc it does because as_mut_ptr() has an &mut Self receiver.

(this kind of thing is exactly why addr_of!() exists)

[Manishearth]

However, AIUI UCG does not want types like Vec to be special here, so maybe this is still fine and the stacked borrow does not poke through the vec.

[thomcc]

the &mut Self only applies to the Vec, not to the pointer it holds (which is inside the RawVec).

[Manishearth]

Yeah, I'm convinced. We're fine as long as Vec isn't special and so far I have mostly heard noises that Vec is not special.

[Manishearth]

Also relevant, from Ralf:

Stacked Borrows and Tree Borrows are strictly 1-layer, they don't go recurisvely looking for nested references

[RalfJung]

Yeah, I'm convinced. We're fine as long as Vec isn't special and so far I have mostly heard noises that Vec is not special.

Cc rust-lang/unsafe-code-guidelines#326

[Manishearth]

Ah, thanks, I was going to go digging for that in a bit 😄

[Manishearth]

Opened rust-lang/rust#113859 upstream to document the common-sense-but-not-guaranteed invariant that this code relies on.