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

Always quote program name in Command::spawn on Windows #42436

Merged
merged 2 commits into from Jun 6, 2017

Conversation

Projects
None yet
5 participants
@ollie27
Copy link
Contributor

ollie27 commented Jun 4, 2017

CreateProcess will interpret args as part of the binary name if it
doesn't find the binary using just the unquoted name. For example if
foo.exe doesn't exist, Command::new("foo").arg("bar").spawn() will
try to launch foo bar.exe which is clearly not desired.

Always quote program name in Command::spawn on Windows
`CreateProcess` will interpret args as part of the binary name if it
doesn't find the binary using just the unquoted name. For example if
`foo.exe` doesn't exist, `Command::new("foo").arg("bar").spawn()` will
try to launch `foo bar.exe` which is clearly not desired.
@retep998

This comment has been minimized.

Copy link
Member

retep998 commented Jun 5, 2017

Alternatively, we could pass the binary name as the first parameter to CreateProcessW instead of having Windows be forced to dig it out of the second parameter. I'd really prefer that approach unless @alexcrichton has some good reason for why we have to do it the way we currently do.

@alexcrichton alexcrichton self-assigned this Jun 5, 2017

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jun 5, 2017

Looks good to me, thanks @ollie27! Would it be possible to add a test for this?

Passing the binary name explicitly sounds fine as well, so it sounds like we should just do both things?

@ollie27

This comment has been minimized.

Copy link
Contributor Author

ollie27 commented Jun 5, 2017

It looks like using the first parameter behaves very differently:

In the case of a partial name, the function uses the current drive and current directory to complete the specification. The function will not use the search path. This parameter must include the file name extension; no default extension is assumed.

I guess I'll try to write a run-make test for this.

Add run-make test for Command::spawn on Windows
Make sure args aren't interpreted as part of the program name.
@ollie27

This comment has been minimized.

Copy link
Contributor Author

ollie27 commented Jun 5, 2017

Right, I've added a run-make test.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jun 5, 2017

@bors: r+

Thanks @ollie27!

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 5, 2017

📌 Commit 02955f5 has been approved by alexcrichton

@retep998

This comment has been minimized.

Copy link
Member

retep998 commented Jun 5, 2017

So I just did a quick test and when I passed a verbatim path as the first parameter to CreateProcessW I was able to specify paths that exceeded MAX_PATH while when I only specified the second parameter it would fail with ERROR_FILENAME_EXCED_RANGE. So even though using the first parameter has fairly different semantics than only using the second parameter, if we ever want to support long paths we'll need to use that first parameter.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 6, 2017

⌛️ Testing commit 02955f5 with merge 9006db1...

bors added a commit that referenced this pull request Jun 6, 2017

Auto merge of #42436 - ollie27:win_spawn_name, r=alexcrichton
Always quote program name in Command::spawn on Windows

[`CreateProcess`](https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425.aspx) will interpret args as part of the binary name if it
doesn't find the binary using just the unquoted name. For example if
`foo.exe` doesn't exist, `Command::new("foo").arg("bar").spawn()` will
try to launch `foo bar.exe` which is clearly not desired.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jun 6, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 9006db1 to master...

@bors bors merged commit 02955f5 into rust-lang:master Jun 6, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
homu Test successful
Details

@ollie27 ollie27 deleted the ollie27:win_spawn_name branch Jun 6, 2017

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 13, 2017

rustc: Spawn `cmd /c emcc.bat` explicitly
In rust-lang#42436 the behavior for spawning processes on Windows was tweaked slightly to
fix various bugs, but this caused rust-lang#42791 as a regression, namely that to spawn
batch scripts they need to be manually spawned with `cmd /c` instead now. This
updates the compiler to handle this case explicitly for Emscripten.

Closes rust-lang#42791

alexcrichton added a commit to nrc/rust that referenced this pull request Sep 14, 2017

rustc: Spawn `cmd /c emcc.bat` explicitly
In rust-lang#42436 the behavior for spawning processes on Windows was tweaked slightly to
fix various bugs, but this caused rust-lang#42791 as a regression, namely that to spawn
batch scripts they need to be manually spawned with `cmd /c` instead now. This
updates the compiler to handle this case explicitly for Emscripten.

Closes rust-lang#42791

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 15, 2017

Rollup merge of rust-lang#44542 - alexcrichton:fix-windows-emscripten…
…, r=nikomatsakis

rustc: Spawn `cmd /c emcc.bat` explicitly

In rust-lang#42436 the behavior for spawning processes on Windows was tweaked slightly to
fix various bugs, but this caused rust-lang#42791 as a regression, namely that to spawn
batch scripts they need to be manually spawned with `cmd /c` instead now. This
updates the compiler to handle this case explicitly for Emscripten.

Closes rust-lang#42791

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 16, 2017

Rollup merge of rust-lang#44542 - alexcrichton:fix-windows-emscripten…
…, r=nikomatsakis

rustc: Spawn `cmd /c emcc.bat` explicitly

In rust-lang#42436 the behavior for spawning processes on Windows was tweaked slightly to
fix various bugs, but this caused rust-lang#42791 as a regression, namely that to spawn
batch scripts they need to be manually spawned with `cmd /c` instead now. This
updates the compiler to handle this case explicitly for Emscripten.

Closes rust-lang#42791

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Sep 16, 2017

Rollup merge of rust-lang#44542 - alexcrichton:fix-windows-emscripten…
…, r=nikomatsakis

rustc: Spawn `cmd /c emcc.bat` explicitly

In rust-lang#42436 the behavior for spawning processes on Windows was tweaked slightly to
fix various bugs, but this caused rust-lang#42791 as a regression, namely that to spawn
batch scripts they need to be manually spawned with `cmd /c` instead now. This
updates the compiler to handle this case explicitly for Emscripten.

Closes rust-lang#42791

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 16, 2017

Rollup merge of rust-lang#44542 - alexcrichton:fix-windows-emscripten…
…, r=nikomatsakis

rustc: Spawn `cmd /c emcc.bat` explicitly

In rust-lang#42436 the behavior for spawning processes on Windows was tweaked slightly to
fix various bugs, but this caused rust-lang#42791 as a regression, namely that to spawn
batch scripts they need to be manually spawned with `cmd /c` instead now. This
updates the compiler to handle this case explicitly for Emscripten.

Closes rust-lang#42791

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 16, 2017

Rollup merge of rust-lang#44542 - alexcrichton:fix-windows-emscripten…
…, r=nikomatsakis

rustc: Spawn `cmd /c emcc.bat` explicitly

In rust-lang#42436 the behavior for spawning processes on Windows was tweaked slightly to
fix various bugs, but this caused rust-lang#42791 as a regression, namely that to spawn
batch scripts they need to be manually spawned with `cmd /c` instead now. This
updates the compiler to handle this case explicitly for Emscripten.

Closes rust-lang#42791

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Sep 17, 2017

Rollup merge of rust-lang#44542 - alexcrichton:fix-windows-emscripten…
…, r=nikomatsakis

rustc: Spawn `cmd /c emcc.bat` explicitly

In rust-lang#42436 the behavior for spawning processes on Windows was tweaked slightly to
fix various bugs, but this caused rust-lang#42791 as a regression, namely that to spawn
batch scripts they need to be manually spawned with `cmd /c` instead now. This
updates the compiler to handle this case explicitly for Emscripten.

Closes rust-lang#42791
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.