Repository navigation
fix(relinker): reject macOS fixups outside writable segments - #11045
adriadam10 merged 3 commits into
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head d42c3dc7fd72722939c9c1ae6d0af7e2b4e065bc. The shared Locate guard checks that the full 8-byte fixup target is inside a segment and that the segment has write permission, covering both rebase and bind emission. The new writable-data success case and read-execute rejection case exercise the boundary; the existing interpreter fixture is made writable for its relocation. The author reports a negative control and 292-test relinker run; I reviewed the test/code and hosted Linux, Windows, macOS arm64/x86_64, ASan and conventions checks pass. Product-specific jobs are skipped by path policy. I did not independently run the macOS-target fixture locally.
|
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.
|
maximilianfv
left a comment
There was a problem hiding this comment.
The Locate check is correct and needed: on macOS 27.0.1 the output it now rejects dies inside dyld before main, and every other test and fixture still converts and runs. The code needs no change. The only change requested is one sentence in the debt entry, which says dyld refuses fixups in executable segments, but here such an output loads and runs.
Tested on Apple M1 Pro, macOS 27.0.1 (x86_64 build under Rosetta 2)
Applied at d42c3dc7fd to mugurc's macOS series (27e968974) merged with main at 2742004ed2, with other open macOS fixes, then toggled only this PR. Full rebuild each way (the relinker, relinker_conversion_tests, relinker_game_patch_tests and the other librelinker_core users relinked). The direct runs use relinker --macos --skip-sce-module on import_fixture() with the extra RELATIVE at 0x5C30 and DT_RELASZ 72 that check_fixup_protection builds, run with libc.prx and libkernel.prx from the same build in libs/.
| Without this PR | With this PR | |
|---|---|---|
63 relinker tests (macos_*, relinker_*, *tls*, interpreter_in_code, sce_relro, sce_code_extent, elf_offsets, none_relocations, optional_rela, overlapping_relocations, windows_bss, game_patches, cli_args, ...) |
63/63 passed | 63/63 passed |
macos_output with only the test changes (no Locate line) |
Failed: fixup-code: rebase into a non-writable segment accepted: 0 |
Passed |
| No extra fixup | exit 0, prints hello from a PS5 ELF on macOS |
the same |
RELATIVE to 0x5200 (PF_R|PF_W) |
exit 0, runs and prints | the same |
RELATIVE to 0x4100 (PF_R|PF_X) |
exit 0. dyld_info -fixups: pointer rebase is in non-writable segment '__ELF0'. Running it: SIGBUS, EXC_BAD_ACCESS (code=2, address=0x140010100) in dyld4::Loader::applyFixupsGeneric |
exit 2, FAIL: Mach-O fixup target is not in a writable segment (offset 0x10100), no output |
RELATIVE to 0x4100 with the code PT_LOAD set to PF_R|PF_W|PF_X |
exit 0. dyld_info -fixups: pointer rebase is in executable segment '__ELF0'. Runs and prints, exit 0 |
the same |
So on this host, dyld does not refuse the non-writable rebase with a message. It writes the rebase and faults on the read-only page. Either way the output never reaches main, which justifies the rejection.
docs/dev/debt/relinker.md:37: "macOS output still writes pointer fixups into executable segments, which dyld refuses ... Not run on macOS." On macOS 27.0.1 under Rosetta, an output with a rebase into anRWX__ELF0loads, applies the fixup and runs. Onlydyld_inforeports it, as a diagnostic. The output declaresminos 11.0,sdk 11.0inLC_BUILD_VERSION, which may be why dyld enforced neither rule here: thePF_R|PF_Xcase died on the page fault, not on a refusal. Fix: state what was measured (dyld_info flags it, and dyld on macOS 27.0.1 x86_64 under Rosetta loads and runs it), or name the dyld version and declared SDK that refuse it.- Optional,
core/relinker/elfpatcher/src/macos/MacOsImage.cpp:192: the message gives the writer's RVA (0x10100 for the ELF address 0x4100). The other relocation errors,Runtime import target is not writablefromWindowsLoadImage::RequireWritable, give the ELF address, so 0x10100 cannot be found withreadelf -r. Fix: report the ELF address. - Optional,
core/relinker/relinker/tests/test_interpreter_in_code.py:27: this test also converts with--windows. With flags 6 it no longer covers aRELATIVEinto a read-onlyPT_LOADin PE output, which the PR keeps valid on purpose. I did not check whether another test covers that. Fix: keep flags 4 for the--windowsrun and use 6 only for--macos.
AI-assisted: yes (Claude Code applied the PR, ran the tests and the fixtures, read the code and wrote this review).
|
Thanks for the macOS measurements, they are the first runs of these fixup cases on a Mac. Addressed in 397d534, a new commit on top of d42c3dc. Point 1 taken: the debt entry no longer claims a refusal. It now states what you measured, that Point 3 taken: Point 2 not taken: Rerun on Windows 11 with a relinker-only Release build: the 18 macOS and rebase tests ( |
|
Tested head 397d534 merged onto main aeff26b (clean) on Windows 11 (Ryzen 5 9600X), WinLibs GCC 15.2.0, Release.
AI-assisted: yes (Claude Code); results above were run on this machine. |
maximilianfv
left a comment
There was a problem hiding this comment.
Approving at 397d534c8. The requested change is in: the debt entry now states what was measured instead of claiming that dyld refuses the case.
- Point 1 (requested): resolved.
docs/dev/debt/relinker.mdno longer says dyld refuses fixups in executable segments. It records thedyld_info -fixupsdiagnostics, the RWX case that loads and runs here, and the RX case that ends with SIGBUS indyld4::Loader::applyFixupsGeneric, and marks other versions as not measured. - Point 3 (optional): resolved.
test_interpreter_in_code.pypasses flags 6 only for--macos, so the--windowsrun keeps covering aRELATIVEinto a read-onlyPT_LOAD. - Point 2 (optional): your reasoning holds.
Locateonly has RVAs, and the writer's own__APS5DATAfixups have no ELF address, so the RVA, which matches the otherLocatemessages and converts as you describe, is fine.
MacOsImage.cpp and the other code are unchanged since d42c3dc7fd, so my earlier macOS results still apply to this head. Those were 63/63 relinker tests passing both ways, and the PF_R|PF_X fixture going from a SIGBUS in dyld to a clean conversion error.
AI-assisted: yes (Claude Code compared the two heads, checked the replies against the code and wrote this review).
jotitadev
left a comment
There was a problem hiding this comment.
Merge-skew check: head 397d534c8 merged onto current main (1a7ac56d27, also reproduced on abed035468) breaks dynamic_segment. The PR's CI is green because it ran before #10561 landed.
Apple M2 Max, macOS 27.2, Xcode 27, relinker-only Release arm64 (-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON), full suite.
mainalone (8aa1f9ef7f): 355/355.main(8aa1f9ef7f) + this PR: 354/355, onlydynamic_segmentfails (3/3 runs, not flaky):
('strict-trailing-bytes-after-null', ['--macos', 'unused-filter=2'], 2, ...
'FAIL: Mach-O fixup target is not in a writable segment (offset 0x10300)')
Cause: #10561 (68f5474, merged in 47d389e) added strict-trailing-bytes-after-null to test_dynamic_segment.py, and it also runs in --macos unused-filter=2. Its strict_fixture() sets the single PT_LOAD to PF_R|PF_X (flags = 5) so the strict filter accepts the layout, but the inherited optional_plt_fixture() keeps a R_X86_64_RELATIVE at 0x300 inside that segment. The new Locate check correctly rejects that rebase. Making the segment writable (7) does not work, because the strict filter then fails with unsupported executable segment layout.
Minimal fix in the test, keeping what the case checks (DT_NULL tail handling under the strict filter): drop the RELATIVE relocation from the strict fixture.
def strict_fixture():
image = optional_plt_fixture([(4, 0x6a0)])
struct.pack_into("<I", image, 64 + 4, 5)
+ # No RELATIVE fixup: a Mach-O fixup into this read-only executable segment is rejected.
+ struct.pack_into("<Q", image, 0x400 + 5 * 16 + 8, 0)
struct.pack_into("<IIII", image, 0x6a0, 1, 1, 0, 0)
return image(0x400 + 5 * 16 + 8 is the value of DT_RELASZ.) With it, dynamic_segment passes 3/3 with this PR, dynamic_segment|macos|interpreter_in_code is 11/11, and the test still passes on main without the PR.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
The macOS image writer emitted dyld rebase and bind opcodes for any target inside a segment, so an R_X86_64_RELATIVE into an R-X PT_LOAD became a rebase into a read-execute segment. dyld-1378 refuses fixups in segments without write access (enforceFixupsInWritableSegments), so Locate now throws when the target segment lacks VM_PROT_WRITE. Such inputs now fail the macOS conversion instead of writing an output that dyld would not load.
The debt entry on pointer fixups in executable segments claimed that dyld refuses them, which no run had shown. It now states what was measured on an M1 Pro running macOS 27.0.1 under Rosetta 2: dyld_info reports both cases, the PF_R|PF_W|PF_X case loads and runs, and a fixup into a PF_R|PF_X segment ends with SIGBUS in dyld. The interpreter_in_code fixture keeps a read-only data PT_LOAD for the Windows runs, so PE output still covers a RELATIVE into it.
…nt fixture Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
397d534 to
9e66ff6
Compare
jotitadev
left a comment
There was a problem hiding this comment.
Follow-up on the merge skew I reported: fixed at head 9e66ff6b2.
Apple M2 Max, macOS 27.2, Xcode 27, relinker-only Release arm64 (-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON), this head merged onto current main 5d52b06ff3 (clean merge).
- Full suite: 361/361.
dynamic_segment: 3/3 pass.- Control: same tree with
test_dynamic_segment.pyreverted tomain's version,dynamic_segmentfails again withMach-O fixup target is not in a writable segment (offset 0x10300)onstrict-trailing-bytes-after-null, so the fixture change is what makes it pass.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
There was a problem hiding this comment.
Re-review at 9e66ff6b2: approve. The Locate check and its tests are unchanged since 397d534c8. The new fixture commit removes the clash with #10561, and the full relinker suite passes on current main with this PR.
Tested on Apple M1 Pro, macOS 27.0.1 (x86_64 build under Rosetta 2)
A relinker-only Release build (-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON -DCMAKE_OSX_ARCHITECTURES=x86_64) of main at 801d5b9bc1, with and without the diff at 9e66ff6b2. The full suite ran with -E 'audio_in|voice'.
| full relinker suite | macos_output |
dynamic_segment |
interpreter_in_code |
|
|---|---|---|---|---|
| Without this PR | 359/359 passed | Passed | Passed | Passed |
| With this PR | 359/359 passed | Passed (3/3 repeats) | Passed (3/3 repeats) | Passed (3/3 repeats) |
PR, but test_dynamic_segment.py from main |
Failed: strict-trailing-bytes-after-null, --macos unused-filter=2, FAIL: Mach-O fixup target is not in a writable segment (offset 0x10300) |
|||
PR tests, but MacOsImage.cpp from main |
Failed: fixup-code: rebase into a non-writable segment accepted |
Passed | Passed |
So the fixture change in 9e66ff6b2 is required once #10561 is in, and the new macos_output case fails without the Locate check. The x86_64 build adds a second architecture to jotitadev's arm64 run.
9e66ff6b2 sets DT_RELASZ to 0: index 5 of test_optional_plt.fixture's tag list is (8, 24). That leaves the DT_NULL-tail parsing that both strict cases test unchanged, and convert() never runs the output.
Non-blocking:
- The PR body is stale for this head.
Whatdoes not mentiontest_dynamic_segment.pyor the clash with #10561. The sentence "With 4 it was the only existing test with a fixup into a non-writable segment of macOS output" no longer holds, becausestrict_fixturewas a second one.Testedstill says "Rebased onto e05ba6f" and has no run of this head. The line numbers in the Overlap bullet also need updating: the GS debt bullet is at line 41 ofdocs/dev/debt/relinker.mdnow, andLocatestarts at line 222 ofMacOsImage.cpp. core/relinker/relinker/tests/test_dynamic_segment.py:24is jotitadev's suggested line, word for word, from their review. The commit and body do not credit them. Suggestion: add aCo-authored-by: Jota <139649225+jotitadev@users.noreply.github.com>trailer or a line in the body.- Minor: the line zeroes
DT_RELASZ, so the strict Linux and Windows runs also lose theirRELATIVErelocation, not only--macos. The subject "drop the RELATIVE fixup" describes the effect;StrictNidFilterTestsstill covers the relative-relocation index directly.
AI-assisted: yes (Claude Code applied the PR, ran the tests, read the code and wrote this review).
|
@maximilianfv thanks. I updated the description for 9e66ff6: the Tests bullet now describes the clash with #10561's |
|
Checked the Added one commit on top, from @jotitadev's merge-skew report: since #10561, |
What
The macOS image writer emitted a dyld fixup for any target inside a segment, whatever the segment's protection: on untouched 4017310,
import_fixture()with an addedR_X86_64_RELATIVEat 0x4100 (inside thePF_R|PF_XPT_LOAD) converts withrelinker --macos --skip-sce-module, exits 0, and itsLC_DYLD_INFO_ONLYrebase stream holds a rebase at__ELF0+0x100, a segment withinitprot5. maximilianfv ran such an output on an Apple M1 Pro with macOS 27.0.1 (x86-64 under Rosetta 2,LC_BUILD_VERSIONminos and sdk 11.0): it ends with SIGBUS indyld4::Loader::applyFixupsGenericbeforemain, anddyld_info -fixupsreportspointer rebase is in non-writable segment '__ELF0'.Locateincore/relinker/elfpatcher/src/macos/MacOsImage.cpp, which both the rebase loop and the bind loop call, throwsMach-O fixup target is not in a writable segmentwith the target RVA when the segment holding the 8-byte target lacksVM_PROT_WRITE; the CLI exits 2 and writes no output. Only an executableR_X86_64_RELATIVEinto a non-writablePT_LOADreaches it today:WindowsRelocationBuilder::Applyadds it to the base relocations withoutRequireWritable, while executable imports (R_X86_64_64,GLOB_DAT,JUMP_SLOT) and every guest-module relocation inMacOsGuestModuleWriterpassRequireWritablefirst, an executableR_X86_64_64,GLOB_DATorJUMP_SLOTagainst a defined symbol is refused, and the metadata and TLS descriptor fixups lie in theRW__APS5DATA. The Windows output is unchanged: PE base relocations into read-only sections are valid. Pointer fixups in executable segments, whichdyld_info -fixupsalso reports, are not rejected: it would require changes to seven existing fixtures that rebase intoRWXsegments, and on the same M1 Pro dyld loads and runs aPF_R|PF_W|PF_Xcase; this is recorded with those measurements indocs/dev/debt/relinker.mdafter the macOS GS entry.test_macos_output.pygets a rebase opcode decoder (rebases) andcheck_fixup_protection, called aftercheck_runpath_lists: the fixture adds aRELATIVEat file offset 0x5C30 and setsDT_RELASZto 72. Target 0x5200 (PF_R|PF_W) must exit 0 with a rebase at('__ELF1', 0x200); target 0x4100 (PF_R|PF_X) must exit 2 withFAIL: Mach-O fixup target is not in a writable segment (offset 0x10100)on stderr and no output.test_interpreter_in_code.py:fixturetakes the flags of the dataPT_LOADthat itsRELATIVEtargets; the--macosruns use 6, the--windowsruns keep 4, so the PE output still covers aRELATIVEinto a read-onlyPT_LOAD. With 4 it was one of two existing tests with a fixup into a non-writable segment of macOS output (__ELF2,initprot1). The other came with fix(relinker): accept PT_DYNAMIC data after DT_NULL #10561:strict_fixtureintest_dynamic_segment.pymarks its singlePT_LOADPF_R|PF_Xbut kept theRELATIVEat 0x300 ofoptional_plt_fixture, and itsstrict-trailing-bytes-after-nullcase also runs with--macos unused-filter=2, which this check refuses (jotitadev's merge-skew report). 9e66ff6, jotitadev's suggested line applied by adriadam10, sets that fixture'sDT_RELASZto 0, so both strict cases still test theDT_NULLtail, now without aRELATIVErelocation in the Linux,--windowsand--macosruns alike.pr_overlap.pyreports no shared lines with open pull requests that merge withmain. It cannot compare this branch with feat(aot): consolidate native ARM64 compiler, runtime and consumers #5021 and fix(relinker): mask embedded data tables in inter-FDE gaps #8076 (docs/dev/debt/relinker.md) and with fix: define decoded AGC state and bound size arithmetic #10249 (MacOsImage.cpp), because those do not merge withmain. Checked by hand: the debt entry of this PR is a new bullet after the one on GS accesses in macOS output (line 41 onmain); feat(aot): consolidate native ARM64 compiler, runtime and consumers #5021 adds a macOS TLS bullet before the--trace-importsbullet and fix(relinker): mask embedded data tables in inter-FDE gaps #8076 adds a bullet on embedded data and relative jump tables after thesce_modulesyscall scan bullet, both near the top of the list. fix: define decoded AGC state and bound size arithmetic #10249 changes one expression inWriteMacOsImage, the displacement of a TLS stub jump (line 595 of its base), notLocate(line 222 onmain), where this PR adds the writable-segment check. The trial merges of this branch with each of them conflict in the same files and regions asmaindoes, none of them in lines of this PR.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.macos_outputwith the new test and without theLocatecheck fails withfixup-code: rebase into a non-writable segment accepted: 0after the 0x5200 control passed (e05ba6f with theLocateline removed). With the change it passes in 0.6 s.interpreter_in_codewith flags 6 for--macosand 4 for--windowspasses with the change and with e05ba6f'sMacOsImage.cpp; with flags 4 for both targets it passes without the change and fails with it on its first--macoscase, after the three--windowscases, withFAIL: Mach-O fixup target is not in a writable segment (offset 0x18300); with flags 6 for both it passes.ctest -Ron 15 tests:macos_outputandinterpreter_in_code; the seven with rebases intoRWXsegments (elf_offsets,none_relocations,optional_rela,overlapping_relocations,sce_code_extent,windows_bss,macos_fixed_base);relinker_conversionandrelinker_game_patch(C++ suites that convert to macOS);relinker_executables,guest_none_relocations,game_patchesandcli_args: 15 passed in 32.8 s.ctest -R 'macos|tls|sce_relro': 50 passed in 116.2 s (machine under load from parallel builds).interpreter_in_coderuns above ran on the follow-up commit 397d534, with and without theLocatecheck.ctest --test-dir build-relinker -j 4on e05ba6f: 292 tests, 289 passed,long_pathsskipped; failures known on this machine onmainas well:select_tests(Windows symlink fixture) andpatch_advisor_unit(path with spaces, fixed by open test(tools): compare quoted and resolved paths in the patch advisor test #10667); both fail again when run alone.Whatparagraph reworded to his macOS measurements,interpreter_in_codeflags per target), rebuilt:ctest -Ron 18 tests (macos_output,interpreter_in_code, the seven with rebases intoRWXsegments,relinker_conversion,relinker_game_patch,guest_none_relocationsand the six othermacos_tests) passed in 50.4 s; full suitectest --test-dir build-relinker -j 6: 292 tests, 288 passed,long_pathsskipped,select_testsandpatch_advisor_unitfailed as above, andshader_cache_runtime_sourcestimed out at 180 s, alone as well, and passes in 235 s when run without the ctest timeout; it builds the shader recompiler, which this PR does not touch.git merge-treeof the branch with upstream/main (6bc1139), fix(relinker): reject --lazy-binding for macOS output #10712, fix(relinker): accept a zero-size TLS export at the end of the macOS TLS block #10946, fix(relinker): reject Windows executable imports bound into the TLS template #10947, fix(relinker): reject nonzero GLOB_DAT and JUMP_SLOT addends in Linux output #10971, fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 and fix(relinker): lower ADCX and ADOX for Rosetta #7173: clean.main1392606 (e529726 and 0326845, the same changed lines as d42c3dc and 397d534) and added 9e66ff6. That head merged withmain8f25510: full suitectest --test-dir build-relinker -j 6: 368 tests, 365 passed,long_pathsskipped, onlyselect_testsandpatch_advisor_unitfailed as above;dynamic_segment,macos_outputandinterpreter_in_codepassed. On macOS, jotitadev ran the full suite of 9e66ff6 merged ontomain5d52b06 on arm64 (361/361), and maximilianfv ran it withoutaudio_in|voiceonmain801d5b9 as x86_64 under Rosetta 2 (359/359).python tools/check_conventions.py --base upstream/mainwith this title and description passed.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)