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

Produce sane CARGO when invoked through ld.so #10115

Closed
wants to merge 3 commits into from

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Nov 23, 2021

If current_exe indicates that the current exe is ld-*.so.* (so it "looks like" the dynamic linker), we fall back to using argv[0] instead.

Fixes #10113, which also has further details on the problem.

@rust-highfive
Copy link

r? @ehuss

(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 Nov 23, 2021
@alexcrichton
Copy link
Member

From the description of #10113 I didn't quite understand why executables were being invoked in this fashion, I actually had no idea this was even possible. To some extent this feels like an issue for the standard library rather than Cargo itself because this presumably plauges all users of current_exe and Cargo just happened to be the first that you ran into.

@alexcrichton
Copy link
Member

Er, sorry, meant to include the question, can you detail a bit more why Cargo is being invoked in this fashion?

@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 1, 2021

Ah, so, invoking through ld.so is basically a slightly saner way to manage the shared library search path than setting LD_LIBRARY_PATH. The latter "infects" child processes with the same search path, whereas ld.so only sets the search path for the invoked process and not any processes that it may in turn spawn. This becomes important in build environments that want to tightly control the search path of each executable to match what it was built with, since Cargo can end up executing other binaries off of $PATH, such as through build scripts.

If the question is "why do you need to set the shared library search path?", the answer is that this is necessary when libraries are not necessarily located in the standard system locations, in my case because ever executable gets its own execution environment that contains the exact version of the shared libraries it was built with.

As for the question of whether this should just be fixed in std, that one's tougher. My take here is that current_exe should return the value of /proc/self/exe, which will be ld.so, precisely so that, for example, a program can detect if it's being executed through ld.so. I suppose it could return a particular kind of error if it detected ld.so, but that seems somewhat fragile. Part of the issue is that if ld.so is current_exe, there isn't really a reliable way to determine the current executable. Using argv[0] like here is a decent fallback, but it's not guaranteed to point at a valid executable (e.g., if exec -a is used), and there are likely other reasonable choices.

@alexcrichton
Copy link
Member

Hm ok that makes sense. I'll let @ehuss decide on whether to sign-off on this or not. Personally this feels so esoteric I would prefer to not have it in Cargo itself. It feels to me like this technique of spawning a process through ld.so invalidates 100% of std::env::current_exe() use cases, and I don't really want Cargo to be the one that fixes this given my perception of how widespread the problem would be.

@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 3, 2021

Yeah, it's a tricky one. I'm also torn on where the fix should ultimately live. My thinking is that it's Cargo's responsibility to set CARGO to something "valid", and that's what's failing here. current_exe returns the "right" value from the OSes perspective, but just as for #10119, Cargo can't just blindly use that value since the semantics of CARGO is to give a usable path to Cargo itself, which is only tangentially related to current_exe.

@joshtriplett
Copy link
Member

I personally agree with @alexcrichton that this seems too specific to go in Cargo as-is. We often get cases where Cargo is asked to work around and fix an issue that applies to almost any other program, and we're typically hesitant to pioneer a solution that isn't pushed in some more common shared place, especially if it isn't clear what the consensus solution should be.

If there were a common library crate that provided a function handling this situation and providing a well-defined result, and some indication that others were willing to use that too, we might be willing to consider using that.

@jonhoo
Copy link
Contributor Author

jonhoo commented Dec 7, 2021

In my particular case, the proposal in #10119 (comment) ("propagate CARGO from the environment if already set") would go a long way in and of itself. It would allow me to set CARGO in the environment, which would take precedence over ld-linux from current_exe. What do you think?

@ehuss
Copy link
Contributor

ehuss commented Dec 14, 2021

I'm going to close this as I agree with the comments above that this is probably not something we would want to add.

As for reading the CARGO environment variable, I'd like to keep that discussion on #10119. (Personally, I don't really know if that is a good idea, as there can be some really subtle interactions with different environments.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CARGO env var set incorrectly when Cargo invoked through ld
5 participants