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.

Sign up for GitHub

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

Jump to bottom

Don't panic on std::env::vars() when env is null. #53208

Merged
merged 1 commit into from
Aug 14, 2018
Merged

Don't panic on std::env::vars() when env is null. #53208

merged 1 commit into from
Aug 14, 2018

Conversation

BurntPizza
Copy link
Contributor

Fixes #53200.

Reviewer(s):

  • Do I need to do any #[cfg()] here?
  • Is this use of libc ok for a dev-dependency?

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(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 Aug 8, 2018
@BurntPizza BurntPizza changed the title Don't panic on std::env::vars() when env in null. Don't panic on std::env::vars() when env is null. Aug 8, 2018
@joshtriplett
Copy link
Member

The change itself seems completely reasonable to me; good catch.

I don't know whether you can use libc here. And even if you can, does the test get isolated such that the use of clearenv doesn't affect other tests?

@BurntPizza
Copy link
Contributor Author

It seems to, it runs first anyway, and they all pass. That was a concern for me as well.

@BurntPizza
Copy link
Contributor Author

The only other thing is trusting environ() to return a non-null. I haven't looked at docs for stuff like _NSGetEnviron, and it hasn't been an issue but it still gives me some heeby-jeebies.

@joshtriplett
Copy link
Member

Wouldn't other tests (not just the environment tests) get compiled into the same binary? I'm concerned about the tests being fragile.

@BurntPizza
Copy link
Contributor Author

BurntPizza commented Aug 8, 2018 •
edited
Loading

The binary that runs is build/x86_64-unknown-linux-gnu/stage1-std/x86_64-unknown-linux-gnu/release/deps/env-a962c0d6d7770051 (when doing ./x.py test --stage 1 src/libstd) so I don't think so.
Should I save-and-restore the env?

@joshtriplett
Copy link
Member

@BurntPizza Ah, OK. No objection in that case, but I don't know if you can use libc here.

@BurntPizza
Copy link
Contributor Author

The main Cargo.toml for libstd uses the same libc_shim import as a regular dependency, so I think it's ok.
cc @alexcrichton Could you or someone who would know weigh in on this?

@alexcrichton
Copy link
Member

Thanks for the PR! The dev-dependency here should work out fine but @joshtriplett is right in that this shouldn't be executing in the main body of tests. Rather can a new entirely standalone program be added to src/test/run-pass to execute this test? That also avoids the need to have a libc dev-dep.

Finally I do think that this'll need a #[cfg(target_os = "linux")] marker probably, it likely won't pass on Windows and some other obscure platforms here and there.

@kennytm kennytm 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 Aug 9, 2018
@BurntPizza BurntPizza force-pushed the protect-the-environment branch from 0cb7099 to c9aca02 Compare August 9, 2018 16:58
@BurntPizza
Copy link
Contributor Author

Rebased with changes.

@alexcrichton
Copy link
Member

@bors: r+

Thanks!

@bors
Copy link
Contributor

bors commented Aug 10, 2018

📌 Commit c9aca02 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 10, 2018
@kennytm kennytm mentioned this pull request Aug 10, 2018
Closed
kennytm added a commit to kennytm/rust that referenced this pull request Aug 11, 2018
@kennytm
Rollup merge of rust-lang#53208 - BurntPizza:protect-the-environment,…
a7830aa 
… r=alexcrichton

Don't panic on std::env::vars() when env is null.

Fixes rust-lang#53200.

Reviewer(s):
* Do I need to do any `#[cfg()]` here?
* Is this use of libc ok for a dev-dependency?
kennytm added a commit to kennytm/rust that referenced this pull request Aug 13, 2018
@kennytm
Rollup merge of rust-lang#53208 - BurntPizza:protect-the-environment,…
cabe191 
… r=alexcrichton

Don't panic on std::env::vars() when env is null.

Fixes rust-lang#53200.

Reviewer(s):
* Do I need to do any `#[cfg()]` here?
* Is this use of libc ok for a dev-dependency?
@kennytm kennytm mentioned this pull request Aug 13, 2018
Closed
kennytm added a commit to kennytm/rust that referenced this pull request Aug 14, 2018
@kennytm
Rollup merge of rust-lang#53208 - BurntPizza:protect-the-environment,…
700c5e8 
… r=alexcrichton

Don't panic on std::env::vars() when env is null.

Fixes rust-lang#53200.

Reviewer(s):
* Do I need to do any `#[cfg()]` here?
* Is this use of libc ok for a dev-dependency?
@kennytm kennytm mentioned this pull request Aug 14, 2018
Merged
bors added a commit that referenced this pull request Aug 14, 2018
@bors
Auto merge of #53354 - kennytm:rollup, r=kennytm
a573305 
Rollup of 11 pull requests

Successful merges:

 - #53112 (pretty print BTreeSet)
 - #53208 (Don't panic on std::env::vars() when env is null.)
 - #53226 (driver: set the syntax edition in phase 1)
 - #53229 (Make sure rlimit is only ever increased)
 - #53233 (targets: aarch64: Add bare-metal aarch64 target)
 - #53239 (rustc_codegen_llvm: Restore the closure env alloca hack for LLVM 5.)
 - #53246 (A few cleanups)
 - #53257 (Idiomatic improvements to IP method)
 - #53274 (Remove statics field from CodegenCx)
 - #53290 (Make LLVM emit assembly comments with -Z asm-comments)
 - #53317 (Mark prior failure to avoid ICE)
@bors bors merged commit c9aca02 into rust-lang:master Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@joshtriplett joshtriplett

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@BurntPizza @rust-highfive @joshtriplett @alexcrichton @bors @kennytm