From 3ba4532352952ef4b3e804fb32f984fb36a19e41 Mon Sep 17 00:00:00 2001 From: milescoviello Date: Sun, 11 Oct 2026 09:41:49 -0700 Subject: [PATCH 1/2] fix(libkernel): reset a once control when its initializer exits or is cancelled pthread_once and scePthreadOnce marked the control as running, called the initializer and marked the control done only after it returned. A thread that left the initializer through pthread_exit or a cancellation point never came back, so the control stayed in progress and every later call on it waited forever. FreeBSD libthr runs the initializer under a cleanup handler (lib/libthr/thread/thr_once.c, _pthread_once: "THR_CLEANUP_PUSH(curthread, once_cancel_handler, once_control); init_routine(); THR_CLEANUP_POP(curthread, 0);"). once_cancel_handler stores ONCE_NEVER_DONE and wakes the waiters, and thread exit and cancellation run the cleanup handlers. POSIX requires this for cancellation: "if init_routine is a cancellation point and is canceled, the effect on once_control shall be as if pthread_once() was never called." Both functions now push such a handler around the initializer, so the control returns to its initial state and the next caller, or one already waiting, runs the initializer. scePthreadOnce waiters now retry when the control goes back to its initial state, and an exception from its initializer resets the control as it already did for pthread_once. CleanupHandler moves to Cancel.hpp so that Once.cpp can use it. Co-Authored-By: Claude Opus 5.5 --- .../libs/prx/libkernel/Pthread/Posix/Once.cpp | 17 +++++-- .../prx/libkernel/Pthread/include/Cancel.hpp | 7 +++ .../libs/prx/libkernel/Pthread/src/Thread.cpp | 34 ++++++++------ core/libs/tests/GuestOnce.cpp | 46 ++++++++++++++++++- 4 files changed, 85 insertions(+), 19 deletions(-) diff --git a/core/libs/prx/libkernel/Pthread/Posix/Once.cpp b/core/libs/prx/libkernel/Pthread/Posix/Once.cpp index 32ed2ee79..dc09e2cb4 100644 --- a/core/libs/prx/libkernel/Pthread/Posix/Once.cpp +++ b/core/libs/prx/libkernel/Pthread/Posix/Once.cpp @@ -1,5 +1,6 @@ #include "prx/libc/include/General.hpp" #include "prx/libc/include/general/VabiMacros.hpp" +#include "prx/libkernel/Pthread/include/Cancel.hpp" #include #include #include @@ -14,6 +15,14 @@ struct GuestOnce { static_assert(offsetof(GuestOnce, mutex) == 8 && sizeof(GuestOnce) == 16); std::mutex onceMutex; std::condition_variable onceChanged; + +void APS5_VABI ResetOnce(void* control) { + { + std::lock_guard lock(onceMutex); + static_cast(control)->state = 0; + } + onceChanged.notify_all(); +} } extern "C" int APS5_VABI pthread_once_nid_postfix( @@ -27,15 +36,15 @@ extern "C" int APS5_VABI pthread_once_nid_postfix( lock.unlock(); // Initializers may initialize other once controls. Never run guest code // under the shared lock. The lock publishes all initializer writes. + CleanupHandler reset{}; + __pthread_cleanup_push_imp_nid_postfix(ResetOnce, control, &reset); try { initialize(); } catch (...) { - lock.lock(); - control->state = 0; - lock.unlock(); - onceChanged.notify_all(); + __pthread_cleanup_pop_imp_nid_postfix(1); throw; } + __pthread_cleanup_pop_imp_nid_postfix(0); lock.lock(); control->state = 1; lock.unlock(); diff --git a/core/libs/prx/libkernel/Pthread/include/Cancel.hpp b/core/libs/prx/libkernel/Pthread/include/Cancel.hpp index 26a7c7176..b85a2a375 100644 --- a/core/libs/prx/libkernel/Pthread/include/Cancel.hpp +++ b/core/libs/prx/libkernel/Pthread/include/Cancel.hpp @@ -12,6 +12,13 @@ inline constexpr int GUEST_CANCEL_DEFERRED = 0; inline constexpr int GUEST_CANCEL_ASYNCHRONOUS = 2; inline void* const GUEST_CANCELED = reinterpret_cast(std::uintptr_t{1}); +struct CleanupHandler { + CleanupHandler* previous; + void (APS5_VABI* routine)(void*); + void* argument; + int onHeap; +}; + extern "C" { void APS5_VABI __pthread_cleanup_push_imp_nid_postfix(void (APS5_VABI* routine)(void*), void* argument, void* info); void APS5_VABI __pthread_cleanup_pop_imp_nid_postfix(int execute); diff --git a/core/libs/prx/libkernel/Pthread/src/Thread.cpp b/core/libs/prx/libkernel/Pthread/src/Thread.cpp index 11400bdd2..cc471f5f5 100644 --- a/core/libs/prx/libkernel/Pthread/src/Thread.cpp +++ b/core/libs/prx/libkernel/Pthread/src/Thread.cpp @@ -53,12 +53,6 @@ struct ThreadArgs { static std::atomic threadDtors{nullptr}; static std::atomic threadAtexitCount{nullptr}; static std::atomic threadAtexitReport{nullptr}; -struct CleanupHandler { - CleanupHandler* previous; - void (APS5_VABI* routine)(void*); - void* argument; - int onHeap; -}; struct GuestThreadState { PthreadPrivate* current = nullptr; @@ -802,20 +796,34 @@ int APS5_VABI scePthreadSetprio(Pthread thread, int prio) { return SCE_OK; } +static void APS5_VABI ResetOnce(void* once) { + constexpr int32_t Never = 0; + std::atomic_ref state(*static_cast(once)); + state.store(Never, std::memory_order_release); + state.notify_all(); +} + int APS5_VABI scePthreadOnce(int32_t* once, void (APS5_VABI* init)(void)) { if (!once || !init) return SCE_KERNEL_ERROR_EINVAL; constexpr int32_t Never = 0; constexpr int32_t Done = 1; constexpr int32_t Running = 2; std::atomic_ref state(*once); - int32_t expected = Never; - if (state.compare_exchange_strong(expected, Running, std::memory_order_acq_rel)) { - init(); - state.store(Done, std::memory_order_release); - state.notify_all(); - return SCE_OK; + for (int32_t expected = Never; !state.compare_exchange_strong(expected, Running, std::memory_order_acq_rel); expected = Never) { + if (expected != Running) return SCE_OK; + state.wait(Running, std::memory_order_acquire); } - while ((expected = state.load(std::memory_order_acquire)) == Running) state.wait(Running, std::memory_order_acquire); + CleanupHandler reset{}; + __pthread_cleanup_push_imp_nid_postfix(ResetOnce, once, &reset); + try { + init(); + } catch (...) { + __pthread_cleanup_pop_imp_nid_postfix(1); + throw; + } + __pthread_cleanup_pop_imp_nid_postfix(0); + state.store(Done, std::memory_order_release); + state.notify_all(); return SCE_OK; } diff --git a/core/libs/tests/GuestOnce.cpp b/core/libs/tests/GuestOnce.cpp index b82ec22ef..ea35859ab 100644 --- a/core/libs/tests/GuestOnce.cpp +++ b/core/libs/tests/GuestOnce.cpp @@ -1,4 +1,4 @@ -#include "prx/libc/include/general/VabiMacros.hpp" +#include "SceTypes.hpp" #include #include #include @@ -9,7 +9,16 @@ struct GuestOnce { std::int32_t state; void* mutex; }; static_assert(sizeof(GuestOnce) == 16 && offsetof(GuestOnce, mutex) == 8); using Initializer = void (APS5_VABI *)(); -extern "C" int APS5_VABI pthread_once_nid_postfix(GuestOnce*, Initializer); +extern "C" { +int APS5_VABI pthread_once_nid_postfix(GuestOnce*, Initializer); +int APS5_VABI scePthreadOnce(std::int32_t* once, Initializer init); +int APS5_VABI scePthreadCreate(Pthread* thread, const PthreadAttr* attr, PthreadEntry entry, void* arg, const char* name); +int APS5_VABI scePthreadJoin(Pthread thread, void** retval); +void APS5_VABI scePthreadExit(void* retval); +int APS5_VABI scePthreadCancel(Pthread thread); +void APS5_VABI scePthreadTestcancel(); +Pthread APS5_VABI scePthreadSelf(); +} static void Require(bool value) { if (!value) std::abort(); } static GuestOnce control{}, nested{}, retry{}; static std::atomic calls{0}, arrived{0}; @@ -24,6 +33,27 @@ static void APS5_VABI Initialize() { static void APS5_VABI Retry() { if (++attempts == 1) throw 7; } +static GuestOnce exited{}, cancelled{}; +static std::int32_t sceExited = 0, sceCancelled = 0; +static int retried = 0; +static void APS5_VABI ExitInsideInitializer() { scePthreadExit(nullptr); } +static void APS5_VABI CancelInsideInitializer() { + Require(scePthreadCancel(scePthreadSelf()) == 0); + scePthreadTestcancel(); +} +static void APS5_VABI CountRetry() { ++retried; } +struct OnceCall { GuestOnce* control; std::int32_t* sceControl; Initializer initialize; }; +static void* APS5_VABI CallOnce(void* argument) { + const auto& call = *static_cast(argument); + if (call.control) pthread_once_nid_postfix(call.control, call.initialize); + else scePthreadOnce(call.sceControl, call.initialize); + return nullptr; +} +static void LeaveInsideInitializer(OnceCall call) { + Pthread thread{}; + Require(scePthreadCreate(&thread, nullptr, CallOnce, &call, nullptr) == 0); + Require(scePthreadJoin(thread, nullptr) == 0); +} int main() { std::array workers; for (auto& worker : workers) worker = std::thread([] { @@ -41,4 +71,16 @@ int main() { Require(pthread_once_nid_postfix(&control, nullptr) == 22); GuestOnce invalid{9, nullptr}; Require(pthread_once_nid_postfix(&invalid, Initialize) == 22); + LeaveInsideInitializer({&exited, nullptr, ExitInsideInitializer}); + Require(exited.state == 0); + Require(pthread_once_nid_postfix(&exited, CountRetry) == 0 && exited.state == 1 && retried == 1); + LeaveInsideInitializer({&cancelled, nullptr, CancelInsideInitializer}); + Require(cancelled.state == 0); + Require(pthread_once_nid_postfix(&cancelled, CountRetry) == 0 && cancelled.state == 1 && retried == 2); + LeaveInsideInitializer({nullptr, &sceExited, ExitInsideInitializer}); + Require(sceExited == 0); + Require(scePthreadOnce(&sceExited, CountRetry) == 0 && sceExited == 1 && retried == 3); + LeaveInsideInitializer({nullptr, &sceCancelled, CancelInsideInitializer}); + Require(sceCancelled == 0); + Require(scePthreadOnce(&sceCancelled, CountRetry) == 0 && sceCancelled == 1 && retried == 4); } From 083c98a0672ed34b06766ee853c0428ef68d85ff Mon Sep 17 00:00:00 2001 From: milescoviello Date: Mon, 12 Oct 2026 07:49:09 +0000 Subject: [PATCH 2/2] fix(libkernel): leave a once initializer's own cleanup handlers alone when it throws pthread_once and scePthreadOnce removed their reset handler with a pop that runs whatever handler is on top of the cleanup stack. An initializer that pushed a handler of its own and then threw left that handler on top, so the pop ran it with dangling state. Both now record the stack top before pushing the reset handler; on a throw they restore the stack to it and reset the control directly, and a normal return restores it as well. Reported in review by Hussein Kadhim, who fixed it the same way. Co-authored-by: Hussein Kadhim Co-Authored-By: Claude Opus 5.5 --- core/libs/prx/libkernel/Pthread/Posix/Once.cpp | 6 ++++-- .../libs/prx/libkernel/Pthread/include/Cancel.hpp | 2 ++ core/libs/prx/libkernel/Pthread/src/Thread.cpp | 14 ++++++++++++-- core/libs/tests/GuestOnce.cpp | 15 +++++++++++++++ 4 files changed, 33 insertions(+), 4 deletions(-) diff --git a/core/libs/prx/libkernel/Pthread/Posix/Once.cpp b/core/libs/prx/libkernel/Pthread/Posix/Once.cpp index dc09e2cb4..6919e008b 100644 --- a/core/libs/prx/libkernel/Pthread/Posix/Once.cpp +++ b/core/libs/prx/libkernel/Pthread/Posix/Once.cpp @@ -37,14 +37,16 @@ extern "C" int APS5_VABI pthread_once_nid_postfix( // Initializers may initialize other once controls. Never run guest code // under the shared lock. The lock publishes all initializer writes. CleanupHandler reset{}; + void* const mark = ThreadCancel::CleanupMark(); __pthread_cleanup_push_imp_nid_postfix(ResetOnce, control, &reset); try { initialize(); } catch (...) { - __pthread_cleanup_pop_imp_nid_postfix(1); + ThreadCancel::CleanupRestore(mark); + ResetOnce(control); throw; } - __pthread_cleanup_pop_imp_nid_postfix(0); + ThreadCancel::CleanupRestore(mark); lock.lock(); control->state = 1; lock.unlock(); diff --git a/core/libs/prx/libkernel/Pthread/include/Cancel.hpp b/core/libs/prx/libkernel/Pthread/include/Cancel.hpp index b85a2a375..6a3dad661 100644 --- a/core/libs/prx/libkernel/Pthread/include/Cancel.hpp +++ b/core/libs/prx/libkernel/Pthread/include/Cancel.hpp @@ -28,6 +28,8 @@ namespace ThreadCancel { bool Requested(); void Check(); +void* CleanupMark(); +void CleanupRestore(void* mark); void BeginWait(TimedWait::Condition* condition); void EndWait(); diff --git a/core/libs/prx/libkernel/Pthread/src/Thread.cpp b/core/libs/prx/libkernel/Pthread/src/Thread.cpp index cc471f5f5..72cc0ff3e 100644 --- a/core/libs/prx/libkernel/Pthread/src/Thread.cpp +++ b/core/libs/prx/libkernel/Pthread/src/Thread.cpp @@ -353,6 +353,14 @@ void ThreadCancel::Check() { if (Requested()) scePthreadExit(GUEST_CANCELED); } +void* ThreadCancel::CleanupMark() { + return CurrentGuestState().cleanupHandlers; +} + +void ThreadCancel::CleanupRestore(void* mark) { + CurrentGuestState().cleanupHandlers = static_cast(mark); +} + void ThreadCancel::BeginWait(TimedWait::Condition* condition) { if (!CurrentGuestState().current) return; std::lock_guard lock(CurrentGuestState().current->cancelLock); @@ -814,14 +822,16 @@ int APS5_VABI scePthreadOnce(int32_t* once, void (APS5_VABI* init)(void)) { state.wait(Running, std::memory_order_acquire); } CleanupHandler reset{}; + void* const mark = ThreadCancel::CleanupMark(); __pthread_cleanup_push_imp_nid_postfix(ResetOnce, once, &reset); try { init(); } catch (...) { - __pthread_cleanup_pop_imp_nid_postfix(1); + ThreadCancel::CleanupRestore(mark); + ResetOnce(once); throw; } - __pthread_cleanup_pop_imp_nid_postfix(0); + ThreadCancel::CleanupRestore(mark); state.store(Done, std::memory_order_release); state.notify_all(); return SCE_OK; diff --git a/core/libs/tests/GuestOnce.cpp b/core/libs/tests/GuestOnce.cpp index ea35859ab..3e60f58ec 100644 --- a/core/libs/tests/GuestOnce.cpp +++ b/core/libs/tests/GuestOnce.cpp @@ -18,6 +18,7 @@ void APS5_VABI scePthreadExit(void* retval); int APS5_VABI scePthreadCancel(Pthread thread); void APS5_VABI scePthreadTestcancel(); Pthread APS5_VABI scePthreadSelf(); +void APS5_VABI __pthread_cleanup_push_imp_nid_postfix(void (APS5_VABI* routine)(void*), void* argument, void* info); } static void Require(bool value) { if (!value) std::abort(); } static GuestOnce control{}, nested{}, retry{}; @@ -42,6 +43,14 @@ static void APS5_VABI CancelInsideInitializer() { scePthreadTestcancel(); } static void APS5_VABI CountRetry() { ++retried; } +static void APS5_VABI AbortCleanup(void*) { std::abort(); } +static void APS5_VABI PushAndThrow() { + std::uintptr_t info[8]; + __pthread_cleanup_push_imp_nid_postfix(AbortCleanup, nullptr, info); + throw 7; +} +static GuestOnce pushed{}; +static std::int32_t scePushed = 0; struct OnceCall { GuestOnce* control; std::int32_t* sceControl; Initializer initialize; }; static void* APS5_VABI CallOnce(void* argument) { const auto& call = *static_cast(argument); @@ -83,4 +92,10 @@ int main() { LeaveInsideInitializer({nullptr, &sceCancelled, CancelInsideInitializer}); Require(sceCancelled == 0); Require(scePthreadOnce(&sceCancelled, CountRetry) == 0 && sceCancelled == 1 && retried == 4); + try { pthread_once_nid_postfix(&pushed, PushAndThrow); std::abort(); } + catch (int value) { Require(value == 7 && pushed.state == 0); } + Require(pthread_once_nid_postfix(&pushed, CountRetry) == 0 && pushed.state == 1 && retried == 5); + try { scePthreadOnce(&scePushed, PushAndThrow); std::abort(); } + catch (int value) { Require(value == 7 && scePushed == 0); } + Require(scePthreadOnce(&scePushed, CountRetry) == 0 && scePushed == 1 && retried == 6); }