fix(ci): patchelf GNU binaries to use system dynamic linker#112
Merged
fix(ci): patchelf GNU binaries to use system dynamic linker#112
Conversation
Nix builds set the ELF interpreter to /nix/store/.../ld-linux-*.so.2, making the binary unrunnable on standard Linux systems. Use patchelf to reset the interpreter to the standard system path after copying from the Nix result.
WalkthroughAdds post-build binary patching to the release workflow for x86_64 and aarch64 Linux targets (chmod + patchelf to set interpreter and remove RPATH) and changes platform detection in install script to unconditionally tag Linux as Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-release-binaries.yml:
- Around line 73-75: After running the patchelf invocation that rewrites the
interpreter and removes rpath for "dist/${ARCHIVE}/statespace", add a fail-fast
verification using patchelf --print-interpreter and patchelf --print-rpath (or
readelf -l) to assert the interpreter equals /lib64/ld-linux-x86-64.so.2 and
that the RPATH is empty; if either check fails, exit with non‑zero status so CI
stops before creating the archive. Target the same block that runs patchelf (the
command using --set-interpreter and --remove-rpath on
"dist/${ARCHIVE}/statespace") and ensure the verification runs immediately after
it and before any archiving/publishing steps.
Comment on lines
+73
to
+75
| # Nix sets the interpreter to /nix/store/...; patch it to the | ||
| # standard system path so the binary runs outside NixOS. | ||
| nix shell nixpkgs#patchelf -c patchelf --set-interpreter /lib64/ld-linux-x86-64.so.2 --remove-rpath "dist/${ARCHIVE}/statespace" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a fail-fast ELF metadata verification after patching.
The patch step is correct, but there’s no assertion that interpreter/RPATH were actually rewritten. Add a quick check before archiving so CI fails instead of publishing a broken tarball.
Suggested hardening
- nix shell nixpkgs#patchelf -c patchelf --set-interpreter /lib64/ld-linux-x86-64.so.2 --remove-rpath "dist/${ARCHIVE}/statespace"
+ nix shell nixpkgs#patchelf -c patchelf --set-interpreter /lib64/ld-linux-x86-64.so.2 --remove-rpath "dist/${ARCHIVE}/statespace"
+ test "$(nix shell nixpkgs#patchelf -c patchelf --print-interpreter "dist/${ARCHIVE}/statespace")" = "/lib64/ld-linux-x86-64.so.2"
+ test -z "$(nix shell nixpkgs#patchelf -c patchelf --print-rpath "dist/${ARCHIVE}/statespace")"
@@
- nix shell nixpkgs#patchelf -c patchelf --set-interpreter /lib/ld-linux-aarch64.so.1 --remove-rpath "dist/${ARCHIVE}/statespace"
+ nix shell nixpkgs#patchelf -c patchelf --set-interpreter /lib/ld-linux-aarch64.so.1 --remove-rpath "dist/${ARCHIVE}/statespace"
+ test "$(nix shell nixpkgs#patchelf -c patchelf --print-interpreter "dist/${ARCHIVE}/statespace")" = "/lib/ld-linux-aarch64.so.1"
+ test -z "$(nix shell nixpkgs#patchelf -c patchelf --print-rpath "dist/${ARCHIVE}/statespace")"Also applies to: 137-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-release-binaries.yml around lines 73 - 75, After
running the patchelf invocation that rewrites the interpreter and removes rpath
for "dist/${ARCHIVE}/statespace", add a fail-fast verification using patchelf
--print-interpreter and patchelf --print-rpath (or readelf -l) to assert the
interpreter equals /lib64/ld-linux-x86-64.so.2 and that the RPATH is empty; if
either check fails, exit with non‑zero status so CI stops before creating the
archive. Target the same block that runs patchelf (the command using
--set-interpreter and --remove-rpath on "dist/${ARCHIVE}/statespace") and ensure
the verification runs immediately after it and before any archiving/publishing
steps.
The Nix-built GNU binaries have a /nix/store dynamic linker and link against glibc 2.42, making them unrunnable on standard Linux systems. Since the CLI uses rustls (no OpenSSL/native deps), musl static builds work everywhere. Default the install script to musl on all Linux. Also keep patchelf fix for GNU builds so they work if downloaded directly.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/build-release-binaries.yml (1)
73-76: 🧹 Nitpick | 🔵 TrivialAdd fail-fast ELF metadata assertions right after patching.
After Line 76 and Line 141, assert interpreter and RPATH explicitly before packaging so CI fails instead of publishing a bad artifact.
Proposed hardening
nix shell nixpkgs#patchelf -c patchelf --set-interpreter /lib64/ld-linux-x86-64.so.2 --remove-rpath "dist/${ARCHIVE}/statespace" + test "$(nix shell nixpkgs#patchelf -c patchelf --print-interpreter "dist/${ARCHIVE}/statespace")" = "/lib64/ld-linux-x86-64.so.2" + test -z "$(nix shell nixpkgs#patchelf -c patchelf --print-rpath "dist/${ARCHIVE}/statespace")" @@ nix shell nixpkgs#patchelf -c patchelf --set-interpreter /lib/ld-linux-aarch64.so.1 --remove-rpath "dist/${ARCHIVE}/statespace" + test "$(nix shell nixpkgs#patchelf -c patchelf --print-interpreter "dist/${ARCHIVE}/statespace")" = "/lib/ld-linux-aarch64.so.1" + test -z "$(nix shell nixpkgs#patchelf -c patchelf --print-rpath "dist/${ARCHIVE}/statespace")"Also applies to: 138-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-release-binaries.yml around lines 73 - 76, After calling patchelf --set-interpreter and --remove-rpath on the built binary (the command using "patchelf --set-interpreter /lib64/ld-linux-x86-64.so.2 --remove-rpath \"dist/${ARCHIVE}/statespace\""), add immediate fail-fast assertions that verify the ELF interpreter is /lib64/ld-linux-x86-64.so.2 and that the RPATH is empty by invoking patchelf --print-interpreter and patchelf --print-rpath against dist/${ARCHIVE}/statespace and making the CI step exit non-zero if the values do not match expected; repeat the same verification after the later patching/packaging block (the duplicate patchelf usage near the second occurrence) so the workflow fails early instead of publishing a bad artifact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/build-release-binaries.yml:
- Around line 73-76: After calling patchelf --set-interpreter and --remove-rpath
on the built binary (the command using "patchelf --set-interpreter
/lib64/ld-linux-x86-64.so.2 --remove-rpath \"dist/${ARCHIVE}/statespace\""), add
immediate fail-fast assertions that verify the ELF interpreter is
/lib64/ld-linux-x86-64.so.2 and that the RPATH is empty by invoking patchelf
--print-interpreter and patchelf --print-rpath against
dist/${ARCHIVE}/statespace and making the CI step exit non-zero if the values do
not match expected; repeat the same verification after the later
patching/packaging block (the duplicate patchelf usage near the second
occurrence) so the workflow fails early instead of publishing a bad artifact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Nix builds set the ELF interpreter to
/nix/store/.../ld-linux-*.so.2, making the GNU binaries unrunnable on standard Linux systems (GitHub runners, user machines, etc.). This was introduced in PR #95 (Nix crane builds) and is why v0.1.5's install script fails:The musl builds are unaffected (statically linked). v0.1.3 worked because it used cargo-dist's native build matrix, not Nix.
Fix
After copying the GNU binary from the Nix result, use
patchelfto reset the interpreter to the standard system path:/lib64/ld-linux-x86-64.so.2/lib/ld-linux-aarch64.so.1Uses
nix shell nixpkgs#patchelfsince patchelf may not be pre-installed on Blacksmith runners.Summary by CodeRabbit