Repository navigation
fix(relinker): reject a needed module name that several bundled modules match only by extension - #11479
fix(relinker): reject a needed module name that several bundled modules match only by extension#11479roboterspinne wants to merge 3 commits into
Conversation
…es match only by extension A needed name with no exact filename match was aliased to the first discovered module whose name differed only in its .prx, .sprx, .suprx or .debug_prx extension, so with two such modules the pick followed path order. GuestModuleBuilder now throws "Ambiguous needed module" naming both paths, as the other resolution paths in Build already do for ambiguous dependency names and module identities. Such an input now fails without an output instead of giving the name to one module by path order. A module whose filename or SONAME equals the needed name still wins, and a single extension match is still aliased.
…ous-needed-module
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The Linux, Windows, macOS arm64/x86_64 and relinker ASan checks pass, and the current merge preview is clean. The branch is now 80 commits behind current main, though, and the repository requires pull request branches to stay up to date. Please rebase onto current main and rerun the checks; I’ll recheck the updated head.
…ous-needed-module
THEMANnt
left a comment
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed f15e9852d169 against 427dd36164bb. No actionable findings.
The alias path records a second same-stem module and rejects an unresolved needed-name alias instead of choosing by path order. Exact filename and SONAME matches still claim the request, while guest-module dependencies remain unchanged. This prevents order-dependent aliasing while retaining single suffix matches.
Validation: Hosted relinker CTests passed on the pinned head: Windows 309, Ubuntu 313, and both macOS jobs 303 each; ASan’s 34 TLS-coverage tests passed. Full-product Windows/Linux jobs were skipped. Locally, the current-main-plus-patch relinker and test targets built with GCC 15.3; the focused 34-test selection passed with filesystem access for the unrelated hard-link fixture. A 12-run review-only sweep covered four suffix/directory ambiguity paths in Linux and Windows modes, reversed creation order, exact filename claims, input preservation, and no output on rejection. Unpatched main accepted the ambiguous fixture and wrote output.
The local tree lacked the pinned PR objects; relevant patch and fixture paths were unchanged across its 190-commit gap, but local execution is not the exact PR head. Hosted jobs tested exact head f15e9852d169.
|
@MarcZX10 Synced: ab4eb25 merges current |
|
Independent macOS validation of exact head
Environment: macOS 27.0.1 arm64, Apple clang 21.0.0, CMake 4.4.3, Ninja 1.12.1. I also reviewed the focused delta: the added coverage preserves an exact SONAME match and rejects two extension-only matches for the same requested module. I did not test a title or PS5 console behavior. |
|
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
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewing exact head ab4eb259b6da3aaeff8f6d671322b24226d88394.
What I ran
- Inspected the exact production and test changes, overlap report, review discussion, and current hosted checks.
- The branch's last synced base is
265f77bf99; current main has not changedGuestModuleBuilder.cpportest_guest_needed_modules.pysince that sync. - Hosted relinker checks pass on macOS arm64/x86_64, Linux, and Windows; ASan, conventions, labels, and eligibility pass.
- I did not run a local build or tests.
Results
| Check | Result | Evidence |
|---|---|---|
| Ambiguous suffix alias behavior | Reviewed | Exact PR diff |
| Focused regression and adjacent guest-module tests | Pass, author-reported | 34/34 on synced head; independent macOS focused and full suite pass |
| 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 resolver now collects all matching suffix stems, but only rejects ambiguity after exact filename/SONAME matches have had a chance to claim the needed name. Single suffix matches and exact SONAME matches are covered, along with ambiguous matches on Linux and Windows. This removes the prior path-order-dependent choice.
- #10861 overlaps in
GuestModuleBuilder.cppand has broader merge conflicts. The PR discussion manually reconciles the changed regions; retain both the TLS scan changes and this alias resolution when merging.
Sources
Not verified
- I did not run the relinker build or CTest locally; the author and independent reviewer reports plus hosted jobs provide the test evidence.
To get this to a good state
No blocking issue remains in the reviewed head. Preserve both PRs' changes when resolving the #10861 overlap.
What
A needed name with no exact filename match (on Windows, no case-folded match) is aliased to a bundled module whose name differs only in its
.prx,.sprx,.suprxor.debug_prxextension (1a9ef2f;.suprxsince 0a324d8), and when two discovered modules share that stem,std::find_ifinGuestModuleBuilder::Buildtakes the first module in the order they are passed in (the CLI sorts them by path) without checking for a second. On untouched 4017310 with--module-dir Media,Media/needed.prxandMedia/needed.sprxand an executable needingneeded.debug_prxconverted with exit 0 and the Linux executable'sDT_NEEDEDnamed only$ORIGIN/app0/Media/needed.prx.guest.prx; withMedia/needed.sprxandMedia/needed.suprxandneeded.prxit named onlyneeded.sprx.guest.prx, so the started module followed byte order of the paths. 1a9ef2f's message calls two candidates for one needed name ambiguous, but only the recursive outside-directory search threwAmbiguous needed module, and 41126bf removed it; the other resolution paths inBuildreject ambiguity (Ambiguous guest dependency name,Ambiguous guest module identity).neededAliases,Buildrecords a second module with the sameModuleStem(Windows folding as before) for each aliased name. In the alias loop, after the existingif (guestNames.contains(name)) continue;, it throwsAmbiguous needed module: <name> (<first path> and <second path>); the CLI exits 2 and writes no output. A module whose filename or SONAME equals the needed name still claims it, a single stem match is still aliased, and guest-module dependencies, which never use the stem alias, are unchanged. The rule covers every scanned module directory (sce_module,sce_modules,prxand--module-dir) and the modules the online converter passes.test_guest_needed_modules.pygets two cases for Linux and--windows, afterdebug-name-ambiguous(whosefirst/is not a module directory and is unchanged):sce_module/needed.prxwithsce_module/needed.sprxcarrying SONAMEneeded.debug_prxmust still convert, and the same pair without the SONAME, needingneeded.debug_prx, must exit 2 with the message and no output.pr_overlap.pyreports no shared lines with open pull requests that merge withmain. It cannot compare this branch with fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 (GuestModuleBuilder.cpp), because that pull request does not merge withmain. Checked by hand: fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 changes only the syscall scan of the code segments inBuild(lines 403 to 433 onmain, 412 to 442 here), not theneededAliaseslookup and alias loop that this PR changes (lines 112 to 180 here, 112 to 171 onmain). A trial merge of this branch with fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 conflicts in the same three files and regions asmainwith fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 (GuestModuleBuilder.cpp,RelinkerPipeline.cppanddocs/dev/debt/relinker.md). InGuestModuleBuilder.cppboth conflicts are in the unwind range setup and the masked syscall scan that fix(relinker): mask embedded data tables in inter-FDE gaps #8076 added (lines 404 to 408 and 414 to 423 onmain, 413 to 417 and 423 to 432 here). Apart from the conflict labels, the conflictedGuestModuleBuilder.cppof the two trial merges differs only in the lines of this PR, which merge cleanly, and the other two conflicted files are identical. fix(relinker): skip backup copies of guest modules in the online converter too #10997 also editsGuestModuleBuilder.cpp; it merges with this branch without conflict and appendsRemoveBackupCopiesafterBuild(which ends at line 499 here), far from those lines.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.guest_needed_moduleswith the new cases on untouched 4017310 fails at the Linux same-stem case withAssertionError: (<stdout>, ''), return code 0 and output written, after the SONAME control passed. Run directly with the same inputs, untouched Linux and--windowsboth exit 0 for both cases; the untouched Windows program runs and exits 42. With the change the same-stem case exits 2 for both targets with no output, the SONAME case still exits 0 and its Windows program exits 42, and the two--module-dir Mediainputs above exit 2, as doessce_module/needed.prxwithMedia/needed.sprxunder--module-dir Mediafor both targets (untouched: exit 0, LinuxDT_NEEDEDlists both). With the changeguest_needed_modulespassed in 3.5 s on 3a2c51a, which has no relinker changes since 4017310.main(265f77b) into the branch without conflicts (ab4eb25; the earlier merge of 518d36e is f15e985); since 3a2c51amainchanged both files of this PR (fix(relinker): mask embedded data tables in inter-FDE gaps #8076, fix(relinker): validate every entry of the Linux run path #10607, test(relinker): decode relinker output as UTF-8 in the path-checking tests #9513, fix(relinker): reject non-TLS guest relocations bound to a guest TLS export #10732), none of them in lines of this PR, and neither file since 518d36e;tools/check_main_sync.py --base upstream/mainreports 0 behind and a clean merge at 265f77b. On ab4eb25,ctest -R 'guest_module_directories|needed|guest_|module' -j 6(run next to a second build): 34 tests passed, among themguest_needed_modules(5.4 s),guest_module_dir(8.0 s),guest_module_directories(3.3 s),guest_module_identity(8.7 s),guest_needed_nested_modules(5.0 s),guest_deferred_dependencies(1.3 s),guest_runpath(3.3 s) andlinux_guest_named_imports(1.1 s).ctest --test-dir build-relinker -j 6: 309 tests, 306 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 ab4eb25.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)