Repository navigation
Tracking issue for panics in mem::uninitialized/zeroed #66151
Description
With #66059, mem::uninitialized and mem::zeroed dynamically detect some misuses (such as mem::zeroed::<&T>() or mem::uninitialized::<bool>()) and panic instead of causing UB. Also see this summary for the original FCP. But the check is conservative for now to reduce breakage. This is to track strengthening the check.
- We should recursively check the fields of structs and tuples. This is happening in might_permit_raw_init: also check aggregate fields #71274.
- We should check inside arrays, too. We currently do not because smallvec < 0.6.13 didn't use
MaybeUninitand thus triggers this check (if the type inside the array has invalid bit patterns). Other widely-used crates that trigger the panic as of Dec 2021:- hyper (fixed since 0.14.12)
- crossbeam-queue 0.2.1, crossbeam-channel 0.3.9, crossbeam 0.2.12 (these are ancient; current versions are fine AFAIK)
- We should check
Variant::Multipleenums, which currently we do not just to stay conservative.
Activity
RalfJung commentedon Mar 14, 2020
git2-rs needed some adjustment to work even with the minimal checks that have already landed, which is good enough for now but even those adjustments will nut suffice once we properly check enums. The reason for this is that ctest uses
mem::uninitialized()to test that the bindings are declared correctly, and an uninitializedenum(with more than one variant in its layout) is UB. It has to be UB because the point of the validity invariant of anenumis to make sure the discriminant is always valid, and for uninitialized memory the discriminant test would raise UB.Cc @gnzlbg @alexcrichton
valarauca commentedon Aug 31, 2020
Will the unstable methods associated with
MaybeUninitbe stabilized along with or as part of this change?I agree this is a positive change for the language, but with a large amount of the
MaybeUninitAPI unstable/nightly only this change feels premature. There's a lot of patterns which simply are not allowed in non-nightlyMaybeUninit, and having it suggested as an upgrade path frommem::uninitialized/zeroedfeels a bit like a bad joke.RalfJung commentedon Sep 1, 2020
@valarauca
MaybeUninithas been stable for quite a while. The remaining unstable methods are just convenience, they do not limit functionality. For example, instead ofget_mut(just renamed toassume_init_mut) you can do&mut *x.as_mut_ptr(). The safety rules are the same as forassume_init_mut: the data must be already fully initialized before you create a mutable reference.Which patterns specifically are you talking about?
Aaron1011 commentedon Oct 21, 2020
In #77970 (comment), the panic in
mem::uninitializedended up causing a segfault, since unwinding caused some unsafe drop code to run in an invalid state.I think it might make sense to force an abort (with a message), rather than use whatever panic strategy is configured. There's some precedent for this - both
RcandArcwill abort in the face of a pathological number ofclone()calls (which can actually be triggered from safe code). Since this panic occurs when unsafe code has already triggered undefined behavior, catching the panic or unwinding will probably just make things worse.One downside is that this test would become more complicated, since we can longer repeatedly trigger the panic in a single test.
scottmcm commentedon Oct 21, 2020
From #66059 (comment):
Aaron1011 commentedon Oct 21, 2020
There's ongoing work to make
Backtraceavailable incore: #72981 (comment)Once that becomes available on nightly, we could use it internally to attempt to print a backtrace, and then abort.
RalfJung commentedon Oct 21, 2020
Yes, that would certainly be worth trying! I ran into such unexpected-unwind segfaults before when analyzing crater runs for this change and they are indeed quite hard to debug.
nico-abram commentedon Nov 22, 2020
It's been about a year since this issue was created. Has enough time passed that PRs adding the check for arrays and enums would be merged?
RalfJung commentedon Nov 22, 2020
We just landed this check for structs and tuples on the stable branch with Rust 1.48. The fallout from that is still rippling through the ecosystem, e.g. here. So I feel we should wait a bit before retching this up another level.
@nico-abram would you be interested in preparing such a change for arrays and seeing it through (in particular analyzing the crater report)? The code change for arrays is trivial, and I can provide mentoring, but last time analyzing the crater fallout took forever so I won't do this again anytime soon.^^
51 remaining items
Auto merge of rust-lang#99182 - RalfJung:mitigate-uninit, r=scottmcm
Rollup merge of rust-lang#98919 - 5225225:stricter-invalid-value, r=R…
RalfJung commentedon Aug 27, 2022
I think we should slightly change track here, and rather than trying to detect and root out all Rust-level UB (which at this point I consider a failed experiment), we should do what we can do mitigate the LLVM-level UB that can be caused by
mem::uninitialized, while minimizing the pain for users that just followed the official advice they were given 5 years ago. #99182 was a first step down that road, by makingmem::uninitializedfill the returned buffer with0x01. Now in #101061 I am proposing to panic in fewer cases than before due to the fact that LLVM UB is already mitigated.Based on that, if #101061 lands, my plan would be to strengthen the checks to also recurse below
Variants::Single, and then close this issue and call it done -- to my knowledge, that mitigates all of the LLVM UB due toaligned,nonnull, orrangeannotations; there is some UB left due todereferenceableannotations, but those are (to my knowledge) very unlikely to actually cause issues if the data is truly unused. In particular we do not have to recurse into arrays, since from what I could see, we never emit metadata for array element types.RalfJung commentedon Aug 27, 2022
Yes, the PR that finishes up the panics and closes this issue (the PR after #101061) should add such a codegen test.
Auto merge of rust-lang#98919 - 5225225:stricter-invalid-value, r=Ral…
Auto merge of #98919 - 5225225:stricter-invalid-value, r=RalfJung
Auto merge of #99182 - RalfJung:mitigate-uninit, r=scottmcm
Auto merge of #98919 - 5225225:stricter-invalid-value, r=RalfJung
Rollup merge of rust-lang#101061 - RalfJung:panic-on-uninit, r=oli-obk
Auto merge of #98919 - 5225225:stricter-invalid-value, r=RalfJung
Auto merge of #98919 - 5225225:stricter-invalid-value, r=RalfJung
Auto merge of #99182 - RalfJung:mitigate-uninit, r=scottmcm