Skip to content

fix(libkernel): return ESRCH from join, detach and cancel of a released thread handle - #10817

Merged
oneandonlydean merged 9 commits into
boykopovar:mainfrom
MarcZX10:marc/fix-thread-join-detach-released
Nov 2, 2026
Merged

oneandonlydean merged 9 commits into
boykopovar:mainfrom
MarcZX10:marc/fix-thread-join-detach-released

Conversation

@MarcZX10

@MarcZX10 MarcZX10 commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

What

scePthreadJoin (and the timed join that shares its code), scePthreadDetach and scePthreadCancel dereferenced the Pthread handle before checking that the thread object still exists. A detached thread frees its PthreadPrivate when it ends, so a join, detach or cancel on that handle could read freed memory. The original guest_pthread_released_handle test reproduced a heap-use-after-free in JoinThread_nid_no_patch after the object was freed, and returned EINVAL during a destruction race instead of ESRCH.

Each operation now checks the guest thread registry while the object is protected. Join, detach and cancel retain a temporary reference while they release the registry lock and continue using the object; join releases it before its blocking wait. The short getters and setters keep the registry lock for the duration of their access. Calls on an already released handle return SCE_KERNEL_ERROR_ESRCH, consistent with a second join (#4481), scePthreadAttrGet (#7957) and the name, priority and affinity calls (#10816).

Joined threads that are still retained and detached threads that have not ended behave as before.

Tested

Before this follow-up, the author reported a Linux x86-64 AddressSanitizer + UBSan build (ANYPS5_ENABLE_ASAN=ON with -fsanitize=undefined, RelWithDebInfo), with the host CPU gate bypassed locally for the run only. The author reported guest_pthread_released_handle passing in five runs and the guest_pthread_*, guest_cond_*, guest_rwlock_*, guest_signal*, guest_semaphore and guest_kernel_sys* tests giving the same results as before. The author reported guest_pthread_stack aborting on main under ASan because its 4 KiB frame-distance assertion is exceeded by ASan frames; guest_semaphore_trace was not built. No Windows, ThreadSanitizer or console hardware run was reported.

For earlier follow-up work, local checks included GCC C++20 syntax-only compilation for Thread.cpp and GuestPthreadReleasedHandle.cpp, MinGW C++20 syntax-only compilation for GuestPthreadReleasedHandle.cpp, and git diff --check. The installed MinGW headers do not provide PROCESSOR_RELATIONSHIP::EfficiencyClass or SetThreadDescription, which existing Thread.cpp code uses. Full CMake configuration could not proceed because SDL2 and required host development packages were unavailable. The pthread runtime tests were not run locally for that follow-up.

Rebase validation (2026-10-25)

This PR's four commits are now stacked on #10810 head b79959c5e3; #10810 is stacked on #10816 head 8eaa2bbad5, and that stack is based on upstream main a5a780eaaa7f187fe8f5c7f2ef244f6a6dab2762. The rebase completed without conflicts. I retained the test registrations from all three PRs.

After updating the complete stack to the latest upstream main, strict GCC C++20 syntax-only checks with -Wall -Wextra -Werror passed for Thread.cpp (suppressing its existing missing-field-initializer warning) and GuestPthreadFinalReleaseRace.cpp. git diff --check upstream/main...HEAD and python3 tools/check_conventions.py --base upstream/main --head HEAD also passed. The runtime/ASan test was not rerun, and full local CMake configuration remains unavailable because the SDL2 submodule/development dependencies are missing. No post-rebase runtime result is claimed.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file (none added)
  • No notes, investigation .md files or images added to the repository
  • New third-party code is a submodule built from source (none added)
  • Generic behaviour, not specific to one title
  • Depends on: fix(libkernel): read and write the detached state of a guest thread atomically #10810
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 3986 core/libs/prx/libkernel/Pthread/src/Thread.cpp
PR 4563 core/libs/prx/libkernel/CMakeLists.txt
core/libs/prx/libkernel/Pthread/include/Pthread.hpp
core/libs/prx/libkernel/Pthread/src/Thread.cpp
PR 4630 docs/dev/debt/libkernel.md
PR 5021 core/libs/prx/libkernel/CMakeLists.txt
core/libs/prx/libkernel/Pthread/include/Pthread.hpp
core/libs/prx/libkernel/Pthread/src/Thread.cpp
docs/dev/debt/libkernel.md
PR 6936 core/libs/prx/libkernel/CMakeLists.txt
docs/dev/debt/libkernel.md
PR 7088 docs/dev/debt/libkernel.md
PR 7105 docs/dev/debt/libkernel.md
PR 7121 docs/dev/debt/libkernel.md
PR 7135 docs/dev/debt/libkernel.md
PR 7149 core/libs/prx/libkernel/Pthread/include/Pthread.hpp
core/libs/prx/libkernel/Pthread/src/Thread.cpp
docs/dev/debt/libkernel.md
PR 7192 core/libs/prx/libkernel/CMakeLists.txt
docs/dev/debt/libkernel.md
PR 7447 docs/dev/debt/libkernel.md
PR 7464 docs/dev/debt/libkernel.md
PR 7804 core/libs/prx/libkernel/Pthread/include/Pthread.hpp
core/libs/prx/libkernel/Pthread/src/Thread.cpp
docs/dev/debt/libkernel.md
PR 8592 core/libs/prx/libkernel/Pthread/include/Pthread.hpp
core/libs/prx/libkernel/Pthread/src/Thread.cpp
PR 9019 core/libs/prx/libkernel/CMakeLists.txt
PR 9156 core/libs/prx/libkernel/CMakeLists.txt
docs/dev/debt/libkernel.md
PR 9199 docs/dev/debt/libkernel.md
PR 9293 core/libs/prx/libkernel/CMakeLists.txt
docs/dev/debt/libkernel.md
PR 9297 core/libs/prx/libkernel/CMakeLists.txt
docs/dev/debt/libkernel.md
PR 9302 core/libs/prx/libkernel/CMakeLists.txt
docs/dev/debt/libkernel.md
PR 9383 core/libs/prx/libkernel/CMakeLists.txt
PR 9389 core/libs/prx/libkernel/CMakeLists.txt
docs/dev/debt/libkernel.md
PR 9557 core/libs/prx/libkernel/CMakeLists.txt
docs/dev/debt/libkernel.md
PR 9566 core/libs/prx/libkernel/CMakeLists.txt
docs/dev/debt/libkernel.md
PR 9843 docs/dev/debt/libkernel.md
PR 10025 docs/dev/debt/libkernel.md
PR 10319 docs/dev/debt/libkernel.md
PR 10442 docs/dev/debt/libkernel.md
PR 10558 docs/dev/debt/libkernel.md
PR 11831 core/libs/prx/libkernel/CMakeLists.txt
PR 11939 core/libs/prx/libkernel/CMakeLists.txt
docs/dev/debt/libkernel.md
PR 12544 core/libs/prx/libkernel/CMakeLists.txt
core/libs/prx/libkernel/Pthread/src/Thread.cpp
PR 12560 docs/dev/debt/libkernel.md
PR 13582 core/libs/prx/libkernel/Pthread/src/Thread.cpp
docs/dev/debt/libkernel.md
PR 13587 docs/dev/debt/libkernel.md
PR 14103 docs/dev/debt/libkernel.md
PR 14128 docs/dev/debt/libkernel.md
PR 14212 docs/dev/debt/libkernel.md
PR 14264 core/libs/prx/libkernel/CMakeLists.txt
docs/dev/debt/libkernel.md
PR 14299 core/libs/prx/libkernel/CMakeLists.txt
PR 14497 docs/dev/debt/libkernel.md

@oneandonlydean oneandonlydean self-assigned this Oct 25, 2026
@MarcZX10
MarcZX10 force-pushed the marc/fix-thread-join-detach-released branch 2 times, most recently from 765c97f to b883eae Compare October 25, 2026 05:28
@oneandonlydean

Copy link
Copy Markdown
Collaborator

@MarcZX10 The temporary holder still permits a retain after destruction is committed. ReleaseThread decrements references from 1 to 0 before the destructor removes the registry entry; a concurrent lookup can find that entry and Retain can increment 0 to 1, but the original release still deletes the object. Please coordinate final retirement with reference admission or reject zero-reference objects atomically, and add a controlled test of last release racing successful join/detach/cancel admission. The current observer only calls Getprio, then those operations run after release.

@MarcZX10
MarcZX10 force-pushed the marc/fix-thread-join-detach-released branch from b883eae to 9e1a0a6 Compare October 25, 2026 07:31
@MarcZX10

MarcZX10 commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

Rebase follow-up: head 9e1a0a66e280e94e9311ab166538cf3b49f30a98 now follows rebased #10810 (3b1041f444c5) and #10816 (607f6550ec8c) on upstream main e5f23554c9c7b1929f1ef7a65f6ea33a24fd189e. The #10817 commits retain MarcZX10 as author and committer. On this complete stack, Thread.cpp and all three pthread regression test sources pass GCC 12.2 C++20 syntax checks with -Wall -Wextra -Werror (suppressing the existing missing-field-initializer warning); conventions and git diff --check pass. Runtime/ASan tests were not rerun: local CMake configuration is blocked by the unpopulated SDL2 submodule and missing XRandR development support. Hosted checks are restarting on the updated stack.

@MarcZX10
MarcZX10 force-pushed the marc/fix-thread-join-detach-released branch from 9e1a0a6 to 53ce237 Compare October 25, 2026 07:45
@MarcZX10
MarcZX10 force-pushed the marc/fix-thread-join-detach-released branch 3 times, most recently from a72c2d5 to bfd8a29 Compare October 25, 2026 09:55
@MarcZX10

Copy link
Copy Markdown
Contributor Author

I independently confirmed the remaining last-release race in the exact current head bfd8a2917a8de000078cc77fdecb8bee4ddfa8da. ReleaseThread does references.fetch_sub(1, acq_rel) and immediately deletes when it observes 1; GuestThreadReference::Retain does an unconditional fetch_add(relaxed) while LockRegisteredGuestThread only holds CpuClocks().lock. The destructor removes the registry entry only after that decrement reaches zero. Thus an admission can still find the entry and increment the count after the releasing thread has committed to deletion; the increment does not prevent the already-triggered delete. The new test races Getprio against release, but it does not hold a successful join/detach/cancel admission across the final release as the review thread requested. Please synchronize reference admission with final retirement (or reject a zero refcount atomically before touching the object) and add a deterministic test for that interleaving. I am keeping approval pending this lifetime race.

@MarcZX10
MarcZX10 force-pushed the marc/fix-thread-join-detach-released branch from bfd8a29 to 075ed1d Compare October 25, 2026 10:53
@MarcZX10

Copy link
Copy Markdown
Contributor Author

Addressed the final-release race on the rebased head 075ed1dbb8a16e37248f777634cf60637e886cec. GuestThreadReference::Retain now uses a compare/exchange loop and refuses to increment a zero reference count. The registry lock keeps the object alive while admission checks that count: if admission increments first, the last release observes the additional reference; if release reaches zero first, admission rejects it while destruction waits to remove the registry entry.\n\nAdded guest_pthread_final_release_race, which pauses a detached thread immediately after its final decrement to zero and exercises Join, Detach, and Cancel before the registry entry is removed; each must return ESRCH. The hook is one-shot and limited to the selected test thread.\n\nValidation on this head: strict C++20 syntax checks (-Wall -Wextra -Werror, with the existing missing-field-initializer warning suppressed for Thread.cpp), conventions, and git diff --check pass. I could not run the new runtime test locally because this checkout has no 3rdparty/SDL2 source for CMake configuration; hosted product checks are restarting.

@MarcZX10
MarcZX10 force-pushed the marc/fix-thread-join-detach-released branch from 075ed1d to 9e05be2 Compare October 25, 2026 11:02
@MarcZX10

Copy link
Copy Markdown
Contributor Author

Rebased once more because upstream advanced to 66e6772ca2094b665488d6ca5cbae3f6fb0c5034. Current head is 9e05be2ddefe70ee8331e797d01b5bd72ce961dd. The strict C++20 syntax checks for Thread.cpp and GuestPthreadFinalReleaseRace.cpp, conventions, and git diff --check pass on this head. The new runtime test remains unrun locally because CMake cannot configure without the missing SDL2 submodule source; hosted checks are restarting.

@MarcZX10
MarcZX10 force-pushed the marc/fix-thread-join-detach-released branch from 9e05be2 to 8f275c4 Compare October 25, 2026 11:09
@MarcZX10

Copy link
Copy Markdown
Contributor Author

Rebased again onto current upstream main 16ff208dc32cf3927ba1bae492eee363a5ae7ae8; current head is 8f275c46738daefddda4c1dda534d30b4eed9886. Strict C++20 syntax checks for Thread.cpp and GuestPthreadFinalReleaseRace.cpp, conventions, and git diff --check pass on this head. The deterministic final-release runtime test remains unrun locally because CMake configuration is blocked by the missing SDL2 submodule source; no runtime result is claimed.

@MarcZX10
MarcZX10 force-pushed the marc/fix-thread-join-detach-released branch 3 times, most recently from 4c5f290 to a5462f9 Compare October 25, 2026 11:47
@github-actions github-actions Bot added docs needs rebase Conflicts with main, rebase needed labels Oct 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants