Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

We are computing the wrong offset for unsized packed structs #118537

Closed
RalfJung opened this issue Dec 2, 2023 · 2 comments · Fixed by #118540
Closed

We are computing the wrong offset for unsized packed structs #118537

RalfJung opened this issue Dec 2, 2023 · 2 comments · Fixed by #118540
Labels
A-layout Area: Memory layout of types C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Dec 2, 2023

This code panics, but it should not:

use std::mem;

struct Slice([u32]);

#[repr(packed, C)]
struct PackedSized {
    f: u8,
    d: [u32; 4],
}

#[repr(packed, C)]
struct PackedUnsized {
    f: u8,
    d: Slice,
}

impl PackedSized {
    fn unsize(&self) -> &PackedUnsized {
        // We can't unsize via a generic type since then we get the error
        // that packed structs with unsized tail don't work if the tail
        // might need dropping.
        let len = 4usize;
        unsafe { mem::transmute((self, len)) }
    }
}

fn main() { unsafe {
    let p = PackedSized { f: 0, d: [1, 2, 3, 4] };
    let p = p.unsize() as *const PackedUnsized;
    let d = std::ptr::addr_of!((*p).d);
    assert_eq!(d.cast::<u32>().read_unaligned(), 1);
} }

The panic disappears when the Newtype is inlined (because then we hit a different code path in this match).

What happens here is a bug in the logic that computes the offset of d: since it is an unsized field, the offset is computed dynamically, by taking the statically known offset and then rounding it up to the dynamic alignment of the field. However, since the struct is packed, we need to cap the dynamic alignment of the field to the packing of the surrounding struct -- which we currently do not do. This is just yet another case where the non-compositional nature of repr(packed) is causing issues...

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 2, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Dec 2, 2023

A slight variant of this code ICEs:

use std::mem;

#[repr(packed(4))]
struct Slice([u32]);

#[repr(packed(2), C)]
struct PackedSized {
    f: u8,
    d: [u32; 4],
}

#[repr(packed(2), C)]
struct PackedUnsized {
    f: u8,
    d: Slice,
}

impl PackedSized {
    fn unsize(&self) -> &PackedUnsized {
        // We can't unsize via a generic type since then we get the error
        // that packed structs with unsized tail don't work if the tail
        // might need dropping.
        let len = 4usize;
        unsafe { mem::transmute((self, len)) }
    }
}

fn main() { unsafe {
    let p = PackedSized { f: 0, d: [1, 2, 3, 4] };
    let p = p.unsize() as *const PackedUnsized;
    let d = std::ptr::addr_of!((*p).d);
    assert_eq!(d.cast::<u32>().read_unaligned(), 1);
} }

It hits this assertion which, it seems, was never updated when support for packed(N) was implemented.

@saethlin saethlin added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-layout Area: Memory layout of types and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 2, 2023
@fmease fmease added the C-bug Category: This is a bug. label Dec 2, 2023
@RalfJung RalfJung added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Dec 3, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 3, 2023
@apiraino
Copy link
Contributor

apiraino commented Dec 4, 2023

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

EDIT: lowered priority after discussion on Zulip

@rustbot rustbot added P-high High priority P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Dec 4, 2023
@apiraino apiraino removed the P-high High priority label Dec 4, 2023
@bors bors closed this as completed in 87625db Dec 4, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Dec 4, 2023
Rollup merge of rust-lang#118540 - RalfJung:unsized-packed-offset, r=TaKO8Ki

codegen, miri: fix computing the offset of an unsized field in a packed struct

`#[repr(packed)]`  strikes again.

Fixes rust-lang#118537
Fixes rust-lang/miri#3200

`@bjorn3` I assume cranelift needs the same fix.
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Dec 5, 2023
codegen, miri: fix computing the offset of an unsized field in a packed struct

`#[repr(packed)]`  strikes again.

Fixes rust-lang/rust#118537
Fixes #3200

`@bjorn3` I assume cranelift needs the same fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-layout Area: Memory layout of types C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants