Miscompilation where binding only some fields leaks the others

[mk12]

I found a bug where Rust leaks values (never calls drop). This is the root cause of a memory leak we saw in a Fuchsia test, caught by LeakSanitizer: https://bugs.fuchsia.dev/p/fuchsia/issues/detail?id=87961.

This is as far as I could minimize it:

#![allow(unused)]

struct S(u32);

impl Drop for S {
    fn drop(&mut self) {
        println!("dropping {}", self.0);
    }
}

enum E {
    A,
    B((S, S)),
}

fn main() {
    let mut foo = Some(E::A);
    match foo {
        Some(E::A) => (),
        _ => return,
    }
    foo.insert(E::B((S(1), S(2))));
    match foo {
        Some(E::B((x, _))) => {}
        _ => {}
    }
}

Expected output:

dropping 2
dropping 1

Actual output:

dropping 1

It leaked S(2). If you change (x, _) to (_, x) in the second match statement, it leaks S(1) instead. If you bind neither (_, _) or both (x, y), then both get dropped as expected.

If you use Box<u8> instead of S, you can detect a memory leak using LeakSanitizer or Valgrind.

Meta

I reproduced this on stable:

rustc 1.56.1 (59eed8a2a 2021-11-01)
binary: rustc
commit-hash: 59eed8a2aac0230a8b53e89d4e99d55912ba6b35
commit-date: 2021-11-01
host: x86_64-unknown-linux-gnu
release: 1.56.1
LLVM version: 13.0.0

And on nightly:

rustc 1.58.0-nightly (8b09ba6a5 2021-11-09)
binary: rustc
commit-hash: 8b09ba6a5d5c644fe0f1c27c7f9c80b334241707
commit-date: 2021-11-09
host: x86_64-unknown-linux-gnu
release: 1.58.0-nightly
LLVM version: 13.0.0

[jyn514]

Note that the miscompilation goes away if you change foo.insert((E::B) to foo = Some(E::B). So I suspect the unreachable_unchecked in insert is what's causing this somehow.

rust/library/core/src/option.rs

Line 1211 in e5cfe84

unsafe { self.as_mut().unwrap_unchecked() }

[jyn514]

Also, the miscompilation is in MIR, not LLVM, because Miri also fails to run the destructor.

[jyn514]

Very slightly smaller (I removed the fields in S):

struct S;

impl Drop for S {
    fn drop(&mut self) {
        println!("dropping");
    }
}

enum E {
    A,
    B((S, S)),
}

fn main() {
    let mut foo = Some(E::A);
    match foo {
        Some(E::A) => (),
        Some(E::B(_)) | None => return,
    }
    foo.insert(E::B((S, S)));
    match foo {
        Some(E::B((x, _))) => {}
        _ => {},
    };
}

[jyn514]

Oh strangely this is not related to the unwrap_unchecked, it still reproduces with a normal unwrap. https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=6e6a87916e94f4012b2d93615a7ce1d1

[jyn514]

Here's a version without std::option::Option (just an in-crate option): https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=025985867f32378dad0785597377a680

[nbdd0121]

A quick manual bisection on godbolt shows that this regresses in Rust 1.43 (is correct in Rust 1.42).

@rustbot label: regression-from-stable-to-stable

[nbdd0121]

Bisection reveals that this issue is introduced in #68528.

I think the issue is that precise_enum_drop_elaboration didn't re-mark all variants as possibly active after the mutable reference to the enum is used. The miscompilation goes away with -Z precise_enum_drop_elaboration=n. I filed #90756 to disable precise_enum_drop_elaboration by default, and we can turn this back on once the issue is fixed.

cc @ecstatic-morse
@rustbot label: +A-mir

[ecstatic-morse]

Thanks for the detailed bug report. I think I know what the root cause is, but it's not a trivial fix. I'll work on this later today.

[ecstatic-morse]

The root cause

Here's my preferred minimization, using an indirect write instead of Option::insert and Option instead of the inner enum. I've annotated the code to show how it is affected by the "active enum variant" logic in #68528 and where things go wrong.

struct S(i32);

impl Drop for S {
    fn drop(&mut self) {
        println!("dropping {}", self.0);
    }
}

fn main() {
    let mut foo = None;
    match foo {
        None => (),
        _ => return,
    }

    // Here we know that `foo` is `None`, otherwise we would have returned above.

    // Overwrite `foo` indirectly. `foo` is no longer `None`, but we fail to update our
    // initialization data to reflect this.
    *(&mut foo) = Some((S(0), S(1)));

    // Move from `(foo as Some).0` (despite the fact that we think it is uninitialized),
    // creating a drop flag for `(foo as Some).0` and making the subsequent
    // drop of `foo` an open one.
    match foo {
        Some((_x, _)) => {
        } // `_x` is dropped: "dropping 0"

        _ => {}
    }

    // `foo` is dropped, but it is an "open" drop, so the move paths associated with its
    //`Some` variants are still marked as uninitialized and the `Drop` is eliminated.
}

I was a bit surprised that the second match statement was necessary to make the bug manifest. Without the partial move of (_1 as Some).0 into _x, the drop of _1 is a shallow one (its subpaths aren't considered), and since _1 is still initialized (only its variants are cleared by #68582), everything gets dropped correctly.

As @nbdd0121 suggests, it's the write through the mutable borrow that causes the problem. Maybe{Un}InitializedPlaces was able to ignore indirect writes in the past because it's illegal to create a mutable reference to uninitialized memory in the first place. Things get more complex when raw pointers and unsafe code is involved (see #90718), but the original behavior was correct for realistic code.

However, once we started marking inactive enum variants as uninitialized, it became possible to have a mutable reference that can update an uninitialized place in safe code, which is what happened here.

A quick fix

To fix the memory leak in the near term, it should suffice to modify Maybe{Un}InitializedPlaces to mark all children of a place as "maybe initialized" when it sees a mutable borrow of that place. This solution is very brittle, since it relies on the fact that a match on some variable introduces a shallow borrow on that variable that prevents its discriminant from changing inside the match body. That shallow borrow would prevent code like the following:

let mut foo = None;
let pfoo = &mut foo; // Mutable borrow here

match foo {
    Some(_) => return,
    None => (),
}
// `Some` variants are marked as uninitialized, despite the fact that a mutable borrow is outstanding.

*pfoo = Some(...); // Reinitialize `Some` variants, without creating a new mutable borrow.

However, there may be ways to circumvent this. For example, converting pfoo to a raw pointer before the first match and then using it to write to foo after it would pass the borrow checker, although I believe that's a violation of stacked borrows. Additionally, there may be a way to generate MIR that looks like a match on a discriminant to Maybe{Un}InitializedPlaces, but does not cause a shallow borrow of foo.

Perhaps there are even more cases where this fix falls short. I've opened a PR that implements this (#90788) so I can discuss it further with the compiler team.

A long-term solution

The real mistake here is conflating the active variant of a local with its initializedness. In the long-term, we should use a bespoke dataflow analysis to track the possible active variants of an enum, and incorporate those results into drop elaboration. The approach in #68582 was taken because it required the smallest number of changes to drop elaboration, a complex part of the compiler whose original author is no longer active and where it is easy to introduce subtle miscompilations.

One safeguard that would have caught this is to check the results of MaybeUninitializedPlaces against the MIR to see if a possibly uninitialized place is moved from. Such places should cause move errors, so we can delay_span_bug if we see a suspicious move. This would be relatively expensive, however.

Even if the quick fix is acceptable for now, I think we should pursue something more robust going forward.

[apiraino]

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-critical

[pnkfelix]

FYI This is addressed by PR #90788, which will be part of the Rust 1.58 release (stable in six weeks).

[apiraino]

Checking progress, @mk12 did you have a chance to test against the new stable 1.58 if it solves the issue? thanks

[mk12]

@apiraino The example I gave in my first comment now behaves as expected on the Rust playground with 1.58.1 stable. I'm fairly sure this solves the original Fuchsia bug, but it's difficult to reproduce that because a workaround was used at the time and that code has changed a lot since.

[apiraino]

I'm going to close this issue as we think it's solved. In case it can be reopened or follow-up issues can be created