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

Tracking issue for panics in mem::uninitialized/zeroed #66151

Closed
1 of 3 tasks
RalfJung opened this issue Nov 6, 2019 · 53 comments · Fixed by #101061
Closed
1 of 3 tasks

Tracking issue for panics in mem::uninitialized/zeroed #66151

RalfJung opened this issue Nov 6, 2019 · 53 comments · Fixed by #101061
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Nov 6, 2019

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 MaybeUninit and 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::Multiple enums, which currently we do not just to stay conservative.
@RalfJung RalfJung added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 6, 2019
@RalfJung
Copy link
Member Author

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 uninitialized enum (with more than one variant in its layout) is UB. It has to be UB because the point of the validity invariant of an enum is to make sure the discriminant is always valid, and for uninitialized memory the discriminant test would raise UB.

Cc @gnzlbg @alexcrichton

@valarauca
Copy link

Will the unstable methods associated with MaybeUninit be 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 MaybeUninit API unstable/nightly only this change feels premature. There's a lot of patterns which simply are not allowed in non-nightly MaybeUninit, and having it suggested as an upgrade path from mem::uninitialized/zeroed feels a bit like a bad joke.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 1, 2020

@valarauca MaybeUninit has been stable for quite a while. The remaining unstable methods are just convenience, they do not limit functionality. For example, instead of get_mut (just renamed to assume_init_mut) you can do &mut *x.as_mut_ptr(). The safety rules are the same as for assume_init_mut: the data must be already fully initialized before you create a mutable reference.

Which patterns specifically are you talking about?

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 27, 2020
…petrochenkov

might_permit_raw_init: also check aggregate fields

This is the next step for rust-lang#66151: when doing `mem::zeroed`/`mem::uninitialized`, also recursively check fields of aggregates (except for arrays) for whether they permit zero/uninit initialization.
@Aaron1011
Copy link
Member

Aaron1011 commented Oct 21, 2020

In #77970 (comment), the panic in mem::uninitialized ended 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 Rc and Arc will abort in the face of a pathological number of clone() 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
Copy link
Member

From #66059 (comment):

It is not entirely clear whether, when a violation of validity is detected, we should panic or abort. For now, we panic, which has the advantage of printing a backtrace where possible. However, unsafe could could be surprised by the panic and not expect unwinding, and that has lead to segfaults on crater. The issue with aborting, however, is that we have no good way to print a backtrace.

@Aaron1011
Copy link
Member

There's ongoing work to make Backtrace available in core: #72981 (comment)

Once that becomes available on nightly, we could use it internally to attempt to print a backtrace, and then abort.

@RalfJung
Copy link
Member Author

Once that becomes available on nightly, we could use it internally to attempt to print a backtrace, and then abort.

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
Copy link
Contributor

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
Copy link
Member Author

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.^^

@nico-abram
Copy link
Contributor

Yes! I can, assuming there is no set timeline required

If I understand correctly, the change would be replacing this comment with a similar check to the FieldsShape::Arbitrary one but hardcoded for index 0, and an or check for FieldsShape::Array { count, .. } count == 0 (Like there is for Abi::Vector). Is that correct?

The change would just be a straight up PR against this repository, and someone authorized would schedule a crater run?

Finally, does analyzing the crater report mean looking into the errors and checking why they happen, analyzing the ecosystem-wide impact of breakage, and contacting maintainers with bug reports?

@RalfJung
Copy link
Member Author

Yes! I can, assuming there is no set timeline required

No timeline required. :) Thanks!

If I understand correctly, the change would be replacing this comment with a similar check to the FieldsShape::Arbitrary one but hardcoded for index 0, and an or check for FieldsShape::Array { count, .. } count == 0 (Like there is for Abi::Vector). Is that correct?

Yes, that sounds right: first determine if the array has at least one element, then check the layout of the element type.

The Array variant has a count field for the element count, as you noted.

The change would just be a straight up PR against this repository, and someone authorized would schedule a crater run?

Exactly. The PR should also extend the test "src/test/ui/intrinsics/panic-uninitialized-zeroed.rs" to make sure the check behaves as expected.

Finally, does analyzing the crater report mean looking into the errors and checking why they happen, analyzing the ecosystem-wide impact of breakage, and contacting maintainers with bug reports?

Basically, yes. You can see my analysis for the last change here. As you can see from the bingbacks above that comment, there were quite a few bugreports to be made for this one.

I believe that the fallout will be smaller this time, since I think that we covered most of the "heavy hitters" last time and they wouldn't show up as regressions any more now (since they are already broken on stable). However, there might well be unexpected things showing up (like vast numbers of people still using outdated smallvec, which is known to be broken).

@RalfJung
Copy link
Member Author

RalfJung commented Nov 22, 2020

Here's a good testcase for an enum that we should detect is UB for mem::uninitialized: #79282 (comment).

EDIT: Ah, we already do.

@RalfJung
Copy link
Member Author

Another possible avenue that has been discussed is to give up on trying to fix old code (or getting people to fix it, by raising panics) and instead switch to mitigation mode, by reducing the amount of UB. We could make mem::uninitialized synthesize a bitpattern that is actually valid at the given type (and keep the panic if the type is uninhabited). This would not fully fix UB due to dereferenceable assumptions for references, but it makes it much less likely that this will lead to problems.

That said, because it would not fully fix UB, I am not sure whether we should rely on this mitigation, or still also go with panics for the cases where we can get away with that.

@5225225
Copy link
Contributor

5225225 commented Jul 12, 2022

Sounds good, but I'd want to not do that if the user is using memory sanitizer or miri, otherwise those tools will get worse at detecting UB. And that would also hide branching on undef if you run valgrind, but that seems more niche. We have those cfgs available as far as I know, and the std is recompiled when you use either of them.

char would still have its validity invariant broken, and there's definitely code out there that does that, but we either don't tell LLVM char's valid range, or we do but only sometimes. But filling with 0x01 seems like a decent and cheap solution.

And I think it's still worth pushing for panics, I definitely think we shouldn't rely on that. But as a "well, it makes it much less likely that LLVM will notice we're lying to it for the time being", eh... I'm fine with that, but not if it means we never get rid of that hack.

@RalfJung
Copy link
Member Author

Sounds good, but I'd want to not do that if the user is using memory sanitizer or miri, otherwise those tools will get worse at detecting UB.

Yeah good point; we should probably cfg this. Like for example:

pub unsafe fn uninitialized<T>() -> T {
    // SAFETY: the caller must guarantee that an uninitialized value is valid for `T`.
    unsafe {
        intrinsics::assert_uninit_valid::<T>();
        #[allow(unused_mut)]
        let mut val = MaybeUninit::uninit();

        // Fill memory with 0x01, as a mitigation for old code that uses
        // this function on bool and nonnull types. But don't do this
        // if we actively want to detect UB.
        #[cfg(not(any(miri, sanitize = "???")))]
        val.as_mut_ptr().write_bytes(0x01, 1);

        val.assume_init()
    }
}

char would still have its validity invariant broken, and there's definitely code out there that does that, but we either don't tell LLVM char's valid range, or we do but only sometimes. But filling with 0x01 seems like a decent and cheap solution.

Ah yeah, there's no single byte that works for char and bool and nonnull types.

@RalfJung
Copy link
Member Author

PR for the 0x01 filling is up at #99182.

@saethlin
Copy link
Member

While we cannot synthesize a known-valid value for references in general, we can for slices. I think that would remove old versions of hyper from crater at least. And if people want to continue using those that's fine, they can upgrade at some point and get their perf back.

bors added a commit to rust-lang-ci/rust that referenced this issue Jul 28, 2022
mem::uninitialized: mitigate many incorrect uses of this function

Alternative to rust-lang#98966: fill memory with `0x01` rather than leaving it uninit. This is definitely bitewise valid for all `bool` and nonnull types, and also those `Option<&T>` that we started putting `noundef` on. However it is still invalid for `char` and some enums, and on references the `dereferenceable` attribute is still violated, so the generated LLVM IR still has UB -- but in fewer cases, and `dereferenceable` is hopefully less likely to cause problems than clearly incorrect range annotations.

This can make using `mem::uninitialized` a lot slower, but that function has been deprecated for years and we keep telling everyone to move to `MaybeUninit` because it is basically impossible to use `mem::uninitialized` correctly. For the cases where that hasn't helped (and all the old code out there that nobody will ever update), we can at least mitigate the effect of using this API. Note that this is *not* in any way a stable guarantee -- it is still UB to call `mem::uninitialized::<bool>()`, and Miri will call it out as such.

This is somewhat similar to rust-lang#87032, which proposed to make `uninitialized` return a buffer filled with 0x00. However
- That PR also proposed to reduce the situations in which we panic, which I don't think we should do at this time.
- The 0x01 bit pattern means that nonnull requirements are satisfied, which (due to references) is the most common validity invariant.

`@5225225` I hope I am using `cfg(sanitize)` the right way; I was not sure for which ones to test here.
Cc rust-lang#66151
Fixes rust-lang#87675
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Aug 27, 2022
…alfJung

Strengthen invalid_value lint to forbid uninit primitives, adjust docs to say that's UB

For context: rust-lang#66151 (comment)

This does not make it a FCW, but it does explicitly state in the docs that uninit integers are UB.

This also doesn't affect any runtime behavior, uninit u32's will still successfully be created through mem::uninitialized.
@RalfJung
Copy link
Member Author

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 making mem::uninitialized fill the returned buffer with 0x01. 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 to aligned, nonnull, or range annotations; there is some UB left due to dereferenceable annotations, 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.

@5225225
Copy link
Contributor

5225225 commented Aug 27, 2022

In particular we do not have to recurse into arrays, since from what I could see, we never emit metadata for array element types.

Could we add a codegen test to ensure that we don't do this, leaving a comment back to this issue and saying that we must start recursing into arrays if we start emitting nonnull on a [fn(); 1] return? (Or, start recursing into arrays, but only for 1-length arrays, under the assumption that any longer won't cause issues since they're returned differently)

It doesn't need to be done before #101061 lands, but I think it would be good to add at some point. I'll have a shot at it once I'm done with cleaning up the FCW PR.

@RalfJung
Copy link
Member Author

Yes, the PR that finishes up the panics and closes this issue (the PR after #101061) should add such a codegen test.

bors added a commit to rust-lang-ci/rust that referenced this issue Aug 30, 2022
…fJung

Strengthen invalid_value lint to forbid uninit primitives, adjust docs to say that's UB

For context: rust-lang#66151 (comment)

This does not make it a FCW, but it does explicitly state in the docs that uninit integers are UB.

This also doesn't affect any runtime behavior, uninit u32's will still successfully be created through mem::uninitialized.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Sep 8, 2022
Strengthen invalid_value lint to forbid uninit primitives, adjust docs to say that's UB

For context: rust-lang/rust#66151 (comment)

This does not make it a FCW, but it does explicitly state in the docs that uninit integers are UB.

This also doesn't affect any runtime behavior, uninit u32's will still successfully be created through mem::uninitialized.
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
mem::uninitialized: mitigate many incorrect uses of this function

Alternative to rust-lang/rust#98966: fill memory with `0x01` rather than leaving it uninit. This is definitely bitewise valid for all `bool` and nonnull types, and also those `Option<&T>` that we started putting `noundef` on. However it is still invalid for `char` and some enums, and on references the `dereferenceable` attribute is still violated, so the generated LLVM IR still has UB -- but in fewer cases, and `dereferenceable` is hopefully less likely to cause problems than clearly incorrect range annotations.

This can make using `mem::uninitialized` a lot slower, but that function has been deprecated for years and we keep telling everyone to move to `MaybeUninit` because it is basically impossible to use `mem::uninitialized` correctly. For the cases where that hasn't helped (and all the old code out there that nobody will ever update), we can at least mitigate the effect of using this API. Note that this is *not* in any way a stable guarantee -- it is still UB to call `mem::uninitialized::<bool>()`, and Miri will call it out as such.

This is somewhat similar to rust-lang/rust#87032, which proposed to make `uninitialized` return a buffer filled with 0x00. However
- That PR also proposed to reduce the situations in which we panic, which I don't think we should do at this time.
- The 0x01 bit pattern means that nonnull requirements are satisfied, which (due to references) is the most common validity invariant.

`@5225225` I hope I am using `cfg(sanitize)` the right way; I was not sure for which ones to test here.
Cc rust-lang/rust#66151
Fixes rust-lang/rust#87675
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
Strengthen invalid_value lint to forbid uninit primitives, adjust docs to say that's UB

For context: rust-lang/rust#66151 (comment)

This does not make it a FCW, but it does explicitly state in the docs that uninit integers are UB.

This also doesn't affect any runtime behavior, uninit u32's will still successfully be created through mem::uninitialized.
@bors bors closed this as completed in ab88c19 Oct 5, 2022
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
Strengthen invalid_value lint to forbid uninit primitives, adjust docs to say that's UB

For context: rust-lang/rust#66151 (comment)

This does not make it a FCW, but it does explicitly state in the docs that uninit integers are UB.

This also doesn't affect any runtime behavior, uninit u32's will still successfully be created through mem::uninitialized.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Strengthen invalid_value lint to forbid uninit primitives, adjust docs to say that's UB

For context: rust-lang/rust#66151 (comment)

This does not make it a FCW, but it does explicitly state in the docs that uninit integers are UB.

This also doesn't affect any runtime behavior, uninit u32's will still successfully be created through mem::uninitialized.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API 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