Skip to content

Fix two PHT-relocation correctness bugs (#643, #639) - #652

Merged
Mic92 merged 6 commits into
NixOS:masterfrom
domenkozar:fix-reloc-bugs
Jun 26, 2026
Merged

Fix two PHT-relocation correctness bugs (#643, #639)#652
Mic92 merged 6 commits into
NixOS:masterfrom
domenkozar:fix-reloc-bugs

Conversation

@domenkozar

Copy link
Copy Markdown
Member

Fixes two independent correctness bugs in the program-header-table (PHT) relocation logic, both of which currently block bumping patchelf in nixpkgs.

#643 — in-place PHT growth assumes the PHT is right after the ELF header

rewriteSectionsLibrary decides whether a section collides with the grown PHT by comparing the section's file offset against phtSize, which only makes sense when the PHT sits at its canonical location right after the ELF header. When the PHT is elsewhere in the file — e.g. the OpenSSL libcrypto.so.1.1 shipped in many manylinux wheels, whose PHT sits near the end — that check looks in the wrong place, the PHT is wrongly kept in place, and patching aborts with cannot find section '.hash'.

The fix detects the non-canonical PHT location up front (e_phoff != sizeof(Elf_Ehdr)) and relocates the PHT to the end of the file in that case.

Verified on the real libcrypto.so.1.1 from the issue: before the fix patchelf errors cannot find section '.hash'; after, --set-rpath succeeds and the result is a structurally valid ELF.

A regression test is included (tests/pht-relocation.sh). The fixture is that real libcrypto.so.1.1, stripped and xz-compressed (decompressed at test time, skipped if xz is unavailable — mirroring the existing short-first-segment.gz fixture). I could not reproduce the corruption with a minimally-synthesized binary — it depends on the real multi-segment / many-section layout — so the fixture is larger (~730 KB) than the existing ones. Happy to swap it for a smaller trigger if anyone knows of one.

#639.dynamic fixup never updated DT_INIT / DT_FINI / DT_*_ARRAY

The .dynamic fixup loop rewrites DT_STRTAB/DT_SYMTAB/DT_HASH/… to the new addresses of their sections, but never updated DT_INIT, DT_FINI, DT_INIT_ARRAY, DT_FINI_ARRAY or DT_PREINIT_ARRAY. When growing the PHT relocates .init (common with lld's tight layout), the loader then jumps to a stale DT_INIT and the process SIGSEGVs, typically on dlopen. The fix updates those entries from the relocated section addresses, like the others.

This fix is correct by construction and the full test suite stays green, but I was not able to synthesize the exact lld layout that relocates .init, so it has no dedicated regression test. Guidance on producing such a fixture welcome.


These are independent; I'm happy to split into two PRs if that's easier to review/merge.

Assisted-by: Claude Code (Claude Opus 4.8)

Fedr added a commit to Fedr/patchelf that referenced this pull request Jun 25, 2026
Add the genuine shim from MeshLib daily release v3.1.2.247 (2026-05-16,
clang-21/lld-21) as a fixture: .init at 0x224 right after an 8-entry PHT
ending at 0x200, DT_INIT=0x224. A --set-rpath that forces a 9th PT_LOAD
relocates .init; control patchelf leaves DT_INIT stale, PR NixOS#652 syncs it.
Asserted via readelf (no dlopen; the shim's deps aren't on the runner).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fedr added a commit to Fedr/patchelf that referenced this pull request Jun 25, 2026
…valid)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Fedr

Fedr commented Jun 25, 2026

Copy link
Copy Markdown

Confirming this addresses a real downstream case — I filed #639 after hitting it while packaging MeshLib's Python bindings (an lld-built libpybind11nonlimitedapi_*.so run through patchelf --set-rpath by linuxdeploy).

I recovered the actual triggering binary from one of our daily builds (clang/lld 21). It has exactly the tight layout that's hard to synthesize — .init packed immediately after an 8-entry PHT:

PHDR                       off 0x40   (8 entries, ends at 0x200)
[ 1] .note.gnu.build-id    0x200
[ 2] .init                 0x224      <- right after the PHT
[ 3] .plt                  0x240
[ 4] .text                 0xff0
DT_INIT = 0x224

Reproduction with patchelf 0.18.0 (the released line linuxdeploy bundles) — appending any path to the existing RUNPATH is enough:

$ patchelf --set-rpath '<existing>:$ORIGIN/x' shim.so
$ readelf -SW shim.so | grep ' .init '   # .init moved to 0x32028
$ readelf -dW shim.so | grep '(INIT)'    # DT_INIT still 0x224  <- stale

_dl_init calls the stale 0x224 (now overwritten by the appended Phdr) and SIGSEGVs on dlopen. Built from this PR, DT_INIT tracks .init and the library loads cleanly.

One caveat if you want it as a regression test: on current master, patchelf relocates the whole PHT to end-of-file for this binary instead of growing it in place, so .init isn't relocated and it's handled correctly even without this PR — the corruption only appears with the 0.18.0-era in-place growth. So this .so confirms #639 is real and that master no longer mangles it, but it wouldn't fail as a test without the DT_INIT change; you'd need an input that still takes the in-place-growth path to exercise that code directly. The binary is attached here: https://github.com/user-attachments/files/29332263/libpybind11nonlimitedapi_meshlib_3.12.so.zip

@domenkozar

Copy link
Copy Markdown
Member Author

Thanks for the detailed confirmation and for recovering the real binary.

You're right, and it goes a bit deeper than this one binary. In rewriteSectionsLibrary the in place growth path bails to full PHT relocation as soon as it hits a non replaceable section in range, and .init is SHT_PROGBITS so it always triggers that. The executable path also refuses to move SHT_PROGBITS sections. So on current master .init never gets relocated in place, which is why your .so loads fine with or without the DT_INIT change.

Given that, I'll keep the #639 fix as defensive correctness (it mirrors the existing DT_STRTAB/DT_SYMTAB fixups) rather than add a regression test that can't actually fail. The #643 path is covered directly by the test in this PR.

Thanks again, the binary is a useful end to end sanity check.

godlygeek and others added 3 commits June 25, 2026 10:38
Add a regression test to ensure that PHT collision detection correctly
detects when growing the PHT in place would cause it to collide with
another section, even if the PHT is not at the start of the ELF file.
Patching a shared library might require extending its program header
table, which can then cause it to run out of its originally allocated
space. After 484d349, patchelf handles this by detecting that growing
the PHT in place would result in a collision with a section that won't
be moved, in which case it instead moves the PHT to the end of the ELF
file.

The logic checking for collisions incorrectly assumes that the PHT is
originally at the start of the file, though. It checks every section
after the first to see if it overlaps with the PHT given its new size,
but it assumes that the PHT's original position is immediately after the
ELF header, `sizeof(Elf_Ehdr)` bytes into the file.

In other words, it's currently looking for collisions in the range:

    [0, roundUp(sizeof(Elf_Ehdr) + newPhtSize, sectionAlignment)]

This commit fixes it to check for collisions in the correct range:

    [hdr()->e_phoff, hdr()->e_phoff + roundUp(newPhtSize, sectionAlignment)]
The .dynamic fixup loop rewrites DT_STRTAB/DT_SYMTAB/DT_HASH/... to the
new addresses of their sections, but never updated DT_INIT, DT_FINI,
DT_INIT_ARRAY, DT_FINI_ARRAY or DT_PREINIT_ARRAY. When relocating one of
those sections (e.g. growing the PHT pushes .init to the end of the
file, common with lld's tight layout), the loader then jumps to a stale
DT_INIT and the process SIGSEGVs, typically on dlopen. Update those
entries from the relocated section addresses like the others.

Closes NixOS#639

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@domenkozar

Copy link
Copy Markdown
Member Author

Restructured this PR to build on @godlygeek's #644 rather than duplicate it:

Verified locally: pht-collision.sh passes, and @Fedr's real lld binary patches cleanly into a valid ELF. If preferred, #644 can land on its own and I'll rebase the #639 commit on top.

ELF does not require section headers to be sorted by sh_offset, and this
path never calls sortShdrs(). Breaking on the first header past the PHT
window can skip a later-indexed colliding section.
@Mic92
Mic92 enabled auto-merge June 26, 2026 08:44
@Mic92

Mic92 commented Jun 26, 2026

Copy link
Copy Markdown
Member

@domenkozar feel free to drop a release

Mic92 added 2 commits June 26, 2026 08:55
Build a shared library whose DT_INIT points at a symbol in .text rather
than the .init section, and check that patchelf leaves the tag alone.
These tags hold a function address (-Wl,-init/-fini), not the section
start. .init/.fini are SHT_PROGBITS and never relocated here, so the
rewrite only broke binaries with a custom init symbol. Keep the
DT_*_ARRAY cases, which are section addresses by spec.
@Mic92
Mic92 added this pull request to the merge queue Jun 26, 2026
Merged via the queue into NixOS:master with commit 67b1a2b Jun 26, 2026
15 checks passed
suyanpanghuang added a commit to suyanpanghuang/zvec that referenced this pull request Aug 3, 2026
Exclude libstdc++.so.6 and libgcc_s.so.1 from auditwheel repair so
patchelf is not invoked. patchelf <= 0.18.0 moves protodesc_cold without
updating R_X86_64_RELATIVE addends, breaking protobuf init.

Ref: NixOS/patchelf#652
suyanpanghuang added a commit to suyanpanghuang/zvec that referenced this pull request Aug 6, 2026
Exclude libstdc++.so.6 and libgcc_s.so.1 from auditwheel repair so
patchelf is not invoked. patchelf <= 0.18.0 moves protodesc_cold without
updating R_X86_64_RELATIVE addends, breaking protobuf init.

Ref: NixOS/patchelf#652
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.

4 participants