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

Using a shebang-less script as a linker doesn't work anymore on Linux #101511

Open
glandium opened this issue Sep 7, 2022 · 14 comments
Open

Using a shebang-less script as a linker doesn't work anymore on Linux #101511

glandium opened this issue Sep 7, 2022 · 14 comments
Labels
C-bug Category: This is a bug. P-low Low priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. relnotes Marks issues that should be documented in the release notes of the next release. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@glandium
Copy link
Contributor

glandium commented Sep 7, 2022

STR:

  • cargo new foo
  • cd foo
  • echo exit 42 > linker
  • chmod +x linker
  • RUSTFLAGS="-Clinker=$PWD/linker" cargo +nightly build

Actual result:

error: could not exec the linker `/tmp/foo/linker`
  |
  = note: Exec format error (os error 8)

With 1.63:

error: linking with `/tmp/foo/linker` failed: exit status: 42

This is a regression from #95026. What's going on is that before that merge, the linker was executed via posix_spawnp@GLIBC_2.2.5, and after, it's executed via posix_spawnp@GLIBC_2.15. The difference between the two is that the former retries executing through the shell when the execution first failed with ENOEXEC. This applies to anything the compiler or cargo would execute via std::process::Command. The change in posix_spawnp behavior in glibc comes from https://sourceware.org/bugzilla/show_bug.cgi?id=13134. It's probably for the better, but it should probably be highlighted in the release notes.

@glandium glandium added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Sep 7, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 7, 2022
@Mark-Simulacrum
Copy link
Member

This would happen with anything using Command, right? Not just rustc/cargo.

@Mark-Simulacrum Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. and removed regression-untriaged Untriaged performance or correctness regression. labels Sep 7, 2022
@Mark-Simulacrum
Copy link
Member

Nominating for libs-api to consider whether we want to workaround this by implementing the re-execution ourselves; my feeling is no but it is a regression so I think worth asking.

@Mark-Simulacrum Mark-Simulacrum added this to the 1.64.0 milestone Sep 7, 2022
@glandium
Copy link
Contributor Author

glandium commented Sep 7, 2022

It only happens when using libstd-*.so from the newer rustc builds, or when building with any rustc against an old glibc.

@glandium
Copy link
Contributor Author

glandium commented Sep 7, 2022

That is, if you build some random crate that uses Command with any version of rustc, the behavior depends on whether the libc used for the crate compile is < 2.15 or not, not whether rustc is 1.64 or not.

@glandium
Copy link
Contributor Author

glandium commented Sep 7, 2022

rustc 1.64 itself is affected by the change because it's built with a newer glibc, while older versions of rustc were built with an older glibc.

@glandium
Copy link
Contributor Author

glandium commented Sep 7, 2022

Note that it might be worth reviewing what other functions from glibc used by rustc/cargo have changed and how, because there might be subtle changes like this hidden somewhere.

@Mark-Simulacrum
Copy link
Member

Ah, that makes sense. It seems likely that folks might bump their build environments to a newer glibc since we only support newer ones now, so I think I'll still leave this nominated, but it's good if it affects few users in practice.

T-compiler may want to do that checking as you suggest and consider a (temporary, perhaps with a future compat warning?) fallback, separately from any decisions by std.

@m-ou-se
Copy link
Member

m-ou-se commented Sep 7, 2022

Nominating for libs-api to consider whether we want to workaround this by implementing the re-execution ourselves; my feeling is no but it is a regression so I think worth asking.

I also think the answer should be "no", but agree it's worth calling out in release notes or similar.

@m-ou-se m-ou-se added T-libs Relevant to the library team, which will review and decide on the PR/issue. I-libs-nominated Nominated for discussion during a libs team meeting. and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Sep 7, 2022
@cuviper
Copy link
Member

cuviper commented Sep 7, 2022

Especially given #101511 (comment), that users who link against newer glibc were already getting the new behavior, I don't think we should change to preserve the old.

@m-ou-se
Copy link
Member

m-ou-se commented Sep 7, 2022

The consensus in the libs meeting was to not fall back to /bin/sh.

@m-ou-se m-ou-se removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. I-libs-nominated Nominated for discussion during a libs team meeting. labels Sep 14, 2022
@Mark-Simulacrum Mark-Simulacrum added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Dec 9, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 9, 2022
@m-ou-se m-ou-se added T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 14, 2022
@m-ou-se m-ou-se added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. labels Dec 14, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Dec 14, 2022

Reassigning to the compiler team. We're not planning to change std::process::Command, but they might want to consider adding special handling in the compiler for this.

@joshtriplett
Copy link
Member

I don't think the compiler team should have a special case for this, unless there's some real-world use case that's affecting projects. @glandium, is there some case that's hitting the specific use case of a linker in practice, or is this a more theoretical concern or a concern about the behavior of Command in general?

@glandium
Copy link
Contributor Author

I don't remember on what I hit this specifically, but I did hit it on some existing code that was using a linker wrapper without a shebang. I can tell for sure it was not Firefox. At the very least, the behavior change should be called out.

@apiraino
Copy link
Contributor

apiraino commented Dec 15, 2022

WG-prioritization assigning priority (Zulip discussion).

Tagging for release notes, I feel the suggestion in the first comment makes sense.

@rustbot label -I-prioritize +P-low +release-notes

@apiraino apiraino added P-low Low priority relnotes Marks issues that should be documented in the release notes of the next release. and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. P-low Low priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. relnotes Marks issues that should be documented in the release notes of the next release. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants