Repository navigation
fix(module): resolve the default dlsym scope against the main program - #864
Conversation
adriadam10
left a comment
There was a problem hiding this comment.
Built and ran ctest merged onto current main (Linux, all green); guest_dynamic_loader fails without the change, good. A few things before this can go in:
- Handle 0 reaches host symbols. On Linux
MainProgramModule()is::dlopen(nullptr, ...)(DynamicLoader.cpp:73), i.e. the whole process scope, andFindSymboltries the plain name before the NID. A probe inguest_dynamic_loader_testsgets non-null fordlsym(0, "pthread_create"),"dlopen"and"malloc": host glibc wins over the guest exports. This contradicts "nothing reaches for host glibc" in the description. On PS5, handle 0 is the main program and the lookup only searches its export table (KytyPS5KernelDlsym,FindProgramById(handle)->export_symbols). Handle 0 should only see guest exports (NID /#guestnames), not the host process scope. - The references disagree on
scriptingGetMem. prosper (docs/archive/ROADMAP_HISTORY.md, IL2CPP bring-up) madesceKernelDlsymreturn ESRCH forscriptingGetMem"instead of fake success, keeping the caller's fallback", and the title then finishesil2cpp_init. Only KytyPS5 special-cases the name. Since the name exists in none of the title's binaries, ESRCH looks like what real hardware does. If you have evidence that the export exists on PS5, please cite it. Otherwise the default-scope fix alone (with an honest not-found) may be enough. Please check whether your title still boots that way. - Windows.
GetModuleHandleW(nullptr)only covers the executable, whilescriptingGetMemlives in the libc DLL, so the new assertions inguest_dynamic_loader(also registered on Windows) look like they will fail there. CI hasn't run on this head yet. - If the export stays: the
(alignment, bytes)signature is inferred, so it needs a line under "Unknown function info" indocs/dev/TechnicalDebt.md. The test should also check the requested alignment (& 0xFFF), not just 16 bytes (GuestDynamicLoader.cpp:55), and coverscriptingFreeMem.
|
Thank you for the careful review - the handle-0 finding is a real bug and the
So I have no evidence the export exists on PS5 and no evidence the title needs it - your reading is right and the fake allocator was my inference, not a measurement. I will rework this PR to be the default-scope fix only, with an honest not-found, and drop the scripting heap (kept locally as a patch in case a later symptom points at it). If it comes back at all it goes in with the Handle 0 reaching host symbols is a genuine defect and I will fix it, not document it. You are right that it contradicts my description, and the probe is decisive: Windows. Agreed, and I had not verified it: One question so I fix the right thing: for handle 0, should a name that exists both as a guest export and as a host symbol resolve to the guest (my reading of PS5 semantics), and should |
654f9c4 to
7f5eacc
Compare
|
All four points accepted; the head is rewritten. Handle 0 no longer reaches host symbols. The default-scope fallback is now filtered by
So the default-scope fix stands on its own, the scripting heap is dropped from this PR, and no Windows. The assertions and the process-scope fallback are both inside Comments. Removed - you were right that the description claimed there were none while the diff carried them. The head has no added comments. CI. Not green on the head yet, and one blocker I cannot fix from my side: I cannot rebase onto current |
adriadam10
left a comment
There was a problem hiding this comment.
Built and ran ctest merged onto current main (Linux, 400/400 green). Three things left:
- The test doesn't cover the fix. With the
DynamicLoader.cppchanges reverted,guest_dynamic_loaderstill passes: onmainhandle 0 is rejected outright, sodlsym(0, "malloc")etc. are already null (GuestDynamicLoader.cpp:92-94). Your "verified to bite" check swapped in a different broken version, notmain. The new behaviour, handle 0 finding an export, isn't asserted anywhere (handle -2 on a global module was already covered at line 40). Please add positive cases:dlsym(0, "GuestModuleAdd")on the RTLD_GLOBAL module from the test, and one export that is only reachable through the process-scope fallback (e.g. a.prxexport by NID). leaf == "app.elf"(DynamicLoader.cpp:59) depends on the file name. The output name is the user's choice (relinker input.elf output.elf;app.elfin USAGE.md is only an example), so a renamed executable has its exports silently rejected. Identify the main program by the object itself instead, e.g. compareinfo.dli_fbasewith the main executable's base (dladdron a symbol of the executable, or the firstdl_iterate_phdrentry).- CI / rebase: no checks have run on this head. Pushing a branch that contains main's workflow commits needs the
workflowscope (gh auth refresh -s workflow), or use "Sync fork" in the GitHub UI and rebase on that.
|
@adriadam10's first point — "the new behaviour, handle 0 finding an export, isn't asserted anywhere" — is a one-line addition next to the handle -2 check he notes is already covered ( Require(dlsym_nid_postfix(reinterpret_cast<void*>(-2), "GuestModuleAdd") == reinterpret_cast<void*>(add));
Require(dlsym_nid_postfix(nullptr, "GuestModuleAdd") == reinterpret_cast<void*>(add));Nothing else changes. Verified by reverting only
The revert is confirmed to be a real one: Two honest limits.
The remaining points on the review — handle 0 reaching host symbols, the Happy to open a PR against |
7f5eacc to
c3ffd7f
Compare
|
You were right, and my earlier "verified to bite" claim was wrong. I checked it your way this time. Head is What I did wrong. My check replaced the filter with a bare What I added. The positive case @Dextroid17 proposed: Require(dlsym_nid_postfix(nullptr, "GuestModuleAdd") == reinterpret_cast<void*>(add));Verified in both directions on this head:
One limit I share with @Dextroid17. The file-name dependency is gone. CI and rebase. I still cannot rebase onto current |
|
Thanks — the corrected negative control is exactly the check that was missing, and the table is the shape I would want: loader from Agreed on the limit, and I would leave it named rather than papered over. On the rebase blocker, the facts are on your side. GitHub refuses any push that modifies
I have no preference between them and it is not mine to set — either gets the head onto current Nothing else from me on the code. The load-base identification ( Written with AI assistance. |
|
On the remaining gap, a test where only the process-scope fallback can resolve the symbol: the test executable itself can provide one. --- a/core/libs/prx/libkernel/CMakeLists.txt
+++ b/core/libs/prx/libkernel/CMakeLists.txt
@@ add_dependencies(guest_dynamic_loader_tests guest_module_fixture)
+ if(NOT WIN32)
+ set_target_properties(guest_dynamic_loader_tests PROPERTIES ENABLE_EXPORTS ON)
+ endif()
--- a/core/libs/tests/GuestDynamicLoader.cpp
+++ b/core/libs/tests/GuestDynamicLoader.cpp
@@ static void Require(bool value) { if (!value) std::abort(); }
+#ifndef _WIN32
+extern "C" int APS5_VABI GuestDefaultScopeOnly(int a, int b) { return a * b; }
+#endif
@@ Require(dlsym_nid_postfix(nullptr, "pthread_create") == nullptr);
+ Require(dlsym_nid_postfix(nullptr, "GuestDefaultScopeOnly") == reinterpret_cast<void*>(&GuestDefaultScopeOnly));
+ Require(dlsym_nid_postfix(reinterpret_cast<void*>(-2), "GuestDefaultScopeOnly") == reinterpret_cast<void*>(&GuestDefaultScopeOnly));Checked on Ubuntu 24.04, GCC 14.2, on top of
The third row is the one that was missing: the test fails when only the fallback is removed. Full suite on this head plus the test: 392/392 passed. Feel free to take it into this PR. AI-assisted: yes (Claude Code). |
adriadam10
left a comment
There was a problem hiding this comment.
Built and ran ctest merged onto current main (413/413 green); with DynamicLoader.cpp reverted, guest_dynamic_loader now fails, so the handle-0 → guest export assertion pins the fix. Thanks for redoing that check.
One thing left:
- The new fallback is never executed by any test.
MainProgramBase,FromGuestModuleandDefaultScopeSymbol(DynamicLoader.cpp:54-84, call site:163-167) are only reached when the module loop finds nothing, and the three host-name asserts (GuestDynamicLoader.cpp:93-95) pass identically without that path, so the dladdr filter itself is unpinned. The existing fixture can cover it:::dlopenthe test guest.prxdirectly withRTLD_GLOBAL(so it is not in the loader's module map) and assertdlsym_nid_postfix(nullptr, "<its export>")returns it, and that a host symbol still returns null. - Minor:
info.dli_fbase == dlpi_addr(:75) only matches for PIE executables (non-PIE hasdlpi_addr == 0). Either compare against the executable's actual mapping start or note the PIE assumption in TechnicalDebt.
Not verified on our side: the title run (PPSA28997).
|
Following up with @adriadam10's suggestion (host return path.size() > 5 && path.compare(path.size() - 5, 5, ".prx") == 0;
Fix plus test (on top of @@ -74,7 +74,7 @@ bool FromGuestModule(void* address) {
if (address == nullptr || !::dladdr(address, &info) || info.dli_fname == nullptr) return false;
if (reinterpret_cast<std::uintptr_t>(info.dli_fbase) == MainProgramBase()) return true;
const std::string path(info.dli_fname);
- return path.size() > 5 && path.compare(path.size() - 5, 5, ".prx") == 0;
+ return path.ends_with(".prx");
}
void* DefaultScopeSymbol(const char* symbolName) {@@ -4,6 +4,9 @@
#include <fstream>
#include <stdexcept>
#include <thread>
+#ifndef _WIN32
+#include <dlfcn.h>
+#endif
extern "C" {
void* APS5_VABI dlopen_nid_postfix(const char*, int);
void* APS5_VABI dlsym_nid_postfix(void*, const char*);
@@ -98,5 +101,15 @@ int main(int argc, char** argv) {
Require(dlsym_nid_postfix(nullptr, "pthread_create") == nullptr);
Require(dlsym_nid_postfix(nullptr, "GuestDefaultScopeOnly") == reinterpret_cast<void*>(&GuestDefaultScopeOnly));
Require(dlsym_nid_postfix(reinterpret_cast<void*>(-2), "GuestDefaultScopeOnly") == reinterpret_cast<void*>(&GuestDefaultScopeOnly));
+ const auto hostLoaded = std::filesystem::absolute("anyps5-host-loaded-module-for-test.prx");
+ std::filesystem::copy_file(argv[1], hostLoaded, std::filesystem::copy_options::overwrite_existing);
+ void* host = ::dlopen(hostLoaded.c_str(), RTLD_NOW | RTLD_GLOBAL);
+ Require(host != nullptr);
+ void* hostAdd = ::dlsym(host, "3qcG+yBRS+s");
+ Require(hostAdd != nullptr);
+ Require(dlsym_nid_postfix(nullptr, "GuestModuleAdd") == hostAdd);
+ Require(dlsym_nid_postfix(nullptr, "malloc") == nullptr);
+ Require(::dlclose(host) == 0);
+ std::filesystem::remove(hostLoaded);
#endif
}The fixture is
Full suite with the fix and both tests: 392/392 passed. I didn't address the PIE point ( AI-assisted: yes (Claude Code). |
c3ffd7f to
888c8b0
Compare
|
Confirmed from the head, and the bug is exactly as you describe. return path.size() > 5 && path.compare(path.size() - 5, 5, ".prx") == 0;
That also closes the fallback gap I noted a few comments up — between your Same limitation on my side: this is from reading the head, not a rebuild (my box is NVIDIA-only), and I can't run the Windows path either. The case is inside the existing Written with AI assistance. |
adriadam10
left a comment
There was a problem hiding this comment.
Built and ran ctest merged onto current main (440/440 green); with DynamicLoader.cpp reverted, guest_dynamic_loader fails, so the handle-0 assertions pin the change. Two things left:
- The
.prxbranch ofFromGuestModulenever matches (DynamicLoader.cpp:77):path.compare(path.size() - 5, 5, ".prx")compares 5 characters against a 4-character string, so it is never 0. The fallback therefore only returns symbols from the main program, and symbols from.prxlibraries (the #694 case) are still rejected.path.ends_with(".prx")fixes it. - The fallback is still not executed by any test.
DefaultScopeSymbolis reached only when the module loop finds nothing, and themalloc/dlopen/pthread_createasserts pass without it. hikazey posted two tests in this thread that cover it: an exported symbol in the test executable (ENABLE_EXPORTS, non-Windows) and a hostdlopen(..., RTLD_GLOBAL)of the fixture. Please add them (with the.prxfix, the second one fails without it) and check they fail with bothDefaultScopeSymbolcalls removed.
Also add Refs #694 to the description, as fisherjoey suggested.
cf33f7d to
5aa7a5b
Compare
|
Both points are fixed, and I verified each control myself. Head is The
Both tests are in, and I re-ran every control @hikazey asked for. I took @hikazey's two tests as posted - the
The second row is the one that proves the Windows. I could not compile for Windows here: no MinGW toolchain on this machine. The two new assertions and the fallback itself are inside
|
blawesom
left a comment
There was a problem hiding this comment.
Verified the design and the test's ordering assumptions against the source before commenting. Two things worth an answer, one small must-fix nit.
What I checked (all correct):
- Test ordering for the final assertions: by the time the new block runs, the
0x100-global fixture isdlclosed and thepinnedmodule was opened without0x100(soRTLD_LOCAL, invisible toRTLD_DEFAULT); the hostRTLD_GLOBALcopy is the only one the default scope can find, sodlsym(nullptr, "GuestModuleAdd") == hostAddis sound. ENABLE_EXPORTS ONon the test target is required forGuestDefaultScopeOnlyto be visible toRTLD_DEFAULT.MainProgramBase()viadl_iterate_phdr(emptydlpi_name= main program) is the right convention; static-local init is thread-safe.- Host symbols from
libc.so(malloc,dlopen,pthread_create) are correctly rejected byFromGuestModule.
1. (question, design) FromGuestModule accepts any default-visibility symbol in the main program. The relinked executable contains the whole AnyPS5 runtime, so a guest dlsym(NULL, ...) can now resolve a same-named runtime symbol that lives in the exe — the test only proves symbols from libc.so are filtered. Real hardware dlsym(NULL) is also main-scope so this is probably fine, but it's the kind of resolution change that can silently differ from the console, so it would be good to state that this is intended.
2. (parity gap) DefaultScopeSymbol tries the plain name and the NID, but FindSymbol also tries name#guest and nid#guest. If a DT_NEEDED-loaded prx exports only #guest-suffixed symbols, a handle lookup finds them but the default scope cannot. Should the fallback mirror all four forms?
3. (nit, must-fix) Formatting glitch in GuestDynamicLoader.cpp — two Require statements on one line:
Require(dlsym_nid_postfix(nullptr, "pthread_create") == nullptr); Require(dlsym_nid_postfix(nullptr, "GuestDefaultScopeOnly") == reinterpret_cast<void*>(&GuestDefaultScopeOnly));4. (nit) The hardcoded NID "3qcG+yBRS+s" could be Nid::ComputeNid("GuestModuleAdd", "") so it can't drift from the fixture.
5. (forward-compat note) link.h / dl_iterate_phdr doesn't exist on macOS; the #ifndef _WIN32 guard will need narrowing when the macOS port lands (that port ships a dl_iterate_phdr emulation). Fine for now.
6. (scope note for #694) The issue's local fix also added the scriptingGetMem/scriptingFreeMem exports, and the "intentionally omitted?" question is still open — the issue probably shouldn't be considered fully closed by this PR alone.
Overall the approach (filter the real RTLD_DEFAULT instead of registering every libs/*.prx in the modules map) looks right and less invasive than the alternative discussed in #694.
Titles pass handle 0 (RTLD_DEFAULT) for a process-wide lookup, and the loader rejected it: the handle is not in the module map, so every such lookup failed even when a loaded module exports the symbol. Handle 0 and -2 now take the default scope, searching the global modules by plain name, NID and `#guest` form, and then the process-wide scope. The process-wide scope of the real loader also contains glibc and libstdc++, so an unfiltered fallback would answer `malloc`, `dlopen` and `pthread_create` where PS5 reports the symbol as missing: handle 0 is the main program, whose lookup sees the program's exports, not the host C++ runtime. A result from the process scope is therefore accepted only when `dladdr` attributes it to a converted or replacement module (a `.prx` library or the executable), which keeps the fallback for guest symbols and removes the host leak. `guest_dynamic_loader` asserts the three host names are not found; disabling the filter makes that test fail. The scripting heap that an earlier revision of this branch added has been dropped: the title reaches the same boot milestone with the lookup failing honestly, so the export was an inference rather than a requirement, and prosper's bring-up notes report `ESRCH` for that name on hardware.
Co-Authored-By: Claude <noreply@anthropic.com>
5aa7a5b to
0ecd75f
Compare
adriadam10
left a comment
There was a problem hiding this comment.
Checked on Linux merged onto current main: build + ctest 450/450 green; with DynamicLoader.cpp reverted, guest_dynamic_loader fails, so the handle-0 / default-scope assertions pin the change. The .prx fix (ends_with) and the two tests from @hikazey (executable export via ENABLE_EXPORTS, host dlopen with RTLD_GLOBAL) cover the fallback; thanks to @hikazey and @Dextroid17 for finding the dead branch.
Added on top (rebased on main):
- Split two statements that shared one line in
GuestDynamicLoader.cpp(pthread_createassertion /GuestDefaultScopeOnly).
Not verified here: Windows runtime (everything new is under #ifndef _WIN32; covered only by the Windows CI job). Merging once CI is green on the pushed head.
What
Titles pass handle 0 (
RTLD_DEFAULT) for a process-wide lookup, and the loader rejected it: the handle is not in the module map, so every such lookup failed even when a loaded module exports the symbol. Handle 0 and -2 now take the default scope - the global modules by plain name, NID and#guestform, then the process-wide scope.The process-wide scope of the real loader also contains glibc and libstdc++, so an unfiltered fallback would answer
malloc,dlopenandpthread_createwhere PS5 reports the symbol as missing. Handle 0 is the main program, whose lookup sees the program's exports and not the host C++ runtime (same model as KytyPS5'sFindProgramById(handle)->export_symbols). A result from the process scope is therefore accepted only whendladdrattributes it to a converted or replacement module - a.prxlibrary or the executable.The scripting heap from the previous revision of this branch is removed; see the reply to the review for the measurement that decided it.
Tested
Arch Linux, GCC,
Release,cmake -DBUILD_TESTING=ON:ctest392/392 passed on this head.guest_dynamic_loadernow asserts both directions of the change. Handle 0 must resolve a guest export (dlsym(0, "GuestModuleAdd")), and the host namesmalloc,dlopenandpthread_createmust stay null. Verified by reverting the feature itself, not a variant of it: withDynamicLoader.cpptaken frommain, the test fails (0% tests passed, 1 tests failed). With this branch's loader it passes.One gap is named rather than hidden.
GuestModuleAddis found by the module loop before the process-scope fallback runs, so no assertion exercisesDefaultScopeSymbolitself yet. That case needs a module that is in the process scope but absent from the loader's module map, which this fixture set does not provide.On a title (PPSA28997, native Linux relink): booting with the scripting heap removed reaches the same milestone as with it - managed bootstrap completes,
level0/level1open, no abort - which is why the export was dropped rather than documented.Checklist
main; no other open PR implements the same functions (checked fix(relinker): suppress duplicate system library lifecycle #1156, feat(relinker): add --module-dir for modules loaded at run time #846, fix(libkernel): pass module args and argp to runtime-loaded modules on Linux #1102 - none touchdlsymscope)One note on freshness: I could not rebase onto the current tip - pushing main's commits that touch
.github/workflows/conventions.ymlto my fork is refused because the OAuth token has noworkflowscope - so this head is based on931109b3. I will rebase as soon as the fork'smainis synced, and re-run the suite after.Refs #694.