Skip to content

Accessing DST field of packed struct calculates the wrong field address #80925

Closed
#118538
Closed
Accessing DST field of packed struct calculates the wrong field address#80925
#118538
Labels
A-DSTsArea: Dynamically-sized types (DSTs)A-codegenArea: Code generationC-bugCategory: This is a bug.I-unsoundIssue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/SoundnessP-mediumMedium priorityT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Description

@mahkoh
mahkoh
opened on Jan 11, 2021

Execute with Miri:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7e7d31feebbea4c3caceaccff1fa6bac

The problem is that

rust/compiler/rustc_codegen_ssa/src/mir/place.rs

Lines 181 to 182 in 6526e5c

// Bump the unaligned offset up to the appropriate alignment
let offset = round_up_const_value_to_alignment(bx, unaligned_offset, unsized_align);
and

rust/compiler/rustc_mir/src/interpret/place.rs

Line 413 in 6526e5c

(base.meta, offset.align_to(align))
do not take the reduced field alignment into account and calculate the offset as-if the standard alignment was in place.

Activity

  1. jonas-schievink
    added
    A-miriArea: The miri tool
    requires-nightlyThis issue requires a nightly compiler in some way. When possible, use a F-* label instead.
    A-const-evalArea: Constant evaluation, covers all const contexts (static, const fn, ...)
    on Jan 11, 2021

  2. mahkoh commented on Feb 10, 2021

    @mahkoh
    mahkoh
    on Feb 10, 2021
    ContributorAuthor

    The labels are incorrect. Neither does this require nightly, nor is it related to const-eval. This is I-unsound on stable.

  4. rustbot commented on Feb 14, 2021

    @rustbot
    rustbot
    on Feb 14, 2021 · Hidden as off-topic

  5. rustbot commented on Feb 14, 2021

    @rustbot
    rustbot
    on Feb 14, 2021 · Hidden as off-topic
  6. rustbot
    added
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    and removed
    A-const-evalArea: Constant evaluation, covers all const contexts (static, const fn, ...)
    A-miriArea: The miri tool
    requires-nightlyThis issue requires a nightly compiler in some way. When possible, use a F-* label instead.
    on Feb 14, 2021
  7. LeSeulArtichaut
    added
    I-unsoundIssue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness
    A-DSTsArea: Dynamically-sized types (DSTs)
    T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
    A-codegenArea: Code generation
    on Feb 14, 2021

  8. apiraino commented on Feb 17, 2021

    @apiraino
    apiraino
    on Feb 17, 2021
    Contributor

    Assigning P-medium as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

  9. apiraino commented on Feb 17, 2021

    @apiraino
    apiraino
    on Feb 17, 2021
    Contributor

    @rustbot label -I-prioritize +P-medium

  10. rustbot
    added
    P-mediumMedium priority
    and removed
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Feb 17, 2021
  11. oli-obk
    added
    T-typesRelevant to the types team, which will review and decide on the PR/issue.
    on Oct 21, 2022

  12. nikomatsakis commented on Nov 30, 2022

    @nikomatsakis
    nikomatsakis
    on Nov 30, 2022
    Contributor

    @rustbot labels -T-types

    Discussed in types team on-site -- we think this is a MIR building bug, in any case, not part of T-types.

    Throwing hot 🥔 back to compiler team.

  13. rustbot
    removed
    T-typesRelevant to the types team, which will review and decide on the PR/issue.
    on Nov 30, 2022

  14. RalfJung commented on Dec 13, 2023

    @RalfJung
    RalfJung
    on Dec 13, 2023
    Member

    we think this is a MIR building bug

    It was actually a bug in the codegen backend, introduced when support for packed(N) was added -- the existing code has logic that assumed "if it is packed then alignment is 1", and that was never re-visited.

  15. RalfJung commented on Dec 13, 2023

    @RalfJung
    RalfJung
    on Dec 13, 2023
    Member

    Ah, scratch that -- there was a different related bug (the size_of_val / align_of_val computation for packed structs with dyn Trait tail was wrong), caused by what I said above.

    This here was caused by the codegen backend just never considering packed types when computing the offset of the unsized field. Almost always this happened to work since the unsized field is a slice/str and so its alignment (and therefore its offset) is dynamically known, but when dyn trait tails are involved this starts to go wrong.

    This got fixed by #118540; #118538 will add a test.

  16. matthiaskrgr
    added a commit that references this issue on Dec 14, 2023

    Rollup merge of rust-lang#118538 - RalfJung:size-of-val-comments, r=W…

    b484337
  17. bors
    added 2 commits that reference this issue on Dec 14, 2023

    Auto merge of rust-lang#118538 - RalfJung:size-of-val-comments, r=Waf…

    b6f6800

    Auto merge of rust-lang#118538 - RalfJung:size-of-val-comments, r=Waf…

    1a8afa0
  18. bors
    closed this as completedin 1a8afa0on Dec 14, 2023
  19. github-actions
    added a commit that references this issue on Dec 15, 2023

    Auto merge of #118538 - RalfJung:size-of-val-comments, r=WaffleLapkin

    70db9df
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-DSTsArea: Dynamically-sized types (DSTs)A-codegenArea: Code generationC-bugCategory: This is a bug.I-unsoundIssue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/SoundnessP-mediumMedium priorityT-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions