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

invalid_reference_casting false positive: dynamically sized types #121074

Closed
dtolnay opened this issue Feb 14, 2024 · 4 comments · Fixed by #121104
Closed

invalid_reference_casting false positive: dynamically sized types #121074

dtolnay opened this issue Feb 14, 2024 · 4 comments · Fixed by #121104
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@dtolnay
Copy link
Member

dtolnay commented Feb 14, 2024

fn main() {
    let x: Box<dyn Send> = Box::new(0i32);
    let z = unsafe { &*(&*x as *const dyn Send as *const i32) };
    println!("{}", z);
}

I believe this code is well defined.

It runs successfully in Miri with RUSTFLAGS='-Zcrate-attr=allow(invalid_reference_casting)' cargo +nightly miri run.

However, since #118983 (nightly-2024-02-14), the following deny-by-default lint is triggered in cargo check:

$ cargo +nightly-2024-02-14 check

error: casting references to a bigger memory layout than the backing allocation is undefined behavior, even if the reference is unused
 --> src/main.rs:3:22
  |
3 |     let z = unsafe { &*(&*x as *const dyn Send as *const i32) };
  |                      ^^^^--^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |                          |
  |                          backing allocation comes from here
  |
  = note: casting from `dyn Send` (0 bytes) to `i32` (4 bytes)
  = note: `#[deny(invalid_reference_casting)]` on by default

Rustc thinks the dyn Send is 0 bytes, whereas for the purpose of the memory model as I understand it, it has a runtime size that is bigger than 0 bytes.

@dtolnay dtolnay added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Feb 14, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 14, 2024
@dtolnay dtolnay added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed regression-untriaged Untriaged performance or correctness regression. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 14, 2024
dtolnay added a commit to dtolnay/cargo-tally that referenced this issue Feb 14, 2024
I believe this is a false positive. rust-lang/rust#121074

    error: casting references to a bigger memory layout than the backing allocation is undefined behavior, even if the reference is unused
      --> src/arena.rs:81:30
       |
    81 |         let arena = unsafe { &*(&**arena as *const dyn Send as *const Arena<T>) };
       |                              ^^^^-------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       |                                  |
       |                                  backing allocation comes from here
       |
       = note: casting from `dyn Send` (0 bytes) to `Arena<T>` (56 bytes)
       = note: `#[deny(invalid_reference_casting)]` on by default
@Urgau
Copy link
Member

Urgau commented Feb 14, 2024

Rustc thinks the dyn Send is 0 bytes, whereas for the purpose of the memory model as I understand it, it has a runtime size that is bigger than 0 bytes.

The 0 bytes here is the size reported at compile time, which obviously doesn't make sense with DST, the lint implementation should be gated on the types being sized. (to be honest, I completely forgot about this).

There should probably be an if !inner_start_ty.is_sized(cx.tcx, cx.param_env) { return None; } somewhere after this

let ty::Ref(_, inner_start_ty, _) = start_ty.kind() else {
return None;
};

@ogoffart
Copy link
Contributor

My code is also regressing in this:

pub struct Opaque(());
pub unsafe fn foo(opaque: *const Opaque) {
   let foo = &*((&*opaque) as *const Opaque as *const u8);
}

Gives the error on nightly

error: casting references to a bigger memory layout than the backing allocation is undefined behavior, even if the reference is unused
 --> src/lib.rs:4:14
  |
4 |    let foo = &*((&*opaque) as *const Opaque as *const u8);
  |              ^^^^^-------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |                   |
  |                   backing allocation comes from here
  |
  = note: casting from `Opaque` (0 bytes) to `u8` (1 bytes)
  = note: `#[deny(invalid_reference_casting)]` on by default

This doesn't involve DST though. But the pattern is the same as we just go through a &* which shouldn't count as a dereference.

@Urgau
Copy link
Member

Urgau commented Feb 14, 2024

@ogoffart Your sample code is as far as I can tell UB and Miri seems to agree with the lint.

error: Undefined Behavior: out-of-bounds pointer use: alloc865 has size 0, so pointer to 1 byte starting at offset 0 is out-of-bounds
 --> src/main.rs:5:15
  |
5 |    let _foo = &*((&*opaque) as *const Opaque as *const u8);
  |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ out-of-bounds pointer use: alloc865 has size 0, so pointer to 1 byte starting at offset 0 is out-of-bounds
  |
  = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
  = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

Note, removing the inner dereference let _foo = &*(opaque as *const u8); still triggers the lint and is also UB; because as noted casting references to a bigger memory layout than the backing allocation is undefined behavior.

If you want to continue this conversation, let's create a new issue as to not diverge this specific issue.

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 15, 2024
@bors bors closed this as completed in 12d70af Feb 15, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 15, 2024
Rollup merge of rust-lang#121104 - Urgau:bigger_layout-fix-fp, r=compiler-errors

Ignore unsized types when trying to determine the size of the original type

Fixes rust-lang#121074 a regression from rust-lang#118983
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. 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