Repository navigation
fix(libkernel): answer a second scePthreadJoin with ESRCH - #4481
1 commit merged into
Conversation
Dextroid17
left a comment
There was a problem hiding this comment.
The control crashes, and the backtrace localises it — a second join dereferences the released object rather than merely failing. Head dc30882e1, parent 33ae282e4, in a worktree:
head guest_pthread_second_join → 1/1 passed (ctest exit 0); direct binary 5/5 runs exit 0
control parent's Pthread/src/Thread.cpp, this PR's test kept → SIGSEGV 4/4 runs, exit 139 (core dumped)
gdb: #0 libc · #1 scePthreadJoin () from libs/unpatched/libkernel.prx · #2 main
That turns the debt entry from a description into a measured crash, and it is the strongest single line available to the body. Isolation check: the neighbouring thread targets are clean at the head — ctest -R 'guest_(pthread|thread|cond|sem|semaphore|posix_sem|libc_threads|ult|ampr_threads)' is 100% tests passed out of 42.
Smaller points:
- The console record predates this:
docs/dev/TechnicalDebt.md:487comes from #4160 (0b7e4ddf9) and says a second join returnsESRCH(0x80020003) on a PS5 (firmware 13.60). The clause this PR replaces — "here a joined thread is released, so a second join reads freed memory" — is the parent revision's line 487, so the debt change reads best as a rewrite of that one sentence, which is what it does. - On the
EINVAL: the accurate version is the hedge the debt line already uses. FreeBSD'spthread_detachreturns it for a thread that is not joinable —THR_FLAGS_DETACHEDset orpthread->joiner != NULL(lib/libthr/thread/thr_detach.c:55-59) — and a completedpthread_joinitself setsTHR_FLAGS_DETACHEDand collects the thread (thr_join.c:141-143), whilepthread->joinercovers only an in-flight joiner (a concurrent second joiner getsENOTSUP,thr_join.c:96-98). Reading the citation as "FreeBSD's answer for a joined thread" puts the weight on the clause that does not apply to a finished join. - "Exactly one joiner wins" reads as designed:
_joinedis astd::atomic<bool>, probed withload(acquire)atThread.cpp:653and taken withexchange(true, acq_rel)at:660, both loser paths returningSCE_KERNEL_ERROR_ESRCHbefore the retval store at:664/:668— and the test pins that (GuestPthreadSecondJoin.cpp:41-42, again at:45and:62). - On the retained object: I read
joinedThreadsas keyed by the address of the guest handle slot (_handleStorage, withusing Pthread = PthreadPrivate*), released only byRecycleJoinedThreadcalled fromscePthreadCreate— which matches what you and the debt line describe, so the open item is the third residual rather than the first. The body says all three residuals are recorded in TechnicalDebt, but only two are: on this head,grep -in "second storage\|handle value copied" docs/dev/TechnicalDebt.mdfinds nothing for "a handle value copied to a second storage and joined there after a recycle". It has a consequence worth recording: joining a copied handle retains the object under the original storage key (Thread.cpp:423), so a laterscePthreadCreateover that original storage callsReleaseThreadon an object the copy still points at — the same freed-memory read this PR removes, reachable through the copy. Either a debt line or aRequireinGuestPthreadSecondJoin.cppwould close it.
Nit: core/libs/tests/GuestPthreadSecondJoin.cpp has no trailing newline (\ No newline at end of file); tools/check_conventions.py does not flag it, but it stands out in a new file.
Written with AI assistance.
Hermes Agent 🤖 at your service
dc30882 to
57105eb
Compare
|
Thank you for the control run — quoting the measured crash into the PR body now. Follow-ups pushed (head
AI-assisted: yes. |
|
All three follow-ups are in the head. Read at
Nothing else from me. The measured crash is the line I would keep at the top of the body — it is reproduceable from the parent's Written with AI assistance. |
57105eb to
a2dfbfd
Compare
|
Tested change: 20e5499ac. Full Release Windows build (MinGW GCC 15.2) exits 0 and 11/11 thread/TLS tests pass, including repeated native/adopted cancellation, event cleanup and handle-count checks. No game or console run. I have a tested Windows native-join cancellation fix on main c952c2b. It waits on the joinee's native handle and the joiner's own persistent cancellation event, preserving the return-value output and allowing the joinee to be joined later. The regression holds a native FLS destructor after guest _finished, requests cancellation only once the native wait is reached, and passes with the fix; the original code completes normally and overwrites the output. This overlaps the _joined.exchange(true) before the native wait in this PR: cancellation in that phase would leave the joinee permanently claimed and a retry would return ESRCH. The lifecycle needs a temporary join claim that rolls back on cancellation, or the final claim after the cancellable wait with appropriate concurrency handling. #4533 inherits this lifecycle. Could we agree to land the cancellation correction first, then preserve its cancellation/retry regression while adapting this lifecycle work? Alternatively, please propose a sequence that keeps the canceled joinee joinable. I am holding publication until the order is agreed, following CONTRIBUTING's overlap rule. AI-assisted: yes. |
|
Thank you for checking the interaction. I read your 20e5499 change and agree that our _joined claim before the native wait needs to account for cancellation. Let's land your cancellation correction first. Once your PR is published, I will declare it as a dependency of #4481 and adapt the join lifecycle while preserving your canceled-join/retry regression. #4533 will then be restacked on the updated #4481. I have not tested the combined changes yet; the combined lifecycle will need Windows and Linux tests and an independent review before pushing. AI-assisted: yes. |
|
Thanks for confirming the order. We are still waiting for the author's response on #4563, which also overlaps the native join lifecycle. Once the ordering is agreed there as well, we will publish our cancellation PR. AI-assisted: yes. |
|
Hi @Roojool — I independently investigated the pthread handle-lifetime issue before seeing #4481, and I won't open a competing PR. I found two additional cases that may be useful when adapting the join lifecycle after Ondra97's change:
I have Windows-only regression tests and concurrency probes covering these scenarios, as well as concurrent joins and cancellation during Join. Happy to share the relevant tests if helpful. No PS5 or Linux validation was performed. AI-assisted: yes. |
|
Thank you for flagging both cases and for avoiding a competing PR. Please share the regression tests and concurrency probes, the exact base commit, and the Windows build/run commands with results or logs. The detach-during-join and cancellation/retry cases would be useful when adapting the lifecycle. We have agreed to land Ondra97's cancellation correction first; publication is currently waiting for the overlapping #4563 ordering discussion. We have not validated your additional cases ourselves, and the PS5 detach-during-join behavior remains unverified. Please include the raise-during-exit probe too so that it can be assessed separately. AI-assisted: yes. |
Thanks @Roojool! I’ll prepare the regression tests, concurrency probes, exact base SHA, and Windows build/run logs. Would you prefer that I open a small PR against your branch with the tests, or share them as a standalone patch/gist? I’m happy to contribute the tests directly if that makes integration easier, especially given the coordination with Ondra97 and #4563. AI-assisted: yes. |
|
A patch or gist works best for now: this branch will be reworked once #4901's cancellation correction lands (ordering agreed upstream), so tests targeting today's head would need adaptation anyway. Share them standalone with your base SHA and run logs, and we will integrate them with your authorship preserved when the lifecycle update goes in. If a PR is easier for you, one against this branch is also fine — whichever you prefer. |
|
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.
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.
|
72a848c
What
A second
scePthreadJoinon the same thread now returnsESRCH(0x80020003) instead of reading freed memory.On a PS5 (firmware 13.60)
scePthreadJoinanswersESRCHfor a thread that was already joined — measured by #4160's author and recorded in its TechnicalDebt entry: "here a joined thread is released, so a second join reads freed memory". #4160 deliberately left that follow-up: keeping the thread for the second answer needs the thread lifetime to change. This is that change:ESRCH.scePthreadCreatepublishes over the same guest handle storage, so a create/join loop over one storage holds one object;libSceJobManagerandlibSceUltjoin patterns are covered.scePthreadDetachon a joined thread answersEINVAL, as FreeBSD does on a thread that is not joinable; the console answer for a detach after a join is unverified. It is required so the retained object is not released twice. Recorded in TechnicalDebt.PosixThread::ToErrnoalready maps0x80020003to3, FreeBSD'sESRCHfor a deadpthread_t; the test pins it.EDEADLK, join-after-detach/detach-twiceEINVAL, the zeroed attribute — all already correct since fix(libkernel): join the calling thread and create from an unset attribute as the console does #4160.Tested
guest_pthread_second_join: create -> join 0 (retval read) -> second join0x80020003with*retvaluntouched -> detach0x80020016-> posixpthread_join3 -> 64 create/join/second-join rounds over the same storage (recycle path) -> join-after-detach and detach-twiceEINVAL.main(9aa5fd2): the test fails 3/3 on Windows (abort0xc0000409, freednativeHandle) and 2/2 on WSL Linux (SIGSEGV) — the use-after-free.Thread.cppwith this PR's test) crashes 4/4 runs — SIGSEGV, exit 139, backtrace intoscePthreadJoin— and the head passes 5/5; the 42 neighbouring thread tests pass at the head.guest_pthread_join_self, pthread/thread/cond/sem/ult/ampr/libc/signals/cpumask suites); WSL2 Linux (g++ 15.2.0, Release, -j4) 17/17 including the new test.python tools/check_conventions.py --base 9aa5fd290passes.Checklist
main; no other open PR implements the same functions.mdfiles or images added to the repositoryConsole semantics are #4160's author's measurements on PS5 firmware 13.60; no console was run for this change. Residuals (a guest that joins into a fresh storage location every time retains those objects; joining a detached handle after it ended; a handle value copied to a second storage and joined there after a recycle) are recorded in TechnicalDebt.