Repository navigation
fix(relinker): protect macOS TLS code entry roots - #11488
roboterspinne wants to merge 3 commits into
Conversation
The macOS TLS scan asked the code collector for no entry roots, so it replaced an fs access with jmp rel32 and NOPs even when a pointer target taken as data lay inside it, and the relink succeeded with the referenced bytes changed. The Windows TLS builder has refused this since boykopovar#5890 and boykopovar#8166. The macOS scan now collects the same entry roots and fails with "TLS rewrite would overwrite an executable entry point" when one lies strictly inside the rewritten instruction. Every input it refuses is already refused by --windows; --macos used to rewrite it silently.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the entry-root guard against the existing Windows TLS check and built this exact head in a Linux relinker-only Release build. The SCE integration script passed, and the focused CTest selection passed 40/40, including sce_code_extent and the TLS coverage tests. Hosted Linux, Windows, macOS arm64 and ASan relinker checks also pass; macOS x86_64 is still pending. Product jobs are skipped by the repository's path policy.
One repository requirement remains: CONTRIBUTING.md says to keep the branch up to date with main. tools/check_main_sync.py --strict reports this head 2 commits ahead and 37 behind current main, although the merge is clean and intervening main commits do not touch the PR's changed paths. Please sync the branch and rerun the focused tests; I found no independent code defect and will re-review the updated head.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
@MarcZX10 Synced: 9ef8b4b merges current |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Validated on macOS: Apple M2 Max, macOS 27.2, Xcode 27 clang, relinker-only Release (-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON). Head 9ef8b4b75a applied on current main cb45ae2ddf (clean merge). Two builds: arm64 native, and x86_64 under Rosetta with /usr/bin/python3 3.9.6.
- Red/green. With only the test change on
main,sce_code_extentfails atsce-tls-rewrite-over-data-target-macos(exit 0, output written) on both arm64 and Rosetta. With the fix, the focused set passes 41/41 on both:sce_code_extent,sce_code_gaps,macos_tls_*,macos_gs_traps,macos_trace_imports,windows_tls_protected_entriesand the 34tls_function_coverage_*. Full suite: arm64 309/309, Rosetta 307/307, the same counts asmain. - Effect in the generated Mach-O on
main. I convertedlea_into_tls_fixture()with--macos --skip-sce-module. The output is an x86_64 Mach-O, and the guest bytes at 0x240 go from64 48 8b 04 25 00 00 00 00 c3toe9 bb 1d 00 00 90 90 90 90 c3. The four bytes theleaaddresses (0x244) change from25 00 00 00to00 90 90 90. - At runtime on
main. I ran that Mach-O under Rosetta with stub x86_64 dylibs forlib.so,libc.prxandlibkernel.prx.Aps5StartGuest_nid_no_patchcalls the entry and prints the four bytes at the pointer it returns (theSTARTpattern oftest_macos_trace_imports.py). The TLS allocate/register hooks are no-ops, since thefsload never runs. The guest reads00 90 90 90. The output is the same whether the relinker was built arm64 or x86_64. - With this PR,
--macosexits 2 withTLS rewrite would overwrite an executable entry point (offset 0x244)and writes no output, matching--windows.
No findings.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
maximilianfv
left a comment
maximilianfv
left a comment
There was a problem hiding this comment.
Approve: the macOS TLS scan now refuses a supported fs: rewrite over a collector root the same way the Windows builder does, and the new case fails without the fix and passes with it on macOS x86_64.
Tested on Apple M1 Pro, macOS 27.0.1 (x86_64 build under Rosetta 2)
Applied at 9ef8b4b75 to mugurc's macOS series (27e968974) merged with main at 2742004ed2, with other open macOS fixes, then toggled only this PR. Full build, Release.
Selection: ctest -R 'sce_code_extent|sce_code_gaps|macos_tls|tls_function_coverage|windows_tls|macos_gs|macos_empty_tls' (42 tests: sce_code_extent, sce_code_gaps, macos_tls_alu, macos_tls_overlap, macos_empty_tls, macos_gs_traps, 4 windows_tls_* and 33 tls_function_coverage_*).
sce_code_extent (sce-tls-rewrite-over-data-target-*) |
other 41 tests | |
|---|---|---|
| Without this PR (relinker unchanged, only the new test case added) | Failed: AssertionError: ('sce-tls-rewrite-over-data-target-macos', 0, ...), output written with Took 1 pointer target(s) outside the unwind functions as data, the first at 0x244; the Windows case before it passed |
41/41 passed |
| With this PR | Passed | 41/41 passed (two runs, 42/42 each) |
main at b7105f7285 still passes nullptr to Collect in ScanTls, so the bug is still there, and this head merges with it without conflicts.
Code
The check at core/relinker/elfpatcher/src/macos/MacOsImage.cpp:338-340 is the Windows checkProtectedEntry rule (WindowsTlsBuilder.cpp), applied to the same span the macOS rewrite overwrites (info.Length). A root at the instruction start is still accepted (upper_bound), which sce-pointer-to-far-tls-load-* already covers. A root at the instruction end is accepted too. Passing &entryRoots only fills the set, so the collector's behaviour does not change. No defects found. Two non-blocking notes:
- GS traps on macOS (
MacOsImage.cpp:316) are still not checked against entry roots. #11090 has since merged, somainnow checks them for Windows (WindowsTlsBuilder.cpp:158-161), and the body's sentence "onmainthe Windows builder does not check them" is out of date. This is a separate gap for a follow-up PR, not part of this one. docs/dev/debt/relinker.md:33still names only the Windows TLS rewrite as failing instead of overwriting an entry point. The body explains why it is left unchanged here; it can be updated once #10479 and #10861 settle.
AI-assisted: yes (Claude Code applied the PR, ran the tests, read the code and wrote this review).
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewing exact head 9ef8b4b75abf646172f1b0cd8b683a21f15ba062.
What I ran
- Inspected the exact code and fixture changes, current review evidence, and hosted checks.
- Confirmed the branch includes a clean merge of main
88795ed448; current main has not changed the TLS scanner, code collector, or touched test file since that sync. - GitHub relinker jobs pass on macOS arm64, macOS x86_64, Linux, and Windows; ASan, conventions, labels, eligibility, and report checks pass.
- I did not run a local build or tests.
Results
| Check | Result | Evidence |
|---|---|---|
| Entry-root guard and regression fixture | Reviewed | Exact PR diff |
| Focused relinker tests | Pass, author-reported | 40 tests on the synced head; independent macOS reports give red/green 41/41 and 42/42 suites |
| Full relinker suite | Pass with known failures, author-reported | 307/310; two failures reproduced on main and one skipped |
| Hosted relinker platform matrix | Pass | macOS arm64/x86_64, Linux, Windows |
| Hosted ASan and policy checks | Pass | GitHub Actions |
| Local build and CTest | Not run | This review |
Findings
Blocking
None.
Non-blocking
- The guard uses the instruction length as the rewrite span and rejects only an entry root strictly inside it, preserving the boundary cases. The regression demonstrates the prior macOS output corrupting bytes addressed by
leaand the new macOS behavior refusing it, matching the existing Windows protection. - The discussion identifies the related
ScanTlsoverlap with #10861 and the separate broader merge conflicts. Reconcile both changes when integrating; the shared collector call must retain both the entry-root argument andextraEntries.
Sources
Not verified
- I did not execute the native/macOS builds or tests locally; platform and regression evidence is from hosted checks and PR participants.
To get this to a good state
No blocking issue remains in the reviewed head. Preserve the collector arguments when resolving #10861's overlapping change.
What
The macOS TLS scan asked
CodeInstructionCollector::Collectfor no entry roots and replaced every supportedfs:[disp32]access withjmp rel32and NOPs, even when a collector root lay inside the instruction. On untouched 4017310, an SCE fixture whose relocation pointer makes64 48 8b 04 25 00 00 00 00 c3at 0x240 code and whose function holdslea rax, [rip -> 0x244](0x244 taken as data) fails for--windowswithFAIL: TLS rewrite would overwrite an executable entry point (offset 0x244), but converts for--macoswith exit 0 andTook 1 pointer target(s) outside the unwind functions as data, the first at 0x244, and the bytes theleaaddresses are rewritten. The Windows TLS builder has refused this since #5890 and, for pointer targets taken as data, #8166.ScanTlsinMacOsImage.cppnow passes&entryRootstoCollect, asWindowsTlsBuilder::Builddoes, and before recording a supported access (load, store immediate, ALU read) throwsTLS rewrite would overwrite an executable entry pointat the root's file offset when a root lies strictly inside[instruction, instruction + length). The macOS rewrite span is the instruction length, never longer than the Windows span, and both targets use the same collector roots and access forms, so every input this refuses is already refused by--windows. Code roots inside anfsinstruction already fail in the collector withoverlapping instruction boundarieson both targets; the newly refused inputs are pointer targets taken as data in PS4 unwind-gap images. GS traps are unchanged; onmainthe Windows builder does not check them against entry roots either (fix(relinker): refuse GS traps that overwrite a code entry point in Windows output #11090 adds that check for Windows).docs/dev/debt/relinker.mdline 33 stays accurate and is not edited, so this PR does not conflict with fix(relinker): trap int 0x80 in a software interrupt table instead of refusing the image #10479 or fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 there.test_sce_code_extent.pygetslea_into_tls_fixture()and the casesce-tls-rewrite-over-data-target-<target>in the Windows/macOS loop, expectingTLS rewrite would overwrite an executable entry point (offset 0x244)and no output on both targets. After fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 (Felipebmaitan), which rewrites the sameCollectcall and changessce-tls-rewrite-over-lea-targetto expect the collector error, this case is the only SCE test of the entry-root rule on either target; whichever of the two merges second resolves theCollectconflict inMacOsImage.cppasCollect(source, headers, {}, &entryRoots, message, nullptr, extraEntries).pr_overlap.pycannot compare this branch with fix: define decoded AGC state and bound size arithmetic #10249 (MacOsImage.cpp) and fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 (MacOsImage.cpp,test_sce_code_extent.py), because both conflict withmain. Checked by hand: fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 changes the sameCollectcall inScanTls(line 293 onmain, lines 293 and 294 here), addingextraEntrieswhere this PR passes&entryRoots, and whichever lands second keeps theentryRootsdeclaration and passes both arguments,Collect(source, headers, {}, &entryRoots, message, nullptr, extraEntries). That is the only conflict this PR adds: a trial merge with fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 also conflicts inGuestModuleBuilder.cpp,RelinkerPipeline.cppanddocs/dev/debt/relinker.md, asmainalone does, in files this PR does not touch. fix(relinker): analyze referenced code beyond SCE scan boundaries #10861's newextraEntriesparameter ofScanTls(line 290 here), itsScanTlscall inWriteMacOsImage(line 456 here) and its new expectations forsce-entry-after-code-<target>andsce-tls-rewrite-over-lea-target(cases at lines 248 and 269 here) merge cleanly withlea_into_tls_fixture(line 184) and the new case (lines 241 and 242). fix: define decoded AGC state and bound size arithmetic #10249 changes one expression inWriteMacOsImage, the displacement of a TLS stub jump (line 636 onmain, 640 here), notScanTls(lines 290 to 345 here);MacOsImage.cppmerges without a conflict, and its conflicts withmainare inState.hppandlibSceJobManager/Export.cpp.Tested
Windows 11, GCC 15.2.0 (WinLibs UCRT), relinker-only Release build (
-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON); synthetic fixtures only, no title or homebrew.sce_code_extentwith the new case on untouched 4017310 fails withAssertionError: ('sce-tls-rewrite-over-data-target-macos', 0, ...); the Windows case before it passes. The same input run directly: Linux exit 0 with thefsload unchanged,--windowsexit 2 at offset 0x244,--macosexit 0 with thefsload replaced. With the change--macosexits 2 with the Windows message and Linux and--windowsare unchanged. Rebased onto 3a2c51a, which has no relinker changes since 4017310.main(88795ed) into the branch without conflicts (9ef8b4b; the earlier merge of 1be850b is b7d0697); since 3a2c51a, fix(relinker): mask embedded data tables in inter-FDE gaps #8076, feat(relinker): trace function imports on macOS #9408 and fix(relinker): accept PT_DYNAMIC data after DT_NULL #10561 changedMacOsImage.cpp,test_sce_code_extent.pyorCodeInstructionCollector.cpp, none of them in lines of this PR, and neither file of this PR changed after 1be850b;tools/check_main_sync.pyreported 0 behind and a clean merge at 88795ed. On 9ef8b4b,ctest -R 'sce_code_extent|macos_tls|tls_function_coverage|windows_tls_protected|macos_gs|macos_trace_imports' -j 6: 40 tests passed:sce_code_extent(2.4 s),macos_tls_alu,macos_tls_overlap,macos_gs_traps,macos_trace_imports,windows_tls_protected_entriesand the 34tls_function_coverage_*tests.ctest --test-dir build-relinker -j 6: 310 tests, 307 passed,long_pathsskipped; failures known on this machine onmainas well:select_tests(Windows symlink fixture) andpatch_advisor_unit(path with spaces, which open test(tools): compare quoted and resolved paths in the patch advisor test #10667 fixes); both failed again alone.python tools/check_conventions.py --base upstream/mainwith this title and description passed on 9ef8b4b.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)