Misaligned reference from drop field in packed struct

[JakobDegen]

I tried this code:

struct U16(u16);

impl Drop for U16 {
    fn drop(&mut self) {
        println!("{:p}", self);
    }
}

struct HasDrop;

impl Drop for HasDrop {
    fn drop(&mut self) {}
}

#[repr(packed)]
struct Misalign(u8, Wrapper);

struct Wrapper {
    _a: U16,
    b: HasDrop,
}

fn main() {
    let m = Misalign(
        0,
        Wrapper {
            _a: U16(10),
            b: HasDrop,
        },
    );
    let _x = m.1.b;
}

I expected to see this happen: The reference is aligned

Instead, this happened:

$ rustc +nightly -Copt-level=3 test.rs
$ ./test
0x7ffc5e1dad79

Meta

rustc --version --verbose:

rustc 1.64.0-nightly (2643b1646 2022-07-27)
binary: rustc
commit-hash: 2643b16468fda787470340890212591d8bc832b7
commit-date: 2022-07-27
host: x86_64-unknown-linux-gnu
release: 1.64.0-nightly
LLVM version: 14.0.6

@rustbot label I-unsound A-mir

[JakobDegen]

We have a pass to add moves for drops in packed structs, but it doesn't fire in this case. That's because there's a bug here - this computation cannot just take into account the innermost field, it needs to compute a minimum of some sort.

[apiraino]

@JakobDegen is this a regression? What did you exactly expected the output to be? (I'd like to run a bisection). thanks.

[JakobDegen]

This is not a regression, it was wrong all the way back in 1.0

This was right in [1.24.0, 1.53.0).

Getting a consistent reproduction of this is slightly difficult because the reference might happen to align correctly by accident. In other words, the fact that the example above prints a pointer that is not 2-aligned means that something is certainly wrong. However, if the address were to be 2-aligned that could either be because the bug is fixed or because we happened to get lucky.

The consistent way to check this is by looking at --emit mir. In 1.52.0 the MIR contains

        _3 = move ((_1.1: Wrapper).0: U16); // scope 0 at /app/example.rs:32:1: 32:2
        drop(_3) -> bb4;                 // scope 0 at /app/example.rs:32:1: 32:2

which is correct (the move ensures that the resulting place is aligned) while 1.53.0 contains

        drop(((_1.1: Wrapper).0: U16)) -> bb4; // scope 0 at /app/example.rs:32:1: 32:2

Note that there is no move.

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high E-needs-bisection

[apiraino]

I have run a bisection with cargo-bisect on your main.rs.

$ RUST_SRC_REPO=~/projects/rust/rust cargo-bisect-rustc --preserve --start=2021-01-01 --script=./script.sh

$ cat script.sh
#!/bin/sh
rustc --emit mir -Copt-level=3 main.rs
grep "drop\(_3\) -> bb4" main.mir

The bisection could not pinpoint a single commit, maybe it sinked into a rollup merge. But if it helps, the regression should be in nightly-2021-03-30 and coming from one of these PRs

found 8 bors merge commits in the specified range
  commit[0] 2021-03-28UTC: Auto merge of #83602 - JohnTitor:cloudabi-flag-is-unnecessary, r=Xanewok
  commit[1] 2021-03-28UTC: Auto merge of #83619 - petrochenkov:nx, r=nagisa
  commit[2] 2021-03-29UTC: Auto merge of #83605 - RalfJung:unaligned, r=petrochenkov
  commit[3] 2021-03-29UTC: Auto merge of #83565 - RalfJung:miri, r=oli-obk
  commit[4] 2021-03-29UTC: Auto merge of #83637 - bjorn3:sync_cg_clif-2021-03-29, r=bjorn3
  commit[5] 2021-03-29UTC: Auto merge of #83609 - klensy:c-str, r=m-ou-se
  commit[6] 2021-03-29UTC: Auto merge of #80839 - tblah:riscv64linux_links, r=Mark-Simulacrum
  commit[7] 2021-03-29UTC: Auto merge of #83185 - jyn514:remove-dead-code, r=oli-obk

@JakobDegen do you see in any of those a possible culprit? (I don't have the context to)

[JakobDegen]

Yeah, it's #83605, cc @RalfJung

[RalfJung]

Not sure how #83605 would have an effect here, it should be a NOP for repr(packed(N)) with N < 2. Looks like something else is amiss as well and that PR surfaced the bug?

Where is the logic that moves packed structs to a new place before dropping implemented?

The consistent way to check this is by looking at --emit mir. In 1.52.0 the MIR contains

That's odd, I would have expected this to be inside drop_in_place... after all calling drop_in_place on a packed struct also has to do this move. Why does MIR building adjust the caller to satisfy a requirement that the callee should be responsible for?

[JakobDegen]

@RalfJung the pass is called AddMoveForPackedDrops or something like that. And this can't take place inside the callee because the callee receives a &mut Self, and if that reference is unaligned then UB has already occurred and there's nothing the callee can do to fix things anymore

[RalfJung]

drop_in_place takes a raw pointer, so it could very well be the place where the packed struct alignment trouble is handled. And IMO it'd be a much more natural place than doing this by transforming caller MIR.

[JakobDegen]

@RalfJung I don't understand the suggestion. The example above never calls drop_in_place for a packed struct. The offending drop in the MIR in the example above is drop(m.1._a). The type of that place is U16 - but if drop_in_place::<U16>() were to be responsible for moving, then we would need to emit a move in literally every drop shim for a non-trivial alignment type.

[RalfJung]

Oh I see, I didn't quite read the example correctly. The problematic drop is the one that happens on field assignment, and we know it's a packed field because of the path we are accessing it with.

EDIT: no wait this makes no sense. Now I am confused by your comment.

[RalfJung]

No what I just said makes no sense. What happens is that this is a partial drop -- one field has been moved out (m.1.b) and then the logic for dropping the remaining fields seems to be wrong.

So #83605 is relevant because it changed is_disaligned. And it looks like somehow the change is wrong; here is also fails to ask for an unsafe block despite the reference being unaligned.

[RalfJung]

#100064 should fix this.