Skip to content

fix-elf: patchelf PT_INTERP to strip +brewing post-install#342

Closed
tannevaled wants to merge 1 commit into
pkgxdev:mainfrom
tannevaled:fix/preserve-pt-interp-post-rename
Closed

fix-elf: patchelf PT_INTERP to strip +brewing post-install#342
tannevaled wants to merge 1 commit into
pkgxdev:mainfrom
tannevaled:fix/preserve-pt-interp-post-rename

Conversation

@tannevaled
Copy link
Copy Markdown

Summary

brewkit installs each pkg to ${prefix}+brewing/ then renames to ${prefix}/. The build's ld --dynamic-linker=...+brewing/.../ld-linux*.so flag bakes the +brewing path into PT_INTERP of every linked binary. fix-elf has historically only patched RPATH via patchelf, leaving PT_INTERP referencing a path that no longer exists post-rename.

This is harmless for typical pantry recipes — their binaries' PT_INTERP points at the host runner's /lib(64)?/ld-linux-*.so, never back into the bottle. So the +brewing substring never appears in PT_INTERP and there's nothing to fix.

For low-level packages whose own bottle ships ld-linux* (glibc is the obvious example, future musl bottle would be another), PT_INTERP after install is broken — bin/ld.so, bin/getconf, bin/iconv etc. all fail to exec because the kernel can't find the +brewing path. This blocks any direct invocation of the bottle's tools in the test phase or by consumers.

Change

Add a no-op-by-default fix_interpreter() step alongside set_rpaths():

export default async function fix_rpaths(installation: Installation, pkgs: string[]) {
  console.info("doing SLOW rpath fixes…")
  for await (const [exename] of exefiles(installation.path)) {
    await set_rpaths(exename, pkgs, installation)
    await fix_interpreter(exename)   // ← new
  }
}

fix_interpreter():

  • reads PT_INTERP with patchelf --print-interpreter
  • if it contains "+brewing", rewrites via patchelf --set-interpreter
  • skips if the rewritten target doesn't exist (guards against bad rewrites)
  • catches errors quietly when the file is statically linked / has no .interp section (consistent with the existing set_rpaths defensive style)

Compatibility

No behaviour change for any existing pantry bottle. They all consume the host runner's ld-linux at PT_INTERP, so the substring "+brewing" never matches and fix_interpreter() is a no-op.

Refs

🤖 Generated with Claude Code

brewkit installs into ${prefix}+brewing/ then renames to ${prefix}/.
The build's `ld --dynamic-linker=...+brewing/.../ld-linux*.so` flag
bakes the +brewing path into PT_INTERP of every linked binary.
fix-elf has historically only patched RPATH via patchelf, leaving
PT_INTERP referencing a path that no longer exists post-rename.

For typical pantry recipes this is harmless: their binaries' PT_INTERP
points at the host runner's /lib(64)?/ld-linux-*.so, not back into
their own bottle, so the bake doesn't matter. For low-level packages
whose own bottle ships ld-linux* (glibc being the obvious example),
PT_INTERP after install is broken — bin/ld.so, bin/getconf, etc. fail
to exec because the kernel can't find the +brewing path. This blocked
gnu.org/glibc (pkgxdev/pantry#5080 / pkgxdev/pantry#12968) from
shipping its own bin/ tools as usable: the test-phase had to fall
back on bash-builtin `test -f` rather than running e.g. `ld.so --version`.

Add a no-op-by-default `fix_interpreter()` step alongside `set_rpaths()`:
- read PT_INTERP with `patchelf --print-interpreter`
- if it contains "+brewing", rewrite via `patchelf --set-interpreter`
- skip if the rewritten target doesn't exist (guards against bad rewrites)
- catch errors quietly when the file is statically linked / has no
  .interp section, consistent with the surrounding set_rpaths style.

No behaviour change for any existing pantry bottle — they all consume
the host runner's ld-linux at PT_INTERP, so the substring "+brewing"
never matches.

Refs: pkgxdev/pantry#12968 (gnu.org/glibc PR).
Copilot AI review requested due to automatic review settings May 20, 2026 14:00
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/bin/fix-elf.ts
} catch {
return // not a dynamic ELF, or statically linked, or no PT_INTERP
}
if (!cur.includes("+brewing")) return
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might be more robust if it used lib/config.ts for config.path.build_install; I don't know if that introduces more difficulties. fix-machos.rb specifically doesn't, since it's in Ruby.

@jhheider
Copy link
Copy Markdown
Contributor

hm, this path isn't fixed and absolute, though. it's relative to PKGX_DIR...

@jhheider
Copy link
Copy Markdown
Contributor

Codex suggests:

  • Do not patch PT_INTERP to pkgx-managed glibc by default.

  • For glibc package self-tests, explicitly invoke its loader:
    {{prefix}}/lib/ld-linux-.so. --library-path {{prefix}}/lib .

  • If a package truly needs hermetic glibc, use wrapper scripts, not PT_INTERP.

  • If fix-elf touches PT_INTERP, restrict it to replacing +brewing only when the final path is guaranteed stable in that install context, not as general bottle metadata.

  • Hermetic binary: use a wrapper:

    exec "$prefix/lib/ld-linux-x86-64.so.2"
    --library-path "$prefix/lib:$other_libs"
    "$prefix/libexec/foo" "$@"
    The inner ELF can even have a dummy/system PT_INTERP; the wrapper explicitly invokes the loader.

tannevaled added a commit to tannevaled/pantry that referenced this pull request May 21, 2026
Address jhheider's review feedback on PR pkgxdev#12968:

- Revert mirror.kumi.systems → ftp.gnu.org (supply-chain concern;
  the transient ftp.gnu.org DNS failure was an unstable pool member
  and shouldn't drive us to a third-party mirror)
- Add a real compile-and-run test using test.c (already in the recipe
  dir but never actually invoked from the test block). Builds the
  test program against THIS bottle's headers / libc / loader, then
  runs it via the bottle's ld.so explicitly with --library-path so
  we don't rely on PT_INTERP (which brewkit's fix-elf doesn't
  currently rewrite — see pkgxdev/brewkit#342). Verifies the reported
  gnu_get_libc_version() matches the bottle's marketing version.

Keeps the env: + script: structure introduced in 3a9517e; just
extends script: with the compile + execute steps and adds a test
dep on gnu.org/gcc.
@tannevaled
Copy link
Copy Markdown
Author

Thanks @jhheider and @codex — you're both right, and I'm going to withdraw this PR.

Why withdraw

The core insight from Codex's review:

Do not patch PT_INTERP to pkgx-managed glibc by default. For glibc package self-tests, explicitly invoke its loader: {{prefix}}/lib/ld-linux-*.so.* --library-path {{prefix}}/lib <binary>. If a package truly needs hermetic glibc, use wrapper scripts, not PT_INTERP.

I've now done exactly that in pkgxdev/pantry#12968's test block — the glibc bottle's test compiles test.c and runs it via the bottle's loader explicitly:

"$LIBDIR/$LDSO" --library-path "$LIBDIR" ./test-bottle

…which proves the bottle's libc + ld.so + headers form a self-consistent set, without needing brewkit to patch PT_INTERP. So the original motivation for this PR (test-time PT_INTERP rewriting) is gone — that part of the problem is better solved at the recipe-test level.

The deeper question — should brewkit ship hermetic binaries?

For runtime hermeticity (binary always uses the bundled glibc regardless of host), Codex's wrapper-script pattern is the right shape:

exec "$prefix/lib/ld-linux-*.so.*" \
  --library-path "$prefix/lib:$other_libs" \
  "$prefix/libexec/foo" "$@"

That's an orthogonal feature that should land as a separate brewkit issue/PR — it's "how does brewkit produce hermetic bottles," not "how does fix-elf rewrite PT_INTERP." Happy to open that follow-up if there's appetite (probably belongs in lib/porcelain/ alongside bottle.ts, not in fix-elf.ts).

Closing this PR. Apologies for the spin-up.

@tannevaled tannevaled closed this May 21, 2026
@jhheider
Copy link
Copy Markdown
Contributor

Never a problem. One option is a brewkit helper to use hermetic glibc, similar to bkpyvenv or fix-shebang.ts. Then, if you need hermetic glibc, you move all of bin to libexec and create hermetic wrappers and depend explicitly (or you get 2.17 by default).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants