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

Capture Command environment at spawn #46789

Merged
merged 1 commit into from
Dec 24, 2017

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Dec 17, 2017

Fixes #28975

This tracks a set of changes to the environment and then replays them at spawn time.

@rust-highfive
Copy link
Collaborator

r? @TimNN

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

@Diggsey
Copy link
Contributor Author

Diggsey commented Dec 17, 2017

While writing this, I found a few potential issues which I didn't attempt to fix as part of this PR:

  • The case conversion on windows assumes only ascii characters are compared case-insensitively. I couldn't find any documentation detailing whether this is correct, but my suspicion is that it's not (case insensitivity in the file-system does not work this way).

  • On windows, if any environment variables are modified, then all environment variables are upper-cased before being passed to the child process. Ideally case would be preserved. It should be relatively easy to implement this change following this PR, but it relies on the AsciiExt trait gaining a case-insensitive ordering method (at the moment it only allows testing for case-insensitive equality).

  • On linux, the exec implementation is very unsafe as it assigns a short-lived heap-allocated string to the global variable environ without taking a lock std::os::unix::process::CommandExt::exec should be unsafe #46775

  • On fuchsia, when the environment is unchanged the code calls launchpad_set_environ(..) with a null pointer, and I couldn't find any details about whether that is correct.

@TimNN
Copy link
Contributor

TimNN commented Dec 17, 2017

r? @dtolnay since you already commented on the issue and I don't feel like I have enough understanding of the whole topic.

@rust-highfive rust-highfive assigned dtolnay and unassigned TimNN Dec 17, 2017
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

This is exactly what I had in mind. Nicely done, and thanks for flagging the confusing previous behavior.

@dtolnay
Copy link
Member

dtolnay commented Dec 17, 2017

@rust-lang/libs As of Rust 1.22 the behavior of modifying environment variables on a Command is that the parent process environment is captured at the first call to env/env_remove/env_clear, otherwise captured during spawn. There is no way to explicitly trigger the environment capture without making a change to the child process environment or spawning. The result is confusing and unpredictable behavior.

// Is this visible to child process? I would hope so.
os::set_var("A", "A");

let mut command = Command::new("...");

// Is this visible? Basically arbitrary, but currently yes.
os::set_var("B", "B");

if cond() {
    // Set in child env but not parent process.
    command.env("C", "C");
}

// Is this visible? It seems obvious to me that the answer should be the same
// as for B. Currently the answer is maybe! Depends on cond(). Child process
// sees either C or D but not both.
os::set_var("D", "D");

command.spawn()

This PR makes A, B, C, D all visible to the child process. The behavior is as if every environment modification to the Command were evaluated eagerly against the parent environment, with the result being captured for the child at the call to spawn.

@dtolnay dtolnay added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Dec 18, 2017
@dtolnay
Copy link
Member

dtolnay commented Dec 18, 2017

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Dec 18, 2017

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Dec 18, 2017
@bors
Copy link
Contributor

bors commented Dec 18, 2017

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

@kennytm kennytm added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Dec 18, 2017
@rfcbot
Copy link

rfcbot commented Dec 19, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Dec 19, 2017
@alexcrichton
Copy link
Member

@bors: r=dtolnay

@bors
Copy link
Contributor

bors commented Dec 19, 2017

📌 Commit 6679768 has been approved by dtolnay

@kennytm kennytm 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Dec 20, 2017
@bors
Copy link
Contributor

bors commented Dec 22, 2017

⌛ Testing commit 6679768bb144337c8141e46eb5a1052297786637 with merge 97e576bbf0e204ebdf1aed46f5dc7b4790d06b69...

@bors
Copy link
Contributor

bors commented Dec 22, 2017

💔 Test failed - status-travis

Copy link
Member

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

wasm32-unknown failed, legit.


////////////////////////////////////////////////////////////////////////////////
// Command
////////////////////////////////////////////////////////////////////////////////

pub struct Command {
env: CommandEnv
Copy link
Member

Choose a reason for hiding this comment

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

Should be CommandEnv<DefaultEnvKey> here?

[00:41:37] error[E0243]: wrong number of type arguments: expected 1, found 0
[00:41:37]   --> /checkout/src/libstd/sys/wasm/process.rs:24:10
[00:41:37]    |
[00:41:37] 24 |     env: CommandEnv
[00:41:37]    |          ^^^^^^^^^^ expected 1 type argument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah will fix that - ooi, is there a cargo check equivalent for x.py? I'd like to be able to compile targets like wasm/fuchsia/etc. without needing to have a whole cross-compiling toolchain...

Copy link
Member

Choose a reason for hiding this comment

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

is there a cargo check equivalent for x.py?

Not yet. Feel free to file a feature-request.

However, doing a x.py check on stage(N+1) still requires a complete stageN compiler, meanwhile --keep-stage is pretty broken (#44737, #46180), so this may not be as useful as you'd think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 22, 2017
@kennytm
Copy link
Member

kennytm commented Dec 23, 2017

@bors r=dtolnay

@bors
Copy link
Contributor

bors commented Dec 23, 2017

📌 Commit da90b1d has been approved by dtolnay

@kennytm kennytm 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 Dec 23, 2017
@bors
Copy link
Contributor

bors commented Dec 23, 2017

⌛ Testing commit da90b1d with merge faecf24...

bors added a commit that referenced this pull request Dec 23, 2017
Capture `Command` environment at spawn

Fixes #28975

This tracks a set of changes to the environment and then replays them at spawn time.
@bors
Copy link
Contributor

bors commented Dec 23, 2017

💔 Test failed - status-travis

}

pub fn env_clear(&mut self) {
pub fn env_mut(&mut self) -> CommandEnv<DefaultEnvKey> {
Copy link
Member

Choose a reason for hiding this comment

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

Moar wasm typo (missing &mut).

[00:39:21] error[E0308]: mismatched types
[00:39:21]   --> /checkout/src/libstd/sys/wasm/process.rs:52:9
[00:39:21]    |
[00:39:21] 51 |     pub fn env_mut(&mut self) -> CommandEnv<DefaultEnvKey> {
[00:39:21]    |                                  ------------------------- expected `sys_common::process::CommandEnv<sys_common::process::DefaultEnvKey>` because of return type
[00:39:21] 52 |         &mut self.env
[00:39:21]    |         ^^^^^^^^^^^^^ expected struct `sys_common::process::CommandEnv`, found mutable reference
[00:39:21]    |
[00:39:21]    = note: expected type `sys_common::process::CommandEnv<sys_common::process::DefaultEnvKey>`
[00:39:21]               found type `&mut sys_common::process::CommandEnv<sys_common::process::DefaultEnvKey>`

@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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 23, 2017
@Diggsey
Copy link
Contributor Author

Diggsey commented Dec 24, 2017

@kennytm think I've got it this time...

@kennytm
Copy link
Member

kennytm commented Dec 24, 2017

@bors r=dtolnay

@bors
Copy link
Contributor

bors commented Dec 24, 2017

📌 Commit ccc91d7 has been approved by dtolnay

@kennytm kennytm 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 Dec 24, 2017
@bors
Copy link
Contributor

bors commented Dec 24, 2017

⌛ Testing commit ccc91d7 with merge c284f88...

bors added a commit that referenced this pull request Dec 24, 2017
Capture `Command` environment at spawn

Fixes #28975

This tracks a set of changes to the environment and then replays them at spawn time.
@bors
Copy link
Contributor

bors commented Dec 24, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: dtolnay
Pushing c284f88 to master...

@bors bors merged commit ccc91d7 into rust-lang:master Dec 24, 2017
@Diggsey Diggsey deleted the command-env-capture branch December 27, 2017 01:36
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 24, 2021
Update outdated comment in unix Command.

The big comment in the `Command` struct has been incorrect for some time (at least since rust-lang#46789 which removed `envp`). Rather than try to remove the allocations, this PR just updates the comment to reflect reality. There is an explanation for the reasoning at rust-lang#31409 (comment), discussing the potential of being able to call `Command::exec` after `libc::fork`.  That can still be done in the future, but I think for now it would be good to just correct the comment.
Aaron1011 added a commit to Aaron1011/rust that referenced this pull request Feb 25, 2021
Update outdated comment in unix Command.

The big comment in the `Command` struct has been incorrect for some time (at least since rust-lang#46789 which removed `envp`). Rather than try to remove the allocations, this PR just updates the comment to reflect reality. There is an explanation for the reasoning at rust-lang#31409 (comment), discussing the potential of being able to call `Command::exec` after `libc::fork`.  That can still be done in the future, but I think for now it would be good to just correct the comment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants