Avoid miri error in slice::sort under Stacked Borrows

[Voultapher]

See comment in code.

Fixes: #151728

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[folkertdev]

I suspect you meant to link to an issue, but currently link to the PR updating the sorting algorithm?

[zachs18]

The issue in question is #151728

[zachs18]

I think this could also fix the issue by ptr::reading and holding pivot_copy as a MaybeUninit<T> instead of a T: 8b7dd57, instead of cfg!(miri)ing out the code

[theemathas]

Can this cause sorting in miri to result in a different order from the order outside miri? (Especially with misbehaving PartialOrd impls, or with equal but distinct elements.)

[Noratrieb]

Yeah I think MaybeUninit should work just fine (or MaybeDangling if that already exists?). I would really prefer to avoid adding cfg!(miri) for SB UB. Note that TB will not become the standard as it is insufficiently permissive for optimizations, the final model will likely be between TB and SB in terms of permissiveness. That's why SB is the default and why we should follow SB when possible.
cc @RalfJung

[Voultapher]

I would really like to avoid the cfg!(miri) myself. My understanding was that the ptr::read(&v[pivot_pos]) itself creating a duplicate Box that is read after the original position was mutated is the problem so I'm not sure how MaybeUninit would help here, unless it get's special treatment by miri.

Testing it out the error persists if I do:

// SAFETY: We only access the temporary copy for Freeze types, otherwise
// self-modifications via `is_less` would not be observed and this would
// be unsound. Our temporary copy does not escape this scope.
let pivot_copy = unsafe { MaybeUninit::new(ptr::read(&v[pivot_pos])) };
// SAFETY: We created the value with `MaybeUninit::new` so it is guaranteed init.
let pivot_ref =
    (!has_direct_interior_mutability::<T>()).then_some(unsafe { &*pivot_copy.as_ptr() });

[Voultapher]

Can this cause sorting in miri to result in a different order from the order outside miri? (Especially with misbehaving PartialOrd impls, or with equal but distinct elements.)

• equal but distinct elements. -> No, this only affects the stable sort.
• misbehaving PartialOrd impls -> Yes, but those mostly panic anyway and we don't define the order, doc:

If the implementation of Ord for T does not implement a total order, the function may panic; even if the function exits normally, the resulting order of elements in the slice is unspecified. See also the note on panicking below.

[Noratrieb]

Yes, but those mostly panic anyway and we don't define the order

it's really really unfortunate if Miri has different behavior than not-Miri, no matter how broken the code.

[theemathas]

Maybe just the ptr::read() is sufficient to invalidate the Box? What about something similar to this:

let pivot_copy = unsafe { ptr::read((&raw const v[pivot_pos]).cast::<MaybeUninit<T>>()) };

[Voultapher]

Nice, that does it.

[RalfJung]

I suspect that this line is constructing a ManuallyDrop<Box> that invalidates the original Box inside the slice

Temporarily creating a child box that then disappears immediately should be fine. So for this to be a problem, there have to be overlapping (non-nested) lifetimes of the boxes here... is that's what is going on?

[Voultapher]

Temporarily creating a child box that then disappears immediately should be fine. So for this to be a problem, there have to be overlapping (non-nested) lifetimes of the boxes here... is that's what is going on?

I'm not sure. I'll try to explain a simplified algorithm:

• Recursion depth 0: left_ancestor_pivot == None, partitions, creates stack copy of pivot
• Recursion depth 1: left_ancestor_pivot == &pivot from level 0, partitions which can swap around the location that v[pivot] is in v, reads left_ancestor_pivot <- miri complains

[RalfJung]

Okay so there's some other place in the code where Boxes are being retagged, specifically when two elements are being swapped? Might be worth also making that an entirely bytewise operation without retags.

[Voultapher]

Not sure what you mean, the swapping - it's more complicated than that since it's first written to an auxiliary buffer - all happens via ptr::copy_nonoverlapping isn't that a bytewise operation? The thing that might be re-tagging it is creating a reference when passing it to the user-provided is_less which for API reasons we can't get around.

[RalfJung]

Yeah, ptr::copy_nonoverlapping is a byte-level operation.

The thing that might be re-tagging it is creating a reference when passing it to the user-provided is_less which for API reasons we can't get around.

Ah, that makes sense. So the order of events is

1. pointer gets retagged and derived pointer stored in left_ancestor_pivot
2. pointer gets retagged again to be passed to is_less, invalidating the pointer from step 1
3. the pointer from step 1 is used 💥

[Voultapher]

That sounds right, yep.