Skip to content

Re-sign Mach-O files after relocation fails - #24183

Merged
MikeMcQuaid merged 1 commit into
mainfrom
codesign-failed-relocation
Oct 6, 2026
Merged

MikeMcQuaid merged 1 commit into
mainfrom
codesign-failed-relocation

Conversation

@MikeMcQuaid

@MikeMcQuaid MikeMcQuaid commented Oct 6, 2026 •

Copy link
Copy Markdown
Member
  • Re-sign saved files even when a later relocation raises.
  • Cover install and bottle relocation while preserving the error.

Fixes #24182


  • 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.

Codex with GPT 6 Astra at Extra High effort, with local review and testing.


Summary by CodeRabbit

  • Bug Fixes
    • Patched binaries are now re-signed even when linkage processing fails partway through, while the original error is still reported.

- Re-sign saved files even when a later relocation raises.
- Cover install and bottle relocation while preserving the error.

Fixes #24182
Copilot AI lite review requested due to automatic review settings October 6, 2026 14:41
@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: af84eadb-520a-466c-9b4b-61a90ed863cd
📥 Commits

Reviewing files that changed from the base of the PR and between f534bc7 and b290e55.

📒 Files selected for processing (2)
  • Library/Homebrew/extend/os/mac/keg_relocate.rb
  • Library/Homebrew/test/os/mac/keg_spec.rb

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

📜 Recent review details
⏰ Context from checks skipped due to timeout. (18)
  • GitHub Check: docker (x86_64 Ubuntu 24.04)
  • GitHub Check: docker (arm64 Ubuntu 26.04)
  • GitHub Check: docker (x86_64 Ubuntu 26.04)
  • GitHub Check: docker (arm64 Ubuntu 24.04)
  • GitHub Check: docs
  • GitHub Check: tests (online, 2/2)
  • GitHub Check: tests (generic OS, 1/2)
  • GitHub Check: tests (load-only macOS)
  • GitHub Check: tests (generic OS, 2/2)
  • GitHub Check: tests (macOS, 1/2)
  • GitHub Check: tests (Linux, 2/2)
  • GitHub Check: tests (macOS, 2/2)
  • GitHub Check: tests (online, 1/2)
  • GitHub Check: tests (no Sorbet)
  • GitHub Check: tests (load-only Linux)
  • GitHub Check: tests (Linux, 1/2)
  • GitHub Check: syntax
  • GitHub Check: Analyze
🔇 Additional comments (2)
Library/Homebrew/extend/os/mac/keg_relocate.rb (1)

72-73: LGTM!

Also applies to: 76-76, 128-129, 132-132

Library/Homebrew/test/os/mac/keg_spec.rb (1)

135-145: LGTM!

Also applies to: 179-189


📝 Walkthrough

Walkthrough

Relocation and fix-up now re-sign patched files in ensure blocks, including when processing raises an error. Relocation returns linkage_files; fix-up returns the result of super. Regression tests cover both error paths.

Changes

Mach-O signing after patch failures

Layer / File(s) Summary
Patch Mach-O files and ensure signing
Library/Homebrew/extend/os/mac/keg_relocate.rb, Library/Homebrew/test/os/mac/keg_spec.rb
Both linkage methods re-sign their initialized file lists in ensure blocks. Tests verify that previously patched files are re-signed and that MachO::HeaderPadError propagates. Relocation returns linkage_files; fix-up returns the result of super.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: issyl0

Merge Risk: ⚪ Minimal · up to b290e

This change re-signs Mach-O files that were already patched even when a later file fails relocation, which prevents invalid signatures and SIGKILL on Apple Silicon. The error still propagates. No actionable merge-blocking risk was found.

Security Architecture Review

Security architecture risk: 🔵 Low · up to b290e

This is a narrowly scoped recovery improvement with unchanged file ownership controls and signing authority. Remaining uncertainty concerns failure handling: re-signing remains best-effort rather than an atomic repair or rollback.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The newly reachable outcome is signature repair of previously modified keg binaries after later processing fails. Those binaries may regain loadability despite incomplete processing, but the signer receives the same selected paths and ad-hoc authority as on the pre-existing successful path.

Trust Boundaries and Controls

  • observed — Metadata-supplied relocation paths pass through keg_files, which checks lexical containment, rejects symlink or non-file entries, and checks resolved containment within the keg. Ordinary Mach-O discovery walks the keg path, skips symlinks, and deduplicates device/inode pairs. These existing selectors remain upstream of the changed signing calls.

Resilience and Maintainability Implications

  • inferred — The cleanup improves recovery for already tracked files without establishing atomic signing or unconditional preservation of the primary error. The helper contains operations that can raise outside its specific signing-error rescue. The installation caller still catches all fix-up exceptions and reports linkage failure, but the behavior of interrupted signing workers and subsequent recovery was not verified.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: re-signing Mach-O files when relocation fails.
Linked Issues check ✅ Passed Issue #24182 requires previously modified Mach-O files to be re-signed when a later relocation fails, while retaining the relocation failure behavior. relocate_dynamic_linkage and `fix_dynamic_linka…
Out of Scope Changes check ✅ Passed The implementation changes are limited to re-signing files saved before a relocation failure. The added tests directly cover issue #24182. No unrelated changes appear in the whole-PR diff.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

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.

Copilot review overview

🔵 Needs a closer look

Cleanup signing failures must not replace the original relocation exception.

Review effort: Lite
Findings: None

What changed in this PR

Updates macOS Mach-O relocation to re-sign previously saved files when a later relocation fails.

Changes:

  • Re-signs patched files during relocation cleanup.
  • Adds install and bottle relocation regression tests.
File Summary
Library/​Homebrew/​extend/​os/​mac/​keg_relocate.rb Re-signs modified Mach-O files after relocation failures. Cleanup errors may still mask the original relocation exception.
Library/​Homebrew/​test/​os/​mac/​keg_spec.rb Adds regression coverage for install and bottle relocation failures.

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

Copilot AI left a comment

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.

Copilot review overview

🔵 Needs a closer look

Preserve the active relocation exception when cleanup signing also fails.

Review effort: Lite
Findings: None

Merged via the queue into main with commit cb64474 Oct 6, 2026
55 checks passed
@MikeMcQuaid
MikeMcQuaid deleted the codesign-failed-relocation branch October 6, 2026 15:20
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.

Failed relocation leaves previously patched Mach-O files with invalid signatures (regression since 6.0.20)

2 participants

⌂ 换网址