Skip to content

False positives from invalid_reference_casting #124685

Description

@khuey
use std::ptr;

fn apply_mask(array: &mut [u8], offset: usize, mask: u64) {
    assert!(offset + 8 <= array.len());
    let a1 = &mut array[offset];
    let a2 = a1 as *mut u8;
    let a3 = a2 as *mut u64;
    unsafe {
        ptr::write_unaligned(a3, ptr::read_unaligned(a3) | mask);
    };
}

Starting with rustc 1.78.0 (because of #118983) this produces the following

error: casting references to a bigger memory layout than the backing allocation is undefined behavior, even if the reference is unused
 --> src/lib.rs:9:9
  |
5 |     let a1 = &mut array[offset];
  |                   ------------- backing allocation comes from here
6 |     let a2 = a1 as *mut u8;
7 |     let a3 = a2 as *mut u64;
  |              -------------- casting happend here
8 |     unsafe {
9 |         ptr::write_unaligned(a3, ptr::read_unaligned(a3) | mask);
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: casting from `u8` (1 bytes) to `u64` (8 bytes)
  = note: `#[deny(invalid_reference_casting)]` on by default

Perhaps I'm missing something but this is not undefined behavior as far as I can tell. The lint is wrong about the backing allocation and I suspect the false warning is downstream from that?

Activity

  1. added
    needs-triageThis issue may need triage. Remove when done. See docs forge.rust-lang.org/release/issue-triaging
    on May 4, 2024
  2. added
    A-lintsArea: Lints (warnings about flaws in source code) such as unused_mut.
    regression-from-stable-to-stablePerformance or correctness regression from one stable version to another.
    and removed
    needs-triageThis issue may need triage. Remove when done. See docs forge.rust-lang.org/release/issue-triaging
    on May 4, 2024
  3. added
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on May 4, 2024
  4. asquared31415 commented on May 4, 2024

    @asquared31415
    Contributor

    Under the memory model checked by Miri by default, a &mut u8 is not permitted to access more than the one byte it points to, even if you got it from a larger object. Reborrowing to a reference type shrinks the allowed access range. Use array[offset..(offset + 8)].as_mut_ptr() instead to obtain a pointer that is valid to write to the correct region. array[offset..].as_mut_ptr() would also be valid, as that region contains the correct bytes, but it's less precise about its intent.

    @rustbot label -regression-from-stable-to-stable -C-bug -I-prioritize +C-discussion

  5. added
    C-discussionCategory: Discussion or questions that doesn't represent real issues.
    and removed
    regression-from-stable-to-stablePerformance or correctness regression from one stable version to another.
    C-bugCategory: This is a bug.
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on May 4, 2024
  6. asquared31415 commented on May 4, 2024

    @asquared31415
    Contributor

    Miri also agrees that the code as written contains undefined behavior, in the top right of the page select "Tools > Miri" to run and you'll see that it says that there's undefined behavior.
    https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=e2d5ba429bbd2fb8401599c04d046213

    The important bit from the error message is that the code is trying to access alloc1270[0x0..0x8] (the intended 8 bytes), but the pointer does not have permission for that range, only [0x0..0x1] (the first byte).

  7. asquared31415 commented on May 4, 2024

    @asquared31415
    Contributor

    I think that the invalid_reference_casting lint's wording should be changed though, "backing allocation" is not precise and does not correctly describe what it's detecting. cc #118983

    @rustbot label +A-diagnostics +D-papercut

  8. added
    A-diagnosticsArea: Messages for errors, warnings, and lints
    D-papercutDiagnostics: An error or lint that needs small tweaks.
    on May 4, 2024
  9. saethlin commented on May 4, 2024

    @saethlin
    Member

    Under the current memory model,

    Your comment and the lint are misleading. There is no accepted memory model. And the lint message is pointing to code which does not create an allocation, so at best the lint has grabbed the wrong span. If this lint is supposed to engage in provenance-based reasoning, it would need to indicate that. Otherwise it is simply buggy.

  10. asquared31415 commented on May 4, 2024

    @asquared31415
    Contributor

    True, "the current" is not quite right. "The memory model currently checked by Miri by default" is perhaps a better wording for what I wanted to say, I will edit my comment.

    Agreed that the lint is definitely sketchy wording at best, and I'm suspicious of its implementation given the incorrect wording.

  11. added
    C-bugCategory: This is a bug.
    and removed
    C-discussionCategory: Discussion or questions that doesn't represent real issues.
    on May 4, 2024
  12. changed the title [-]False positive invalid_reference_casting?[/-] [+]False positives from invalid_reference_casting[/+] on May 4, 2024
  13. Urgau commented on May 4, 2024

    @Urgau
    Member

    And the lint message is pointing to code which does not create an allocation, so at best the lint has grabbed the wrong span. If this lint is supposed to engage in provenance-based reasoning, it would need to indicate that. Otherwise it is simply buggy.

    The lint is not supposed to engage in "provenance-based reasoning", it is just supposed to peel all the reference/raw pointer casting until it finds an allocation, and then it should compare the size of type and report an error if the target is bigger than the source. Nothing else, nothing more.

    I tried describing the intent of the change in #118983 (comment).

    So as @saethlin correctly mentions it, nothing in this code creates an "allocation", so I think the lint shouldn't have fired here. (whenever there is actual UB or not is irrelevant here)

  14. added a commit that references this issue on May 8, 2024
  15. added a commit that references this issue on May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-diagnosticsArea: Messages for errors, warnings, and lintsA-lintsArea: Lints (warnings about flaws in source code) such as unused_mut.C-bugCategory: This is a bug.D-papercutDiagnostics: An error or lint that needs small tweaks.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions