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

Miri preserves padding and partial initialization on copies #845

Closed
Tracked by #2159
RalfJung opened this issue Jul 17, 2019 · 4 comments · Fixed by rust-lang/rust#129778
Closed
Tracked by #2159

Miri preserves padding and partial initialization on copies #845

RalfJung opened this issue Jul 17, 2019 · 4 comments · Fixed by rust-lang/rust#129778
Labels
A-interpreter Area: affects the core interpreter A-validation Area: This affects enforcing the validity invariant, and related UB checking C-bug Category: This is a bug. I-misses-UB Impact: makes Miri miss UB, i.e., a false negative (with default settings)

Comments

@RalfJung
Copy link
Member

Miri should be able to detect that the following is UB because it prints uninitialized memory:

use std::mem;

#[repr(C)]
struct Pair(u8, u16);

fn main() { unsafe {
    let p: Pair = mem::transmute(0u32); // The copy when `Pair` is returned from `transmute` should destroy padding.
    let c = &p as *const _ as *const u8;
    println!("{}", *c.add(1)); // Print the padding byte.
} }

However, currently assignment is just implemented as an untyped memcpy, so we incorrectly preserve padding.

@RalfJung RalfJung added C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement A-validation Area: This affects enforcing the validity invariant, and related UB checking labels Jul 17, 2019
@RalfJung RalfJung added I-misses-UB Impact: makes Miri miss UB, i.e., a false negative (with default settings) A-interpreter Area: affects the core interpreter labels Aug 3, 2019
@RalfJung
Copy link
Member Author

A related issue is that when copying, e.g., [usize; 3], Miri will not perform a "typed copy": a typed copy would make the entire array element "undefined" if any of its bytes is undefined, whereas Miri will just copy the underlying bytes. Also see this stackoverflow question.

@RalfJung RalfJung changed the title Miri preserves padding on copies Miri performs byte-level copies instead of typed copies Apr 15, 2020
@RalfJung RalfJung changed the title Miri performs byte-level copies instead of typed copies Miri preserves padding on copies Jun 3, 2022
@RalfJung RalfJung changed the title Miri preserves padding on copies Miri preserves padding and partial initialization on copies Jun 3, 2022
@RalfJung RalfJung added C-bug Category: This is a bug. and removed C-enhancement Category: a PR with an enhancement or an issue tracking an accepted enhancement labels Jun 5, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jun 6, 2022
… r=oli-obk

interpret: better control over whether we read data with provenance

The resolution in rust-lang/unsafe-code-guidelines#286 seems to be that when we load data at integer type, we implicitly strip provenance. So let's implement that in Miri at least for scalar loads. This makes use of the fact that `Scalar` layouts distinguish pointer-sized integers and pointers -- so I was expecting some wild bugs where layouts set this incorrectly, but so far that does not seem to happen.

This does not entirely implement the solution to rust-lang/unsafe-code-guidelines#286; we still do the wrong thing for integers in larger types: we will `copy_op` them and then do validation, and validation will complain about the provenance. To fix that we need mutating validation; validation needs to strip the provenance rather than complaining about it. This is a larger undertaking (but will also help resolve rust-lang/miri#845 since we can reset padding to `Uninit`).

The reason this is useful is that we can now implement `addr` as a `transmute` from a pointer to an integer, and actually get the desired behavior of stripping provenance without exposing it!
@RalfJung RalfJung reopened this Jun 6, 2022
@RalfJung
Copy link
Member Author

RalfJung commented Jun 6, 2022

No GH I did not want you to close this issue...

@sporksmith
Copy link

sporksmith commented Nov 17, 2022

Another example where Miri may be missing UB. I was attempting to use Miri to remind myself of how to safely work with libc style socket unions, and mistakenly (?) convinced myself that that this is sound:

#[repr(C)]
#[derive(Debug, Copy, Clone)]
struct Foo {
    val16: u16,
    // Padding bytes go here!
    val32: u32,
}

#[repr(C)]
#[derive(Debug, Copy, Clone)]
struct Bar {
    bytes: [u8; 8],
}

union FooBar {
    foo: Foo,
    bar: Bar,
}

pub fn main() {
    // Initialize as u8 to ensure padding bytes are zeroed.
    let mut foobar = FooBar { bar: Bar { bytes: [0u8; 8] } };
    // Reading either field is ok.
    println!("{:?} {:?}", unsafe { foobar.foo }, unsafe { foobar.bar });
    // Does this assignment copy the uninitialized padding bytes
    // over the initialized padding bytes? miri doesn't seem to think so.
    foobar.foo = Foo { val16: 1, val32: 2 };
    // Still OK to read.
    println!("{:?} {:?}", unsafe { foobar.foo }, unsafe { foobar.bar });
}

IIUC the line foobar.foo = Foo { val16: 1, val32: 2 }; is allowed to overwrite the padding bytes in foobar.foo with undefined values, but no UB is detected when those bytes are read on the following line.

Unsurprisingly (?) miri does detect UB if we assign via a bytewise copy:

    let mut foobar = FooBar { bar: Bar { bytes: [0u8; 8] } };
    // OTOH if we do the Rust equivalent of a memcpy instead of assignment,
    // the undefined padding bytes from the source definitely copied to the destination.
    unsafe { std::ptr::copy_nonoverlapping(&Foo { val16: 1, val32: 2 }, &mut foobar.foo, 1) };
    // UB reading the uninitd padding bytes
    println!("{:?} {:?}", unsafe { foobar.foo }, unsafe { foobar.bar });

@RalfJung
Copy link
Member Author

IIUC the line foobar.foo = Foo { val16: 1, val32: 2 }; is allowed to overwrite the padding bytes in foobar.foo with undefined values, but no UB is detected when those bytes are read on the following line.

Yes that sounds correct. This will be hard to do even after the basics of this got implemented, since AFAIK MIR does not even preserve this as a regular assignment... so even if regular assignments reset padding, this would still fail to report UB. @oli-obk would know better what the status is of deaggregation and having a Finalize statement that we could use for erasing padding (which seems better than the alternative of running Miri on non-deaggregated MIR).

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 8, 2024
…saethlin

interpret: make typed copies lossy wrt provenance and padding

A "typed copy" in Rust can be a lossy process: when copying at type `usize` (or any other non-pointer type), if the original memory had any provenance, that provenance is lost. When copying at pointer type, if the original memory had partial provenance (i.e., not the same provenance for all bytes), that provenance is lost. When copying any type with padding, the contents of padding are lost.

This PR equips our validity-checking pass with the ability to reset provenance and padding according to those rules. Can be reviewed commit-by-commit. The first three commits are just preparation without any functional change.

Fixes rust-lang/miri#845
Fixes rust-lang/miri#2182
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 9, 2024
…<try>

interpret: make typed copies lossy wrt provenance and padding

A "typed copy" in Rust can be a lossy process: when copying at type `usize` (or any other non-pointer type), if the original memory had any provenance, that provenance is lost. When copying at pointer type, if the original memory had partial provenance (i.e., not the same provenance for all bytes), that provenance is lost. When copying any type with padding, the contents of padding are lost.

This PR equips our validity-checking pass with the ability to reset provenance and padding according to those rules. Can be reviewed commit-by-commit. The first three commits are just preparation without any functional change.

Fixes rust-lang/miri#845
Fixes rust-lang/miri#2182

try-job: x86_64-gnu-aux
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 9, 2024
…<try>

interpret: make typed copies lossy wrt provenance and padding

A "typed copy" in Rust can be a lossy process: when copying at type `usize` (or any other non-pointer type), if the original memory had any provenance, that provenance is lost. When copying at pointer type, if the original memory had partial provenance (i.e., not the same provenance for all bytes), that provenance is lost. When copying any type with padding, the contents of padding are lost.

This PR equips our validity-checking pass with the ability to reset provenance and padding according to those rules. Can be reviewed commit-by-commit. The first three commits are just preparation without any functional change.

Fixes rust-lang/miri#845
Fixes rust-lang/miri#2182

try-job: x86_64-gnu-aux
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 9, 2024
…<try>

interpret: make typed copies lossy wrt provenance and padding

A "typed copy" in Rust can be a lossy process: when copying at type `usize` (or any other non-pointer type), if the original memory had any provenance, that provenance is lost. When copying at pointer type, if the original memory had partial provenance (i.e., not the same provenance for all bytes), that provenance is lost. When copying any type with padding, the contents of padding are lost.

This PR equips our validity-checking pass with the ability to reset provenance and padding according to those rules. Can be reviewed commit-by-commit. The first three commits are just preparation without any functional change.

Fixes rust-lang/miri#845
Fixes rust-lang/miri#2182

try-job: x86_64-gnu-aux
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 9, 2024
…saethlin

interpret: make typed copies lossy wrt provenance and padding

A "typed copy" in Rust can be a lossy process: when copying at type `usize` (or any other non-pointer type), if the original memory had any provenance, that provenance is lost. When copying at pointer type, if the original memory had partial provenance (i.e., not the same provenance for all bytes), that provenance is lost. When copying any type with padding, the contents of padding are lost.

This PR equips our validity-checking pass with the ability to reset provenance and padding according to those rules. Can be reviewed commit-by-commit. The first three commits are just preparation without any functional change.

Fixes rust-lang/miri#845
Fixes rust-lang/miri#2182
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Sep 10, 2024
…r=saethlin

interpret: make typed copies lossy wrt provenance and padding

A "typed copy" in Rust can be a lossy process: when copying at type `usize` (or any other non-pointer type), if the original memory had any provenance, that provenance is lost. When copying at pointer type, if the original memory had partial provenance (i.e., not the same provenance for all bytes), that provenance is lost. When copying any type with padding, the contents of padding are lost.

This PR equips our validity-checking pass with the ability to reset provenance and padding according to those rules. Can be reviewed commit-by-commit. The first three commits are just preparation without any functional change.

Fixes rust-lang/miri#845
Fixes rust-lang/miri#2182
RalfJung pushed a commit to RalfJung/miri that referenced this issue Sep 10, 2024
interpret: make typed copies lossy wrt provenance and padding

A "typed copy" in Rust can be a lossy process: when copying at type `usize` (or any other non-pointer type), if the original memory had any provenance, that provenance is lost. When copying at pointer type, if the original memory had partial provenance (i.e., not the same provenance for all bytes), that provenance is lost. When copying any type with padding, the contents of padding are lost.

This PR equips our validity-checking pass with the ability to reset provenance and padding according to those rules. Can be reviewed commit-by-commit. The first three commits are just preparation without any functional change.

Fixes rust-lang#845
Fixes rust-lang#2182
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-interpreter Area: affects the core interpreter A-validation Area: This affects enforcing the validity invariant, and related UB checking C-bug Category: This is a bug. I-misses-UB Impact: makes Miri miss UB, i.e., a false negative (with default settings)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants