std: Slightly more robust env var handling #29305

Merged
merged 1 commit into from Nov 6, 2015

Conversation

Projects
None yet
5 participants
Owner

alexcrichton commented Oct 25, 2015

As discovered in #29298, env::set_var("", "") will panic, but it turns out
that it also deadlocks on Unix systems. This happens because if a panic
happens while holding the environment lock, we then go try to read
RUST_BACKTRACE, grabbing the environment lock, causing a deadlock.

Specifically, the changes made here are:

  • The environment lock is pushed into std::sys instead of std::env. This
    also only puts it in the Unix implementation, not Windows where the functions
    are already threadsafe.
  • The std::sys implementation now returns io::Result so panics are
    explicitly at the std::env level.

brson was assigned by rust-highfive Oct 25, 2015

Collaborator

rust-highfive commented Oct 25, 2015

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

Contributor

tbu- commented Oct 25, 2015

It would probably be nicer to include either the error code or the offending key, rather than just a static message.

Contributor

tbu- commented Oct 25, 2015

Also, do we want to treat invalid key as missing? Should we also treat File::open("\0") as file not found?

Owner

alexcrichton commented Oct 26, 2015

Hm that's true about treating an invalid key, I'll switch it back to being a panic.

@alexcrichton alexcrichton std: Slightly more robust env var handling
As discovered in #29298, `env::set_var("", "")` will panic, but it turns out
that it *also* deadlocks on Unix systems. This happens because if a panic
happens while holding the environment lock, we then go try to read
RUST_BACKTRACE, grabbing the environment lock, causing a deadlock.

Specifically, the changes made here are:

* The environment lock is pushed into `std::sys` instead of `std::env`. This
  also only puts it in the Unix implementation, not Windows where the functions
  are already threadsafe.
* The `std::sys` implementation now returns `io::Result` so panics are
  explicitly at the `std::env` level. The panic messages have also been improved
  in these situations.
4b43e07
Owner

alexcrichton commented Nov 5, 2015

ping r? @brson

Contributor

brson commented Nov 5, 2015

@bors r+

Contributor

bors commented Nov 5, 2015

📌 Commit 4b43e07 has been approved by brson

Contributor

bors commented Nov 6, 2015

⌛️ Testing commit 4b43e07 with merge 41308a0...

@bors bors added a commit that referenced this pull request Nov 6, 2015

@bors bors Auto merge of #29305 - alexcrichton:bad-getenv, r=brson
As discovered in #29298, `env::set_var("", "")` will panic, but it turns out
that it *also* deadlocks on Unix systems. This happens because if a panic
happens while holding the environment lock, we then go try to read
RUST_BACKTRACE, grabbing the environment lock, causing a deadlock.

Specifically, the changes made here are:

* The environment lock is pushed into `std::sys` instead of `std::env`. This
  also only puts it in the Unix implementation, not Windows where the functions
  are already threadsafe.
* The `std::sys` implementation now returns `io::Result` so panics are
  explicitly at the `std::env` level.
41308a0

@bors bors merged commit 4b43e07 into rust-lang:master Nov 6, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

alexcrichton deleted the alexcrichton:bad-getenv branch Jan 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment