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

Get rid of uses of mem::uninitialized #62397

Closed
4 of 6 tasks
RalfJung opened this issue Jul 5, 2019 · 16 comments · Fixed by #67507
Closed
4 of 6 tasks

Get rid of uses of mem::uninitialized #62397

RalfJung opened this issue Jul 5, 2019 · 16 comments · Fixed by #67507
Labels
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 Jul 5, 2019

007d87f added a bunch of allow(deprecated) because there are still a few places where we use mem::uninitialized. These should all be fixed, to remove the allow (and hopefully prevent other deprecated methods from being used there).

  • SGX
  • libterm
  • librustc_codegen_llvm
  • std::io::util
  • CloudAbi layout tests (src/libstd/sys/cloudabi/abi/cloudabi.rs)
  • rustc tests

Cc @EdSchouten for CloudAbi, @jethrogb for SGX.

@RalfJung RalfJung added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Jul 5, 2019
Centril added a commit to Centril/rust that referenced this issue Jul 5, 2019
…lacrum

Remove last use of mem::uninitialized in SGX

See rust-lang#62397
Centril added a commit to Centril/rust that referenced this issue Jul 5, 2019
…lacrum

Remove last use of mem::uninitialized in SGX

See rust-lang#62397
Centril added a commit to Centril/rust that referenced this issue Jul 6, 2019
…xcrichton

Remove some uses of mem::uninitialized

cc rust-lang#62397
r? @RalfJung
Centril added a commit to Centril/rust that referenced this issue Jul 6, 2019
…xcrichton

Remove some uses of mem::uninitialized

cc rust-lang#62397
r? @RalfJung
Centril added a commit to Centril/rust that referenced this issue Jul 6, 2019
…xcrichton

Remove some uses of mem::uninitialized

cc rust-lang#62397
r? @RalfJung
Centril added a commit to Centril/rust that referenced this issue Jul 6, 2019
…xcrichton

Remove some uses of mem::uninitialized

cc rust-lang#62397
r? @RalfJung
@JulianGindi
Copy link

JulianGindi commented Jul 11, 2019

Going to take a stab at addressing this issue if that's all right.

@JulianGindi
Copy link

Actually looks like some of this was already handled.

@RalfJung
Copy link
Member Author

The SGX and libterm and codegen ones have been handled. CloudAbi and std::io::util remain open as far as I can see.

@JulianGindi
Copy link

CloudAbi and std::io::util remain open as far as I can see.

Ah, gotcha. Will take a crack at it. Thanks!

@jethrogb
Copy link
Contributor

And what about all the tests?

@JulianGindi
Copy link

JulianGindi commented Jul 12, 2019

And what about all the tests?

Good question. I'm a bit less confident with figuring out how to handle those tests. I removed mem::uninitialized from CloudAbi and std::io::util and tests still pass. I can submit a PR with just that change, but I want to remove it from the tests as well if that's appropriate.

My progress can be viewed here for reference/guidance.

@RalfJung
Copy link
Member Author

I'm less worried about the tests TBH. The affected ones fairly deliberately cause UB. They can probably be fixed by replacing it with MaybeUninit::uninit().assume_init() and adding a comment that we cause UB here but for some reason we still want to test this.

@JulianGindi IMO it anyway makes sense to separate the test changes from the others as they are unrelated, so why don't you make a PR with what you got?

@JulianGindi
Copy link

Sounds good @RalfJung. I have that test code as well for later if we want to move forwards with it. I have submitted the PR and it's linked above.

@RalfJung
Copy link
Member Author

The remaining open boxes in this issue are up for grabs again. To properly fix this, you should have some experience with unsafe code.

Maybe I should mark this "hard" instead of "medium"? I don't know how to figure out which label is more appropriate. It's not a lot of work to do this, but it requires rewriting some subtle unsafe code that deals with uninitialized memory.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jul 18, 2019
Remove last use of mem::uninitialized from std::io::util

Addresses rust-lang#62397 for std::io::util
@RalfJung
Copy link
Member Author

@nathanwhit please make you to mention in an issue when you are (partially) addressing it. Now we both did the same work but in different ways.

@nathanwhit
Copy link
Member

I'm sorry, that's totally my mistake. These are my first few contributions, so I'm still in the process of learning the ropes. I'll make sure to do so in the future.

@RalfJung
Copy link
Member Author

No hard feelings, just letting you know. :)

@Diggsey
Copy link
Contributor

Diggsey commented Jul 19, 2019

Seems like under the new UB guidelines, those tests should be testing unspecified rather than uninitialised values. (I believe they're being called "frozen" values?)

Centril added a commit to Centril/rust that referenced this issue Jul 21, 2019
@nathanwhit
Copy link
Member

I've done work on addressing this for CloudAbi (excluding the tests), the PR with my work so far is #62738, feedback is much appreciated!

Centril added a commit to Centril/rust that referenced this issue Jul 22, 2019
Centril added a commit to Centril/rust that referenced this issue Jul 23, 2019
… r=RalfJung

Remove uses of mem::uninitialized from std::sys::cloudabi

Addresses rust-lang#62397 for std::sys::cloudabi, excluding the tests within cloudabi, which will be a separate PR
Centril added a commit to Centril/rust that referenced this issue Jul 24, 2019
… r=RalfJung

Remove uses of mem::uninitialized from std::sys::cloudabi

Addresses rust-lang#62397 for std::sys::cloudabi, excluding the tests within cloudabi, which will be a separate PR
Centril added a commit to Centril/rust that referenced this issue Jul 24, 2019
… r=RalfJung

Remove uses of mem::uninitialized from std::sys::cloudabi

Addresses rust-lang#62397 for std::sys::cloudabi, excluding the tests within cloudabi, which will be a separate PR
Centril added a commit to Centril/rust that referenced this issue Aug 15, 2019
…fJung

Remove uses of `mem::uninitialized()` from cloudabi

This PR removes uses of `mem::uninitialized` from `cloudabi` module,
excluding the layout test in `src/libstd/sys/cloudabi/abi/cloudabi.rs`.

r? @RalfJung
cc @EdSchouten
cc rust-lang#62397
Centril added a commit to Centril/rust that referenced this issue Aug 15, 2019
…fJung

Remove uses of `mem::uninitialized()` from cloudabi

This PR removes uses of `mem::uninitialized` from `cloudabi` module,
excluding the layout test in `src/libstd/sys/cloudabi/abi/cloudabi.rs`.

r? @RalfJung
cc @EdSchouten
cc rust-lang#62397
@RalfJung RalfJung added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Aug 25, 2019
@Mark-Simulacrum
Copy link
Member

Remaining (removable) cases in our test cases are removed in #67507. I also filed NuxiNL/cloudabi#18 upstream to suggest fixing the CloudABI piece; I don't think we should keep this open once #67507 lands as the CloudABI issue must be fixed upstream (the file says "don't edit").

@m-ou-se
Copy link
Member

m-ou-se commented Dec 22, 2019

I've updated the upstream cloudabi.rs to fix this problem, and also include the other changes that were made in the rust repository (#[non_exhaustive], and using *mut instead of &mut for output parameters).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
7 participants