Repository navigation
fix(ajm, apr): keep AJM and APR state alive through static teardown - #1926
Conversation
klosixgaming-arch
left a comment
There was a problem hiding this comment.
I can approve this.
- The
*new Tidiom to make these statics immortal is the established pattern here (you cite #1197 for NGS2), so it's not a novel hack — it's the project's chosen remedy for "title thread reads freed statics duringstd::exit". Consistent. - Callers are unchanged because the namespace-scope names become references to heap objects — correct, and the smallest possible surface for the change.
- The test is excellent: it
dlopens the library after registering theatexithandler so the library's statics are destroyed before the handler runs, then exercises AJM from the handler.mainaborts 3/3 (double free in tcache 2), this branch passes. That's a genuine regression test for the exact failure class, not a symbol check. - You correctly scope it to AJM+APR and explicitly note the other
libkernelnamespace statics (Equeue.cpp,SyncOnAddress.cpp,DirectMemory.cpp) that weren't hit — honest boundary, not silent.
Non-blocking note: the deliberate leak is the whole point, but a reader glancing at std::mutex& g_lock = *new std::mutex(); might not know it's intentional project policy. A one-line debt/CONVENTIONS pointer (or a shared helper Immortal<T>) would stop a future reviewer from "fixing" it back to a normal static. Worth it given it's now in at least three files.
Written with AI assistance.
|
Windows build check, since the description says Windows wasn't built. Setup: Windows 10 (19045), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release. Merged onto
🤖 Addressed by Claude Code |
|
@klosixgaming-arch thanks. I added a TechnicalDebt entry for the leaked statics (NGS2, AJM, APR) in f79d69c, since code comments aren't allowed here. A shared helper would also change NGS2, so I'd leave it for another PR. @popcoder565 thanks for the Windows build. The teardown test is Unix-only on purpose. It depends on the ELF order in which a AI-assisted: yes |
f79d69c to
b0011a1
Compare
b0011a1 to
5eb2a13
Compare
AJM's instance and batch tables and APR's file, directory and AMM state were namespace-scope statics, destroyed by std::exit while the title's threads keep calling the libraries. A decoder thread then read a freed instance (an unknown codec), sceAjmBatchWait no longer found its batch, and APR resolved file ids through a freed vector. The state is now allocated once and never destroyed.
5eb2a13 to
78388c2
Compare
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
|
…order fix(ajm, apr): keep AJM and APR state alive through static teardown
What
A window close ends in
std::exiton a helper thread (LibcRequestExit_nid_postfix), andstd::exitdestroys the host libraries' statics while the title's threads are still running. AJM and APR keep their state in namespace-scope statics, so a title thread that calls them during exit reads freed memory:libSceAjm.native:g_instancesandg_batches(and their mutexes). A job on a freedInstancedecodes a garbage codec,sceAjmBatchWaitreturnsSCE_AJM_ERROR_INVALID_BATCHfor a batch it just started, andsceAjmInstanceDestroyfrees an instance twice.libkernelAPR:g_files,g_idsByPath,g_directories,g_counterWritesand the AMM state from_amm(). A read resolves its file id through the freed vector.All of these are now allocated once and never destroyed, the same approach #1197 took for NGS2. Callers don't change: the namespace-scope names are now references to the heap objects.
Closes #1925
Tested
Nobara 44 x86-64 (Linux 7.2), GCC 16.2, RTX 5070 Ti, driver 595.91.07. Base:
mainf145139.guest_ajm_teardown(UNIX only, it needsdlopen). It registers anatexithandler, thendlopenslibSceAjm.nativeand creates an instance. Because the library is loaded after the handler is registered, its statics are destroyed before the handler runs. The handler then runs a batch on the instance, waits for the batch and destroys the instance. Withmain'sAjm.cppit aborts 3 of 3 times (free(): double free detected in tcache 2). With this change it passes.ctest -j1 --timeout 60: 494 of 495 pass. The one failure,guest_dynamic_loader, also fails here without this change.tools/check_conventions.pyandtools/check_test_registration.pypass.mainfrom 10-07 with fix(agc): release the current address space with the image mirrors #1071, perf(agc): pipelined draws on by default #1202, feat(recompiler): bindless image tables up to the device's sampled-image limit #1184 and local performance changes) with this commit on top. Before: 15 of 15 closes crashed (SIGABRT onAJM: decoding codec 4026830656 is not implemented,APR: cannot open <garbage>ordouble free or corruption, or SIGSEGV afterNCA DecoderManager: sceAjmBatchWait() failed with error code 0x80930004), and 3 of 3 with fix(libc): run guest exit handlers before the host exit handlers #1658. After: 5 of 5 closes exit 0 with no core. One more run aborted at boot, before any close, on an X RandR error (BadRROutput) andVideoOut: presentation thread cannot stop itself. That is unrelated to this change.Other libkernel state lives in namespace-scope statics too (
Equeue.cpp,SyncOnAddress.cpp,DirectMemory.cpp). Demon's Souls didn't hit those in these runs, so they are left as they are.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository