Check ELF relocatability by structure when bottling

[MikeMcQuaid]

This PR deliberately rebalances the bottle relocatability checker towards classifying bottles as relocatable rather than pinned, now documented as design decision 11 in plans/relocatable-bottles.md.

The original strings-based checker treats every prefix byte sequence in a file as a pin. For ELF files that counts bytes the program can never reach: Meson's install-time RPATH fixer overwrites the build RPATH with the shorter install RPATH without clearing the rest of the old string (intentional upstream; Nix patches it out), and patchelf leaves the old dynamic string table or interpreter behind when growing them. A wrongly pinned bottle forces source builds for every non-default-prefix user and poisons its whole dependent subtree, whereas a wrongly relocatable one surfaces as a per-formula bug report with a trivial fix and is caught by the planned validation sweep and test-bot relocated-pour test.

Concretely, ELF files are no longer strings-scanned as a whole. Keg.text_matches_in_file now takes its candidates from the ELF structures themselves, using the vendored elftools:

• the interpreter the loader uses (PT_INTERP);
• the dynamic strings the loader references: dynamic tags (including the ELF ABI's real DT_FILTER, which elftools 1.3.1 mislabels) and .dynsym names, read from their offset to the NUL so a suffix-merged reference never yields the dead prefix in front of it;
• the printable contents of the remaining sections.

Bytes outside every section and unreferenced entries in loader-owned string tables are never candidates. Files that are not ELF, cannot name their sections (e_shstrndx of SHN_UNDEF) or whose tables are truncated mid-parse keep the whole-file scan, so nothing is dropped where the structure cannot be read. Other file types are unchanged.

Affected formulae flip to cellar :any on their next rebottle.

This change is part of plans/relocatable-bottles.md

---

• Have you followed our Contributing guidelines?
• Have you checked for other open Pull Requests for the same change?
• Have you explained what your changes do? Performance claims (e.g. "this is faster") must include brew benchmark results.
• Have you explained why you'd like these changes included, not just what they do?
• For bug fixes, have you given step-by-step brew commands to reproduce the bug?
• Have you written new tests (excluding integration tests)? Here's an example.
• Have you successfully run brew lgtm (style, typechecking and tests) locally?

---

• I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

Claude Fable 5 with local review and testing.

---

[github-actions]

Thanks for your pull request. This has been closed because it appears to be missing the pull request template, perhaps because this was written by an AI not a human. We require humans to read and fill in these templates.

Please edit this pull request to fill in the current pull request template. This workflow will reopen this pull request automatically once the template is complete. Do not open a new pull request for this.

[Copilot]

Pull request overview

Updates bottling scans to ignore stale ELF prefix strings left by patchelf while preserving live references.

Changes:

• Adds ELF-aware match filtering.
• Adds regression tests for stale and live strings.
• Removes the completed relocation-plan item.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Summary	Findings
Library/Homebrew/test/keg_relocate/elf_checker_spec.rb	Adds ELF filtering regression coverage.	None
Library/Homebrew/plans/relocatable-bottles.md	Removes the completed plan item.	None
Library/Homebrew/keg_relocate.rb	Implements ELF-aware filtering.	Two moderate findings (4 votes each): account for DT_CONFIG, DT_DEPAUDIT, and DT_AUDIT; rescue parser truncation errors such as IOError/EOFError.

Suppressed comments (3)

Library/Homebrew/keg_relocate.rb:540

• The vendored elftools defines DT_FILTER as 0x7ffffffe, the same value as DT_USED, while the ELF ABI value for DT_FILTER is 0x7fffffff. Thus an actual DT_FILTER tag is not added to live_string_offsets (and a DT_USED value is treated as a string offset), so a live filter path can be discarded. Correct the dependency constant or account for the ABI value here.

          ELFTools::Constants::DT::DT_FILTER,

Library/Homebrew/keg_relocate.rb:572

• match_range covers the entire printable run returned by strings, not the specific prefix occurrence being reported. For example, if a dynamic tag points at the interior libfoo.so of /old/prefix/libfoo.so, the live offset lies in this range and the unreferenced /old/prefix is retained, so this still pins a stale suffix-merged prefix. Track the byte range of the actual matched occurrence before deciding it is live.

        # A string some dynamic tag still points into is live, including
        # references into the middle of a suffix-merged string.
        next true if live_string_offsets.any? { |live| match_range.cover?(live) }

Library/Homebrew/keg_relocate.rb:536

• The dynamic string table is also indexed by loader-visible structures such as .dynsym symbol st_name entries (and version tables), not only by the string-valued dynamic tags listed here. A prefix-containing live symbol name will therefore be treated as an unreferenced .dynstr string and dropped. Preserve offsets referenced by the dynamic symbol/version tables before classifying the remainder as dead.

        # Dynamic tags whose value is an offset into the dynamic string
        # table, i.e. the strings the loader can actually see.
        string_tags = [
          ELFTools::Constants::DT::DT_NEEDED,
          ELFTools::Constants::DT::DT_SONAME,

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[carlocab]

I don't know if we can justify treating every match outside the section table as dead, which seems to be what this approach does implicitly.

ELFFile#sections reflects only the section headers, while strings - scans the whole file. ELF execution is described by program headers, not just sections, so I think we can pick up text matches in non-section data but mistakenly classify them as non-live.

Also: I think Copilot's suppressed comments contain correctness issues that shouldn't have been suppressed.

[carlocab]

I think we want to keep the exception narrow here. We only need to identify strings that are demonstrably stale artefacts left behind by patchelf.rb; treating every match outside the current ELF structure as non-live seems broader than necessary, and risks suppressing strings we don't actually know are dead.

[MikeMcQuaid]

I think we want to keep the exception narrow here. We only need to identify strings that are demonstrably stale artefacts left behind by patchelf.rb; treating every match outside the current ELF structure as non-live seems broader than necessary, and risks suppressing strings we don't actually know are dead.

I disagree. As the person who wrote the original strings implementation: it's far too broad and we don't have a rock solid approach that allows us to narrow it sufficiently. It's a trade-off, ultimately, and the benefit to users on non-default prefixes is much higher when we nudge things closer towards assuming relocation rather than assuming not.

I'll look at all the comments here including the suppressed one but just want to be explicit that the there is an active goal here to be less "safe" and prioritise removing false negatives over false positives which we can trivially fix if we get bug reports.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

[carlocab]

just want to be explicit that the there is an active goal here to be less "safe" and prioritise removing false negatives over false positives which we can trivially fix if we get bug reports

In that case, this goal should have been articulated clearly in at least one of, and ideally all three of:

• the PR body
• the commit message
• the design document

The PR body and design document describe the goal as removing false positives caused by stale patchelf artefacts. Deliberately making ELF relocation detection less conservative is a materially broader policy, and changes how this PR should be reviewed.

Moreover, if the intended policy is actually that strings outside ELF sections never count, we should skip the whole-file strings - scan for ELF files and just scan the relevant ELF sections instead. This avoids wasted work, makes that policy explicit in the implementation, and makes the code easier to reason about.

[MikeMcQuaid]

Deliberately making ELF relocation detection less conservative is a materially broader policy, and changes how this PR should be reviewed.

The broad policy here is "make all bottles relocatable" and that is and was very clearly documented in the plan. Literally the goal is "every Homebrew/core bottle pours at every prefix".

Moreover, if the intended policy is actually that strings outside ELF sections never count, we should skip the whole-file strings - scan for ELF files and just scan the relevant ELF sections instead.

If you're suggesting just do this for ELF files specifically: yes, if this is what you'd like me to do I'd be in favour.

We shouldn't skip this for other files types.

[carlocab]

The broad policy here is "make all bottles relocatable" and that is and was very clearly documented in the plan. Literally the goal is "every Homebrew/core bottle pours at every prefix".

Making all bottles actually relocatable is not the same policy as making the checker more willing to classify bottles as relocatable. The former is clearly documented; the latter is the policy trade-off I’m saying was not.

If you're suggesting just do this for ELF files specifically: yes, if this is what you'd like me to do I'd be in favour.

We shouldn't skip this for other files types.

Yes, this is what I was suggesting.

[MikeMcQuaid]

@carlocab adjusted code/plan and PR title/body accordingly, thanks!

[carlocab]

Minor nits. I don't know if I'm sold on how much we're trading off false-positives vs false-negatives here, especially considering that this seems to be much less conservative than what conda or spack do. But I'm not opposed to trying it.

[cho-m]

growing an RPATH or interpreter makes patchelf move the dynamic string table or interpreter, leaving the old prefix string dead in the file, so bottles whose live linkage is fully placeholdered (e.g. harfbuzz) were wrongly pinned to their build cellar

Just mentioning that harfbuzz is more likely from meson shrinking the RPATH rather than patchelf growing the RPATH. I looked into these when trying to find out why bubblewrap was not relocatable:

• bubblewrap: workaround for RPATHs homebrew-core#287486

During install, Meson replaces build RPATH with install RPATH but doesn't erase old contents which can leave behind unwanted paths. Nix has carried a patch for over 5 years now to avoid this.

• https://github.com/NixOS/nixpkgs/blob/nixos-unstable/pkgs/by-name/me/meson/package.nix#L42-L48

Given this seems to be an intentional design decision in Meson based on comment, ignoring the leftover strings is reasonable.

• https://github.com/mesonbuild/meson/blob/1.12.0/mesonbuild/scripts/depfixer.py#L655-L668

[MikeMcQuaid]

@carlocab thanks for approval!

I don't know if I'm sold on how much we're trading off false-positives vs false-negatives here, especially considering that this seems to be much less conservative than what conda or spack do

As far as I can tell, Conda and Spack don't run a relocatability checker at all; they assume relocatability by construction (padded prefix + unconditional rewrite) and rely on bug reports, which is arguably less conservative than what we're doing.

If you have any links to their differences in approaches (or corrections to above): will happily read an incorporate!