-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
fix dynamic size/align computation logic for packed types with dyn trait tail #118538
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
r? compiler |
r? compiler |
c51dd07
to
0a2ac74
Compare
☔ The latest upstream changes (presumably #118534) made this pull request unmergeable. Please resolve the merge conflicts. |
if let ty::Adt(def, _) = t.kind() { | ||
if def.repr().packed() { | ||
let unsized_tail = | ||
bx.tcx().struct_tail_with_normalize(field_ty, |ty| ty, || {}); | ||
assert!(matches!(unsized_tail.kind(), ty::Slice(..) | ty::Str)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not right. Won't this ICE for code like this:
#[repr(C, packed)]
struct Packed<T: ?Sized>(u8, core::mem::ManuallyDrop<T>);
let p = Packed(0, core::mem::ManuallyDrop::new(1));
let p: &Packed<usize> = &p;
let p: &Packed<dyn Send> = p;
dbg!(core::mem::size_of_val(p), core::mem::align_of_val(p));
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, if such code is accepted then it would currently just compute the wrong size for that code. An ICE would still be an improvement. ;)
I'll play around with examples that involve ManuallyDrop
to see if there's a bug here, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- There is no justification for why packed types ignore the alignment of their fields. This is correct only because it is impossible to write a packed struct with
dyn Trait
tail, i.e., unsized packed structs always have statically known alignment. (With slices, the size is dynamic but the alignment is static; only withdyn Trait
do we have both dynamic size and alignment.)
I don't think that's correct, given the example above? And either way, packed structs always ignore the alignment of their fields, don't they? I don't think Packed<dyn Send>
's size (or alignment) depends on the alignment of .0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packed structs always ignore the alignment of their fields, don't they?
No they don't. Remember we have #[packed(N)]
. The alignment of the fields in that struct is min(natural_field_align, N)
. For N > 1
, this does depend on the alignment of the field type. And if that's a dyn
type then it needs to be taken into account to compute the offset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the alignment should only be ignored if N = 1
. The bug is trivial to trigger then, yikes: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=80f13045af7f1dd52cf6c79ee2897829 (and obviously it's easy to get unsound by boxing the value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this was a problem since at least stabilization of packed(N)
in 1.33: https://godbolt.org/z/37s44fEKe. I assume when we implemented packed(N)
we forgot to fixup size/align computation logic 💀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes indeed.
We also forgot to fixup the offset computation logic, I fixed that recently in #118540. I thought size computation wouldn't be affected since I was unable to construct a packed struct with a dyn-typed tail, but you managed to bring in that missing piece.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah for offsets, #118540 removed all special cases for packed types, so they are now handled properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the PR to fix this, and added tests.
0a2ac74
to
fe45ba6
Compare
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
46d86aa
to
7e4c427
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@bors r+ |
Marked as
relnotes
TL;DR: we were not computing the size and alignment of packed structs incorrectly, which lead to unsoundness for |
…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.
…iaskrgr Rollup of 3 pull requests Successful merges: - rust-lang#118375 (Add -Zunpretty=stable-mir output test) - rust-lang#118538 (fix dynamic size/align computation logic for packed types with dyn trait tail) - rust-lang#118789 (fix --dry-run when the change-id warning is printed) r? `@ghost` `@rustbot` modify labels: rollup
…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.
💔 Test failed - checks-actions |
@bors retry armhf-gnu: client.read_exact(&mut header) failed with Connection reset by peer (os error 104) |
☀️ Test successful - checks-actions |
Finished benchmarking commit (1a8afa0): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 670.811s -> 670.867s (0.01%) |
Pkgsrc changes: * Adapt checksums and patches. Upstream chnages: Version 1.76.0 (2024-02-08) ========================== Language -------- - [Document Rust ABI compatibility between various types] (rust-lang/rust#115476) - [Also: guarantee that char and u32 are ABI-compatible] (rust-lang/rust#118032) - [Warn against ambiguous wide pointer comparisons] (rust-lang/rust#117758) Compiler -------- - [Lint pinned `#[must_use]` pointers (in particular, `Box<T>` where `T` is `#[must_use]`) in `unused_must_use`.] (rust-lang/rust#118054) - [Soundness fix: fix computing the offset of an unsized field in a packed struct] (rust-lang/rust#118540) - [Soundness fix: fix dynamic size/align computation logic for packed types with dyn Trait tail] (rust-lang/rust#118538) - [Add `$message_type` field to distinguish json diagnostic outputs] (rust-lang/rust#115691) - [Enable Rust to use the EHCont security feature of Windows] (rust-lang/rust#118013) - [Add tier 3 {x86_64,i686}-win7-windows-msvc targets] (rust-lang/rust#118150) - [Add tier 3 aarch64-apple-watchos target] (rust-lang/rust#119074) - [Add tier 3 arm64e-apple-ios & arm64e-apple-darwin targets] (rust-lang/rust#115526) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries --------- - [Add a column number to `dbg!()`] (rust-lang/rust#114962) - [Add `std::hash::{DefaultHasher, RandomState}` exports] (rust-lang/rust#115694) - [Fix rounding issue with exponents in fmt] (rust-lang/rust#116301) - [Add T: ?Sized to `RwLockReadGuard` and `RwLockWriteGuard`'s Debug impls.] (rust-lang/rust#117138) - [Windows: Allow `File::create` to work on hidden files] (rust-lang/rust#116438) Stabilized APIs --------------- - [`Arc::unwrap_or_clone`] (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.unwrap_or_clone) - [`Rc::unwrap_or_clone`] (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.unwrap_or_clone) - [`Result::inspect`] (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect) - [`Result::inspect_err`] (https://doc.rust-lang.org/stable/std/result/enum.Result.html#method.inspect_err) - [`Option::inspect`] (https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.inspect) - [`type_name_of_val`] (https://doc.rust-lang.org/stable/std/any/fn.type_name_of_val.html) - [`std::hash::{DefaultHasher, RandomState}`] (https://doc.rust-lang.org/stable/std/hash/index.html#structs) These were previously available only through `std::collections::hash_map`. - [`ptr::{from_ref, from_mut}`] (https://doc.rust-lang.org/stable/std/ptr/fn.from_ref.html) - [`ptr::addr_eq`](https://doc.rust-lang.org/stable/std/ptr/fn.addr_eq.html) Cargo ----- See [Cargo release notes] (https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-176-2024-02-08). Rustdoc ------- - [Don't merge cfg and doc(cfg) attributes for re-exports] (rust-lang/rust#113091) - [rustdoc: allow resizing the sidebar / hiding the top bar] (rust-lang/rust#115660) - [rustdoc-search: add support for traits and associated types] (rust-lang/rust#116085) - [rustdoc: Add highlighting for comments in items declaration] (rust-lang/rust#117869) Compatibility Notes ------------------- - [Add allow-by-default lint for unit bindings] (rust-lang/rust#112380) This is expected to be upgraded to a warning by default in a future Rust release. Some macros emit bindings with type `()` with user-provided spans, which means that this lint will warn for user code. - [Remove x86_64-sun-solaris target.] (rust-lang/rust#118091) - [Remove asmjs-unknown-emscripten target] (rust-lang/rust#117338) - [Report errors in jobserver inherited through environment variables] (rust-lang/rust#113730) This [may warn](rust-lang/rust#120515) on benign problems too. - [Update the minimum external LLVM to 16.] (rust-lang/rust#117947) - [Improve `print_tts`](rust-lang/rust#114571) This change can break some naive manual parsing of token trees in proc macro code which expect a particular structure after `.to_string()`, rather than just arbitrary Rust code. - [Make `IMPLIED_BOUNDS_ENTAILMENT` into a hard error from a lint] (rust-lang/rust#117984) - [Vec's allocation behavior was changed when collecting some iterators] (rust-lang/rust#110353) Allocation behavior is currently not specified, nevertheless changes can be surprising. See [`impl FromIterator for Vec`] (https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#impl-FromIterator%3CT%3E-for-Vec%3CT%3E) for more details. - [Properly reject `default` on free const items] (rust-lang/rust#117818)
This logic was never updated to support
packed(N)
whereN > 1
, and it turns out to be wrong for that case.Fixes #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.