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

Make uninit checks stricter but avoid issues for old hyper #99389

Closed
wants to merge 8 commits into from

Conversation

5225225
Copy link
Contributor

@5225225 5225225 commented Jul 17, 2022

This makes the checks for std::mem::uninitialized stricter, but tries to avoid making hyper panic.

hyper versions 0.11 to early 0.14 all make slices inside arrays, so we don't panic on slices inside arrays. We still will panic on any other reference, even to ZSTs.

No change was made to the std::mem::zeroed checks.

We still consider uninit primitives that have a full range to be valid, as well as &[u8] (more generally, any type with an align of 1) and &str but only inside arrays.

I ran these on some known bad hyper crates, and they all passed, whereas a known bad crossbeam crate using mem::zeroed failed. Which is what I expected.

See: #66151

Once the code's been reviewed and we're happy with the amount of checks this is doing, this will definitely need a crater run. I'm expecting hopefully less than the previous run, mostly crossbeam but maybe with some extra surprises.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 17, 2022
@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2022

@bors try

@bors
Copy link
Contributor

bors commented Jul 18, 2022

⌛ Trying commit 168fa786e9c38469881a8490b7752e75de90f1b1 with merge af445596a53bb2f55a69322f552f9f231e2522ad...

@5225225
Copy link
Contributor Author

5225225 commented Jul 18, 2022

Also, this is strictly speaking only not LLVM ub for mem::uninit if/once #99182 gets merged, but no need to block this on that getting in, it's currently LLVM ub for hyper to do what its doing.

Also gonna cc @RalfJung if you want to have a look at this. I did mostly follow what you described, but also figured the const eval checks for mem::zeroed would be fine since not many people misuse that, I don't expect us to be breaking a lot more code since the only thing that breaks is someone doing Result<NonZeroT, _> or any other enum where the 0 variant doesn't allow being left zeroed.

Plus it exercises the const eval check code path by default, so no surprises if/when we go to turn it on everywhere. Which is probably a long way off considering how much code makes uninit ints, but I'm optimistic we can do it Eventually.

@bors
Copy link
Contributor

bors commented Jul 18, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 18, 2022
///
/// This code is intentionally conservative, and will not detect
/// * making uninitialized types who have a full valid range (ints, floats, raw pointers)
/// * uninit invalid &[T] where T has align 1 (only inside arrays)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error: unresolved link to `T`
--> compiler/rustc_middle/src/ty/layout.rs:3491:24
|
3491|/// * uninit invalid &[T] where T has align 1 (only inside arrays)
| ^no item named `T` in scope
|
= note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`
= help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`

I guess you need to put these into backticks ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Yeah, that's the second time I messed up a docs thing, I really gotta get into the habit of checking x.py doc :)

Will fix now, sorry about that :)

(How expensive would it be for PR CI to run docs? I don't know if I'm just the only one hitting this or if it would be worth adding it to PR CI.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workspace: /home/jess/src/rust/Cargo.toml
   Compiling serde v1.0.125
   Compiling rand v0.8.5
   Compiling tempfile v3.2.0
   Compiling serde_json v1.0.59
   Compiling lint-docs v0.1.0 (/home/jess/src/rust/src/tools/lint-docs)
    Finished release [optimized] target(s) in 6.08s
warning: the code example in lint `unfulfilled_lint_expectations` in /home/jess/src/rust/compiler/rustc_lint_defs/src/builtin.rs failed to generate the expected output: did not find lint `unfulfilled_lint_expectations` in output of example, got:

error[E0554]: `#![feature]` may not be used on the beta release channel
 --> lint_example.rs:1:1
  |
1 | #![feature(lint_reasons)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^


error: aborting due to previous error


For more information about this error, try `rustc --explain E0554`.

warning: the code example in lint `unused_allocation` in /home/jess/src/rust/compiler/rustc_lint/src/unused.rs failed to generate the expected output: did not find lint `unused_allocation` in output of example, got:

error[E0554]: `#![feature]` may not be used on the beta release channel
 --> lint_example.rs:1:1
  |
1 | #![feature(box_syntax)]
  | ^^^^^^^^^^^^^^^^^^^^^^^


error: aborting due to previous error


For more information about this error, try `rustc --explain E0554`.

Rustbook (x86_64-unknown-linux-gnu) - rustc
Rustbook (x86_64-unknown-linux-gnu) - cargo

known issue? I saw that on master doc runs too. In any case, I couldn't see any failures that seemed to be caused by me, so.... It's probably fine now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How expensive would it be for PR CI to run docs?

Not too terribly long, but for it to be reliable we can't use stage 0 rustdoc, which means we have to add it to the LLVM-12 builder, which is already kinda slow :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh also do you mind opening an issue for that feature gate error? We should fix it, but we really should make sure it's tested somewhere and not just ignored.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, lint reasons have recently stabilized so their error should be gone upon rebase. The box_syntax feature gate is being removed by #97774 but that PR is kinda stalled right now, mostly because the lint is warn by default but has a bunch of false positives and widening it to Box::new would lead to exposing it to a wide audience.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 18, 2022

@bors try

@bors
Copy link
Contributor

bors commented Jul 18, 2022

⌛ Trying commit 6ca481ecf3ca04e913712af16e07f0fc9ba91957 with merge e468a8b2c03d14f4cc595ff11f731cc44d6af743...

@RalfJung
Copy link
Member

RalfJung commented Jul 18, 2022

I did mostly follow what you described, but also figured the const eval checks for mem::zeroed would be fine since not many people misuse that

Well, except for old crossbeam...
so I think making the mem::zeroed check stricter should probably be a separate change.

@bors
Copy link
Contributor

bors commented Jul 18, 2022

☀️ Try build successful - checks-actions
Build commit: e468a8b2c03d14f4cc595ff11f731cc44d6af743 (e468a8b2c03d14f4cc595ff11f731cc44d6af743)

@RalfJung
Copy link
Member

Is the goal of this PR that, assuming #99182 lands, we never have LLVM UB from mem::uninit? If yes then the may_allow_raw_init function should be documented and commented accordingly. (But this can wait until after crater, when we are sure we can land this.)

@RalfJung RalfJung changed the title Make uninit checks stricter but avoid issues for old hyper, fully use const checks for mem::zeroed Make uninit checks stricter but avoid issues for old hyper Jul 19, 2022
"attempted to zero-initialize type `LR_NonZero`, which is invalid"
test_panic_msg(
|| mem::uninitialized::<(&[u8], &str, &())>(),
"attempted to leave type `(&[u8], &str, &())` uninitialized, which is invalid"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one we could accept, if we wanted, by accepting all references to sized align-1-ZST... doesn't seem very important though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though allowing &[u8] and rejecting &() feels rather arbitrary so we should probably allow both then?

Copy link
Contributor Author

@5225225 5225225 Jul 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could do, but I figure since not many people are actually making a reference to a ZST, no harm in forbidding it now.

The changes here are rather arbitrary in their nature, we're only not panicking on the bits that we can't panic on just yet, while making the checks stricter where we can.

Making an uninit [bool; 1] will start to panic after this change, even if that's still not LLVM UB if we 0x01 fill, because there's not really a widely used crate that does that (inb4 some crate started doing that since the last crater run :p).

Hyper doesn't make uninit refs to sized ZSTs, so I don't think we need to allow them here, even if it is somewhat inconsistent to forbid them.

I'm fine with ignoring &() as well, I just don't feel it's needed.

@RalfJung
Copy link
Member

Okay, I think this is ready for crater.
@bors try

@5225225
Copy link
Contributor Author

5225225 commented Aug 1, 2022

Okay, went through all of them. I didn't create an issue for the ones that I couldn't reproduce / worked fine with a fresh lockfile, but I reported most of them upstream.

Still need to edit the PR description before this is merged to describe what was done but I figure I might as well mark this ready. PR desc edited

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 1, 2022
@5225225
Copy link
Contributor Author

5225225 commented Aug 9, 2022

#98862 (comment) gave a go-ahead to introduce a FCW. Might be smart to do that before this, so people have some warning period. (Though that would be for all uses of mem::uninitialized, and not just the ones we're panicking on here)

@5225225
Copy link
Contributor Author

5225225 commented Aug 15, 2022

Gonna mark this as blocked on #100342. Once/if the FCW goes in and people get a chance to bump dependencies, it'll be worth revisiting this, but as stated, I think it'd be unwise to get this in first.

@rustbot label +S-blocked -S-waiting-on-review

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2022
@RalfJung
Copy link
Member

#101061 is an alternative to this PR that relaxes the panics in light of mem::uninitiailized now filling its return value with 0x01. That PR + just recursing through arrays should still work with hyper, and keeps the logic a lot simpler than this PR.

@RalfJung
Copy link
Member

Actually, this makes me wonder -- is there even LLVM UB from mem::uninitialized on arrays? We never use scalar layout for arrays, so I don't think we emit any metadata that would cause LLVM UB from an uninitialized unused array. In that sense there seems to be no good reason to break old crossbeam.

Maybe stopping at arrays is actually fine if all we are aiming for is avoid LLVM UB -- and the only thing we still should strengthen are the checks for Variants::Single?

@bors
Copy link
Contributor

bors commented Oct 4, 2022

☔ The latest upstream changes (presumably #102622) made this pull request unmergeable. Please resolve the merge conflicts.

@5225225
Copy link
Contributor Author

5225225 commented Feb 26, 2023

Closing this seeing that #101061 was implemented, this is not really needed anymore

@5225225 5225225 closed this Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.