Stacked Borrows violation in BufWriter::into_parts

[CAD97]

I tried this code: [playground]

use std::io::{Cursor, BufWriter};

fn main() {
    let mut v = vec![0; 1024];
    let c = Cursor::new(&mut v);
    let w = BufWriter::new(Box::new(c));
    let _ = w.into_parts();
}

I expected to see this happen: it runs cleanly under Miri.

Instead, this happened:

error: Undefined Behavior: trying to retag from <2686> for Unique permission at alloc1164[0x0], but that tag does not exist in the borrow stack for this location
   --> /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/buffered/bufwriter.rs:175:10
    |
175 |         (inner, buf)
    |          ^^^^^
    |          |
    |          trying to retag from <2686> for Unique permission at alloc1164[0x0], but that tag does not exist in the borrow stack for this location
    |          this error occurs as part of retag at alloc1164[0x0..0x10]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <2686> was created by a Unique retag at offsets [0x0..0x10]
   --> src/main.rs:7:13
    |
7   |     let _ = w.into_parts();
    |             ^^^^^^^^^^^^^^
help: <2686> was later invalidated at offsets [0x0..0x10] by a Unique retag (of a reference/box inside this compound value)
   --> src/main.rs:7:13
    |
7   |     let _ = w.into_parts();
    |             ^^^^^^^^^^^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `std::io::BufWriter::<std::boxed::Box<std::io::Cursor<&mut std::vec::Vec<u8>>>>::into_parts` at /playground/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/io/buffered/bufwriter.rs:175:10: 175:15
note: inside `main`
   --> src/main.rs:7:13
    |
7   |     let _ = w.into_parts();
    |             ^^^^^^^^^^^^^^

The issue is in this construct:

https://github.com/rust-lang/rust/blob/master/library%2Fstd%2Fsrc%2Fio%2Fbuffered%2Fbufwriter.rs#L171-L173

By the SB model, moving a (struct directly containing) Box retags that Box as the live unique owner of the referenced allocation. Thus passing self to forget invalidates the duplicated Box (W) we just retrieved. The correct way to write this is with ManuallyDrop:

// inner is the only remaining field with nontrivial drop glue
let this = ManuallyDrop::new(self);
// SAFETY: this takes logical ownership of inner
let inner = unsafe { ptr::read(&this.inner) };

Meta

Tested against 1.81.0-nightly (2024-07-09 6be96e3).

This also catches BufWriter::into_inner, which is implemented using into_parts.

IIRC the LLVM backend is currently not informed about this instance of this UB, as the relevant attributes are only added when the argument passing ABI is Scalar or ScalarPair, which it is not in this case. Additionally, Miri's SB implementation does not complain when the box is Box<dyn Write>, (conjecture: since dyn Write is not Unpin).

cc @RalfJung — another case of read; forget instead of ManuallyDrop::new; read in std.

This was randomly spotted. It might be worth it to do a grep for forget to try to spot further such mistakes. forget's docs call this out as incorrect, but it's still quite easy to overlook.

[RalfJung]

Good catch!

Additionally, Miri's SB implementation does not complain when the box is Box, as the allocation size is not known at compile time.

That is odd. Miri uses size_of_val to determine the size to use for retags, so dyn Write should not make a difference.

[CAD97]

Just searched for forget(self) using the GitHub search. The only other suspect snippet is already marked as kinda iffy:

https://github.com/rust-lang/rust/blob/master/library%2Fstd%2Fsrc%2Fsys%2Fpal%2Fsgx%2Fabi%2Ftls%2Fmod.rs#L97-L99

and somewhat amusingly CString calls out this exact issue in comments as well:

https://github.com/rust-lang/rust/blob/master/library%2Falloc%2Fsrc%2Fffi%2Fc_str.rs#L608-L613

That is odd

The reason I assigned was conjecture based on likely old information (Miri only checking cases where we passed constant assertions to LLVM by default), but it is the observed behavior. The second potential reason now coming to mind is that dyn Trait is !Unpin, thus getting raw pointer retags instead of unique retags.

[RalfJung]

The second potential reason now coming to mind is that dyn Trait is !Unpin, thus getting raw pointer retags instead of unique retags.

Ah yes that would explain it.

[saethlin]

Just searched for forget(self) using the GitHub search. The only other suspect snippet is already marked as kinda iffy:

I'm putting up a PR to fix just the instance reported here, but I would like to see a larger effort to replace all uses of mem::forget.

[GrigorenkoPV]

but I would like to see a larger effort to replace all uses of mem::forget

In compiler, libs, or both?

All usages of mem::forget or just some particular ones?

[saethlin]

Note that I am not a libs reviewer, so before you submit a large PR you should check in with one of these lovely people or post in Zulilp:

rust/triagebot.toml

Lines 941 to 949 in 88fa119

	libs = [
	"@cuviper",
	"@Mark-Simulacrum",
	"@Amanieu",
	"@Nilstrieb",
	"@workingjubilee",
	"@joboet",
	"@jhpratt",
	]

I would prefer a simple policy that we do not use mem::forget. All uses of it can (with varying amounts of typing) be replaced with ManuallyDrop. I don't think it's worth the brain cycles to try to think about whether a certain use of mem::forget follows the pattern that was reported here.

The compiler is much less important here; nobody is running the compiler in Miri and this hazard is specific to an aspect of Stacked Borrows that is not actively used in optimizations the current toolchain does. For the library, being clean with respect to Stacked Borrows is an important quality-of-implementation issue. The same concern does not apply to the compiler.

Note that if you run the example program in this issue with MIRIFLAGS=-Zmiri-tree-borrows there is no UB report, and that is a deliberate aspect of Tree Borrows. But that is just #124346 (the general issue is described in rust-lang/unsafe-code-guidelines#133); the same property that makes this well-defined in Tree Borrows also removes some optimizations that people instinctively believe are valid.