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

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

Closed
mahkoh opened this issue Jan 11, 2021 · 9 comments · Fixed by #118538
Closed

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

mahkoh opened this issue Jan 11, 2021 · 9 comments · Fixed by #118538
Labels
A-codegen Area: Code generation A-DSTs Area: Dynamically-sized types (DSTs) 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

@mahkoh
Copy link
Contributor

mahkoh commented Jan 11, 2021

Execute with Miri:

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

The problem is that

// Bump the unaligned offset up to the appropriate alignment
let offset = round_up_const_value_to_alignment(bx, unaligned_offset, unsized_align);
and
(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.

@mahkoh mahkoh added the C-bug Category: This is a bug. label Jan 11, 2021
@jonas-schievink jonas-schievink added A-miri Area: The miri tool requires-nightly This issue requires a nightly compiler in some way. A-const-eval Area: Constant evaluation (MIR interpretation) labels Jan 11, 2021
@mahkoh
Copy link
Contributor Author

mahkoh commented Feb 10, 2021

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

@ghost

This comment has been minimized.

@rustbot

This comment has been minimized.

@rustbot

This comment has been minimized.

@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. and removed A-const-eval Area: Constant evaluation (MIR interpretation) A-miri Area: The miri tool requires-nightly This issue requires a nightly compiler in some way. labels Feb 14, 2021
@LeSeulArtichaut LeSeulArtichaut added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness A-DSTs Area: Dynamically-sized types (DSTs) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-codegen Area: Code generation labels Feb 14, 2021
@apiraino
Copy link
Contributor

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

@apiraino
Copy link
Contributor

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 17, 2021
@oli-obk oli-obk added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Oct 21, 2022
@nikomatsakis
Copy link
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.

@rustbot rustbot removed the T-types Relevant to the types team, which will review and decide on the PR/issue. label Nov 30, 2022
@RalfJung
Copy link
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.

@RalfJung
Copy link
Member

RalfJung commented Dec 13, 2023

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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 14, 2023
…affleLapkin

fix dynamic size/align computation logic for packed types with dyn trait tail

This logic was never updated to support `packed(N)` where `N > 1`, and it turns out to be wrong for that case.

Fixes rust-lang#80925

`@bjorn3` I have not looked at cranelift; I assume it basically copied the size-of-val logic and hence could use much the same patch.
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 14, 2023
…fleLapkin

fix dynamic size/align computation logic for packed types with dyn trait tail

This logic was never updated to support `packed(N)` where `N > 1`, and it turns out to be wrong for that case.

Fixes rust-lang#80925

`@bjorn3` I have not looked at cranelift; I assume it basically copied the size-of-val logic and hence could use much the same patch.
@bors bors closed this as completed in 1a8afa0 Dec 14, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Dec 15, 2023
fix dynamic size/align computation logic for packed types with dyn trait tail

This logic was never updated to support `packed(N)` where `N > 1`, and it turns out to be wrong for that case.

Fixes rust-lang/rust#80925

`@bjorn3` I have not looked at cranelift; I assume it basically copied the size-of-val logic and hence could use much the same patch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-DSTs Area: Dynamically-sized types (DSTs) 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.

8 participants