Use argv[0] for cargo_exe so we don't rely on /proc on Linux by kivikakk · Pull Request #4634 · rust-lang/cargo

kivikakk · 2017-10-17T13:29:10Z

This is a proposed solution to #4450. I'm not at all wedded to the idea or the code, though, so feel free to shoot it down with abandon if this isn't something that'd work out or that you like.

In short, we use the existing CARGO_ENV ("CARGO") if present, and only if not do we attempt to perform a lookup with env::current_exe() ourselves. This means users without access to current_exe (such as Linux without procfs mounted) can supply the CARGO env var themselves for external commands to use.

My concern here is: what if maybe we intentionally switch cargo binaries and didn't intend for this to happen? Could this ever happen outside a test environment? This kind-of-sorta-happened by accident in the test suite, necessitating the explicit removal of CARGO_ENV from the subprocess environment, because the actual cargo executing the test suite propagated its own path into the test subprocess!

/cc @alexcrichton as the originator of the idea of CARGO_ENV

rust-highfive · 2017-10-17T13:29:16Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

kivikakk · 2017-10-17T13:31:00Z

src/cargo/util/config.rs

I didn't canonicalise in both cases on purpose, i.e.:

env::var(::CARGO_ENV).map(PathBuf::from).or_else(|_| env::current_exe()).and_then(|path| path.canonicalize())

I figure if the user is supplying their own, we can trust them to select an appropriate incantation. (And if this is one we've propagated ourselves, we'll have canonicalised it here first.)

matklad · 2017-10-17T13:36:28Z

An alternative solution which comes to mind is a fallback to looking argv[0] in $PATH. Not sure if it works though!

kivikakk · 2017-10-17T13:39:56Z

That was my first thought! It might work, but it might not, depending on the use case. The original issue at #3778 (comment) which introduced this env var said:

Most of them seem to just call cargo (that is, the one in PATH) and hope it's the right one.

Cargo should pass the path to itself to subcommands so that they can be sure they're running the right cargo. IMHO the best way would be an environment variable, something like CARGO_BIN.

This provides a means to continue to provide the reassurance that it won't suddenly switch to the first cargo in PATH. It's really quite an edge case though, and it wouldn't be so bad to fall back to that if procfs wasn't available, though it might be quite surprising in the tiny fraction of cases in which that was the case.

kivikakk · 2017-10-17T13:40:42Z

"the tiny fraction of cases in which that was the case", wow. Can you tell I should be asleep already?

matklad · 2017-10-17T13:48:41Z

Yeah, now that I think of it, your solution seems great to me! Let's ask @alexcrichton thoughts though?

I think we might want to document this here

https://github.com/rust-lang/cargo/blob/master/src/doc/environment-variables.md

or here

https://github.com/rust-lang/cargo/blob/master/src/doc/book/src/reference/environment-variables.md

(I am totally confused: why do we have two sets of docs? 🤷‍♂️ )

And we might want to provide a custom warning message along the lines of

Can't get path to the Cargo executble. This is most likely caused by the absence of procfs. Please specify path to Cargo executable via `CARGO` environmental variable.

kivikakk · 2017-10-17T13:52:19Z

You're right about documentation and the better error message. I'll add those if Alex thinks this looks okay. 👍

alexcrichton · 2017-10-17T14:23:13Z

Thanks for the PR! I'm a little wary of using a CARGO env var for this though as it opens up a few avenues to misconfiguration I think? I wonder if we could perhaps just thread through argv[0] into Config from program start?

kivikakk · 2017-10-17T23:41:03Z

@alexcrichton Ah, yes; that's a way better solution! Will do.

kivikakk · 2017-10-18T02:33:22Z

Looking good except for a broken test on Windows (because a UNC-style \\?\ appears at the start of one path) — just finishing bootstrapping my Windows dev environment so I can fix this without waiting on CI, then we should be done!

matklad · 2017-10-18T07:56:33Z

src/cargo/util/config.rs

Could we, instead of passing an argument to default method, call std::env::args here? default method with an argument looks funny :)

Also, perhaps we should try to use current_exec first, and only then fall-back to using argv[0]? I don't have any solid arguments pro or contra, but using current_exec seems appropriate in most cases.

Ah yeah I agree with @matklad that using current_exe first and then falling back to env::args is probably the best strategy here

alexcrichton · 2017-10-18T15:23:24Z

Thanks! One other comment I'd have is that when using env::args the path returned may not be absolute, but I believe we rely on it being absolute. Perhaps a mixture of PATH probing and current_dir could help?

kivikakk · 2017-10-19T03:43:30Z

I was thinking canonicalize might be smart enough to do this on its own, but nope; PATH probing will have to be the way of it.

kivikakk · 2017-10-19T05:18:39Z

Here's the latest attempt:

We try env::current_exe.
If that fails, we grab argv[0] (assuming it even exists), and then attempt to canonicalize it.
- This works if it's an absolute path, or if it's a relative path to the current working directory.
If that fails, and argv0 is one component (no directory separators), we then probe PATH in order for a matching file.

The code is a bit janky; curious for your feedback.

alexcrichton · 2017-10-19T22:43:26Z

Looks reasonable to me! It's sort of unfortunate but AFAIK this is the only solution to "env::current_exe failed but I really do need the current executable"

Mind adding some comments for why we have these fallbacks?

kivikakk · 2017-10-20T01:24:10Z

Excellent call! Done.

matklad · 2017-10-20T08:04:41Z

src/cargo/util/config.rs

+                    .and_then(|argv0| argv0.canonicalize().or_else(|_| probe_path(argv0)))
+            }
+
+            fn probe_path(argv0: PathBuf) -> CargoResult<PathBuf> {


Something similar happens in execute_external_subcommand, but I don't think we can reasonable extract common functionality here.

matklad · 2017-10-20T08:11:12Z

Hm, what I still don't find perfect about current approach is that we still get a hard error if we fail to resolve current_exe, although we actually unlikely to use it at all (that is, we need to set CARGO variable when executing any target process, but few target processes actually use CARGO).

Could we just avoid settings this environmental variable if we failed to get path to an executable (with a warning)?

Also, does this have any security implications? Will it be possible to make, for example, a custom subcommand execute an arbitrary process instead of Cargo by, for example, placing a binary in a relative path so that canonicalize resolves to it?

alexcrichton · 2017-10-20T18:54:32Z

I'd be a little wary to proceed to execute a build script without a CARGO env var, presumably some crates depend on that and you'd be deferring Cargo returning an error to otherwise perhaps confusing build errors?

kivikakk · 2017-10-21T04:15:41Z

I'm +1 for erroring out; it means we can catch the weird case and maybe do something about it like we did in #4450.

alexcrichton · 2017-10-23T15:50:37Z

@matklad thoughts?

matklad · 2017-10-23T17:15:58Z

I don't have a strong opinion here, so I am happy with either solution, although I am especially not sure about this line: https://github.com/rust-lang/cargo/pull/4634/files#diff-4559bc8f75ebf0a4d42be6e4e7fe9eaaR170. Looks like because of this we can put in CARGO a wrong path, especially because .canonicalize happens before we probe path, so we'd prefer cargo from working directory to the one from path. Presumably, it should be possible to check if the path is a single component (and look it up in $PATH), take it as is, if it's absolute, and error out otherwise?

I am thinking about "failing silently" because it seems that few builds actually use CARGO environmental variable (as opposed to custom subcommands), but I agree that this fallback might cause hard to debug errors as well!

kivikakk · 2017-10-26T07:09:27Z

Summarising what to do in case current_exe fails:

If path is single component, search $PATH only (and error out if not present, to avoid finding one in cwd which is incorrect).
Otherwise, .canonicalize (as it'll be either absolute, or relative to cwd e.g. target/debug/cargo or ./cargo).

matklad · 2017-10-26T07:32:19Z

@kivikakk oh, wait, argv[0]can legitimately be a relative path! Sorry for causing confusion :(

So yeah, I believe your last comment is absolutely correct and precise, thanks for clarifying it to me!

kivikakk · 2017-10-26T07:33:35Z

@matklad not at all, your comment was important and I think I read what you had intended! That's now implemented in 1bb43b0.

matklad · 2017-10-26T07:35:41Z

@bors r+

Thanks!!!

bors · 2017-10-26T07:35:42Z

📌 Commit 1bb43b0 has been approved by matklad

bors · 2017-10-26T07:35:49Z

⌛ Testing commit 1bb43b0 with merge f6ef634...

@alexcrichton

Use argv[0] for cargo_exe so we don't rely on /proc on Linux This is a proposed solution to #4450. I'm not at all wedded to the idea or the code, though, so feel free to shoot it down with abandon if this isn't something that'd work out or that you like. In short, we use the existing `CARGO_ENV` (`"CARGO"`) if present, and only if not do we attempt to perform a lookup with `env::current_exe()` ourselves. This means users without access to `current_exe` (such as Linux without `procfs` mounted) can supply the `CARGO` env var themselves for external commands to use. My concern here is: what if maybe we intentionally switch cargo binaries and didn't intend for this to happen? Could this ever happen outside a test environment? This kind-of-sorta-happened by accident in the test suite, necessitating the explicit removal of `CARGO_ENV` from the subprocess environment, because the actual cargo executing the test suite propagated its own path into the test subprocess! /cc @alexcrichton as the originator of the idea of `CARGO_ENV`

matklad · 2017-10-26T07:39:22Z

at all, your comment was important and I think I read what you had intended!

To make it super clear, I intended to ban relative paths altogether, because I thought (wrongly), that argv[0] can't be relative. However, it can be relative, but it better be explicitly relative, so that's where the special case of a single components comes from 😆

bors · 2017-10-26T08:13:58Z

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

rust-highfive assigned matklad Oct 17, 2017

kivikakk commented Oct 17, 2017

View reviewed changes

kivikakk mentioned this pull request Oct 17, 2017

Cargo fails in Linux chroot without /proc #4450

Closed

kivikakk changed the title ~~Allow an existing CARGO_ENV through~~ Use argv[0] for cargo_exe so we don't rely on /proc on Linux Oct 18, 2017

matklad reviewed Oct 18, 2017

View reviewed changes

Try current_exe, then argv0 canonicalize, then PATH

6ad2b9b

Comments!

e08efb5

matklad reviewed Oct 20, 2017

View reviewed changes

kivikakk added 2 commits October 26, 2017 18:18

Merge remote-tracking branch 'upstream/master' into cargo-env-prevails

2fb6f25

Try to replicate shell behavior more accurately

1bb43b0

bors merged commit 1bb43b0 into rust-lang:master Oct 26, 2017

kivikakk deleted the cargo-env-prevails branch October 26, 2017 08:14

mar-kub mentioned this pull request May 30, 2020

Upgrade relies on /proc (Linux) denoland/deno#5969

Closed

alexcrichton mentioned this pull request Oct 22, 2021

Don't canonicalize executable path #9991

Merged

ehuss added this to the 1.23.0 milestone Feb 6, 2022

Conversation

kivikakk commented Oct 17, 2017

Uh oh!

rust-highfive commented Oct 17, 2017

Uh oh!

kivikakk Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matklad commented Oct 17, 2017

Uh oh!

kivikakk commented Oct 17, 2017

Uh oh!

kivikakk commented Oct 17, 2017

Uh oh!

matklad commented Oct 17, 2017

Uh oh!

kivikakk commented Oct 17, 2017

Uh oh!

alexcrichton commented Oct 17, 2017

Uh oh!

kivikakk commented Oct 17, 2017

Uh oh!

kivikakk commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matklad Oct 18, 2017

Choose a reason for hiding this comment

Uh oh!

alexcrichton Oct 18, 2017

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Oct 18, 2017

Uh oh!

kivikakk commented Oct 19, 2017

Uh oh!

kivikakk commented Oct 19, 2017

Uh oh!

alexcrichton commented Oct 19, 2017

Uh oh!

kivikakk commented Oct 20, 2017

Uh oh!

matklad Oct 20, 2017

Choose a reason for hiding this comment

Uh oh!

matklad commented Oct 20, 2017

Uh oh!

alexcrichton commented Oct 20, 2017

Uh oh!

kivikakk commented Oct 21, 2017

Uh oh!

alexcrichton commented Oct 23, 2017

Uh oh!

matklad commented Oct 23, 2017

Uh oh!

kivikakk commented Oct 26, 2017

Uh oh!

matklad commented Oct 26, 2017

Uh oh!

kivikakk commented Oct 26, 2017

Uh oh!

matklad commented Oct 26, 2017

Uh oh!

bors commented Oct 26, 2017

Uh oh!

bors commented Oct 26, 2017

Uh oh!

matklad commented Oct 26, 2017

Uh oh!

bors commented Oct 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kivikakk Oct 17, 2017 •

edited

Loading

kivikakk commented Oct 18, 2017 •

edited

Loading