Merge pull request #3011 from MarcZX100/codex/prbatch-2988-cond-wait

fix(libkernel): return errors for waits with an unowned mutex

core/libs/prx/libkernel/CMakeLists.txt

@@ -343,6 +343,16 @@ if(BUILD_TESTING)
         set_tests_properties(guest_cond_recursive_mutex PROPERTIES ENVIRONMENT_MODIFICATION
             "PATH=path_list_prepend:$<TARGET_FILE_DIR:libkernel>")
     endif()
+    add_test_executable(guest_cond_unowned_mutex_tests ${LIBS_INCLUDE_DIR}/tests/GuestCondUnownedMutex.cpp)
+    target_include_directories(guest_cond_unowned_mutex_tests PRIVATE ${LIBS_INCLUDE_DIR})
+    target_link_libraries(guest_cond_unowned_mutex_tests PRIVATE libkernel libc)
+    configure_windows_unwind(guest_cond_unowned_mutex_tests)
+    add_test(NAME guest_cond_unowned_mutex COMMAND guest_cond_unowned_mutex_tests)
+    set_tests_properties(guest_cond_unowned_mutex PROPERTIES TIMEOUT 20)
+    if(WIN32)
+        set_tests_properties(guest_cond_unowned_mutex PROPERTIES ENVIRONMENT_MODIFICATION
+            "PATH=path_list_prepend:$<TARGET_FILE_DIR:libkernel>")
+    endif()
     add_test_executable(guest_cpu_time_clocks_tests ${LIBS_INCLUDE_DIR}/tests/GuestCpuTimeClocks.cpp)
     target_include_directories(guest_cpu_time_clocks_tests PRIVATE ${LIBS_INCLUDE_DIR})
     target_link_libraries(guest_cpu_time_clocks_tests PRIVATE libkernel libc)

core/libs/prx/libkernel/Pthread/src/Cond.cpp

@@ -3,6 +3,7 @@
 #include "prx/libkernel/Pthread/include/Cond.hpp"
 #include "prx/libkernel/Pthread/include/Cancel.hpp"
 #include "prx/libkernel/Pthread/Posix/Common.hpp"
+#include "prx/libkernel/KernelErrors.hpp"
 #include "prx/libkernel/Time/include/Time.hpp"
 #include "prx/libkernel/Time/include/TimedWait.hpp"
 #include <atomic>
@@ -22,6 +23,14 @@ PthreadCond destroyedCond() {
     return reinterpret_cast<PthreadCond>(std::uintptr_t{2});
 }
 
+PthreadMutex destroyedMutex() {
+    return reinterpret_cast<PthreadMutex>(std::uintptr_t{2});
+}
+
+PthreadMutex adaptiveInitializer() {
+    return reinterpret_cast<PthreadMutex>(std::uintptr_t{1});
+}
+
 PthreadCond resolveCond(PthreadCond* cond) {
     if (!cond)
         throw std::invalid_argument("Condition variable pointer is null");
@@ -39,18 +48,23 @@ PthreadCond resolveCond(PthreadCond* cond) {
     return created;
 }
 
-PthreadMutex lockedMutex(PthreadMutex* mutex) {
-    if (!mutex || !*mutex)
+int mutexOwnership(PthreadMutex* mutex, PthreadMutex* owned) {
+    if (!mutex)
         throw std::invalid_argument("Mutex pointer is null");
-    auto* current = *mutex;
-    if (current->_owner.load(std::memory_order_acquire) != std::this_thread::get_id())
-        throw std::runtime_error("Condition wait mutex is not owned by the current thread");
-    return current;
+    const auto current = std::atomic_ref<PthreadMutex>(*mutex).load(std::memory_order_acquire);
+    if (current == destroyedMutex())
+        return SCE_KERNEL_ERROR_EINVAL;
+    if (!current || current == adaptiveInitializer() || current->_owner.load(std::memory_order_acquire) != std::this_thread::get_id())
+        return SCE_KERNEL_ERROR_EPERM;
+    *owned = current;
+    return 0;
 }
 
 int waitUntil(PthreadCond* cond, PthreadMutex* mutex, std::optional<std::uint64_t> deadlineNanos, const void* caller) {
     auto* c = resolveCond(cond);
-    auto* m = lockedMutex(mutex);
+    PthreadMutex m = nullptr;
+    if (const int error = mutexOwnership(mutex, &m))
+        return error;
     bool timedOut = false;
     const auto waitStart = std::chrono::steady_clock::now();
     struct Trace {

core/libs/tests/GuestCondUnownedMutex.cpp

@@ -0,0 +1,106 @@
+#include "SceTypes.hpp"
+#include <atomic>
+#include <cstdint>
+#include <cstdlib>
+#include <thread>
+
+extern "C" {
+int APS5_VABI scePthreadCreate(Pthread* thread, const PthreadAttr* attr, PthreadEntry entry, void* arg, const char* name);
+int APS5_VABI scePthreadJoin(Pthread thread, void** retval);
+int APS5_VABI scePthreadMutexInit(PthreadMutex* mutex, const PthreadMutexattr* attr, const char* name);
+int APS5_VABI scePthreadMutexDestroy(PthreadMutex* mutex);
+int APS5_VABI scePthreadMutexLock(PthreadMutex* mutex);
+int APS5_VABI scePthreadMutexUnlock(PthreadMutex* mutex);
+int APS5_VABI scePthreadCondInit(PthreadCond* cond, const PthreadCondattr* attr, const char* name);
+int APS5_VABI scePthreadCondDestroy(PthreadCond* cond);
+int APS5_VABI scePthreadCondWait(PthreadCond* cond, PthreadMutex* mutex);
+int APS5_VABI scePthreadCondTimedwait(PthreadCond* cond, PthreadMutex* mutex, unsigned int usec);
+int APS5_VABI pthread_cond_wait_nid_postfix(PthreadCond* cond, PthreadMutex* mutex);
+int APS5_VABI pthread_cond_timedwait_nid_postfix(PthreadCond* cond, PthreadMutex* mutex, const KernelTimespec* abstime);
+int APS5_VABI clock_gettime_nid_postfix(int clockId, KernelTimespec* tp);
+}
+
+static constexpr int SCE_OK = 0;
+static constexpr int SCE_KERNEL_ERROR_EPERM = static_cast<int>(0x80020001);
+static constexpr int SCE_KERNEL_ERROR_EINVAL = static_cast<int>(0x80020016);
+static constexpr int POSIX_EPERM = 1;
+static constexpr int POSIX_EINVAL = 22;
+static constexpr std::uintptr_t ADAPTIVE_INITIALIZER = 1;
+
+static void Require(bool value) { if (!value) std::abort(); }
+
+static int PosixTimed(PthreadCond* cond, PthreadMutex* mutex) {
+    KernelTimespec deadline{};
+    Require(clock_gettime_nid_postfix(0, &deadline) == 0);
+    deadline.tv_sec += 60;
+    return pthread_cond_timedwait_nid_postfix(cond, mutex, &deadline);
+}
+
+struct Wait {
+    int (*run)(PthreadCond*, PthreadMutex*);
+    int notOwned;
+    int destroyed;
+};
+
+static int SceWait(PthreadCond* cond, PthreadMutex* mutex) { return scePthreadCondWait(cond, mutex); }
+static int SceTimed(PthreadCond* cond, PthreadMutex* mutex) { return scePthreadCondTimedwait(cond, mutex, 60000000); }
+static int PosixWait(PthreadCond* cond, PthreadMutex* mutex) { return pthread_cond_wait_nid_postfix(cond, mutex); }
+
+struct Holder {
+    PthreadMutex mutex = nullptr;
+    std::atomic<bool> locked{false};
+    std::atomic<bool> release{false};
+};
+
+static void* APS5_VABI Hold(void* arg) {
+    auto& holder = *static_cast<Holder*>(arg);
+    Require(scePthreadMutexLock(&holder.mutex) == SCE_OK);
+    holder.locked.store(true);
+    while (!holder.release.load())
+        std::this_thread::yield();
+    Require(scePthreadMutexUnlock(&holder.mutex) == SCE_OK);
+    return nullptr;
+}
+
+static void Check(const Wait& wait) {
+    PthreadCond cond = nullptr;
+    Require(scePthreadCondInit(&cond, nullptr, nullptr) == SCE_OK);
+
+    PthreadMutex unlocked = nullptr;
+    Require(scePthreadMutexInit(&unlocked, nullptr, nullptr) == SCE_OK);
+    Require(wait.run(&cond, &unlocked) == wait.notOwned);
+    Require(scePthreadMutexLock(&unlocked) == SCE_OK);
+    Require(scePthreadMutexUnlock(&unlocked) == SCE_OK);
+    Require(scePthreadMutexDestroy(&unlocked) == SCE_OK);
+
+    PthreadMutex uninitialized = nullptr;
+    Require(wait.run(&cond, &uninitialized) == wait.notOwned);
+
+    PthreadMutex adaptive = reinterpret_cast<PthreadMutex>(ADAPTIVE_INITIALIZER);
+    Require(wait.run(&cond, &adaptive) == wait.notOwned);
+
+    PthreadMutex destroyed = nullptr;
+    Require(scePthreadMutexInit(&destroyed, nullptr, nullptr) == SCE_OK);
+    Require(scePthreadMutexDestroy(&destroyed) == SCE_OK);
+    Require(wait.run(&cond, &destroyed) == wait.destroyed);
+
+    Holder holder;
+    Require(scePthreadMutexInit(&holder.mutex, nullptr, nullptr) == SCE_OK);
+    Pthread thread = nullptr;
+    Require(scePthreadCreate(&thread, nullptr, Hold, &holder, nullptr) == SCE_OK);
+    while (!holder.locked.load())
+        std::this_thread::yield();
+    Require(wait.run(&cond, &holder.mutex) == wait.notOwned);
+    holder.release.store(true);
+    Require(scePthreadJoin(thread, nullptr) == SCE_OK);
+    Require(scePthreadMutexDestroy(&holder.mutex) == SCE_OK);
+
+    Require(scePthreadCondDestroy(&cond) == SCE_OK);
+}
+
+int main() {
+    Check({SceWait, SCE_KERNEL_ERROR_EPERM, SCE_KERNEL_ERROR_EINVAL});
+    Check({SceTimed, SCE_KERNEL_ERROR_EPERM, SCE_KERNEL_ERROR_EINVAL});
+    Check({PosixWait, POSIX_EPERM, POSIX_EINVAL});
+    Check({PosixTimed, POSIX_EPERM, POSIX_EINVAL});
+}

docs/dev/TechnicalDebt.md

@@ -150,7 +150,7 @@ Throughout the project, every function at every stage either **does exactly what
 - [sceKernelSyncOnAddressWait](../../core/libs/prx/libkernel/SyncOnAddress/src/SyncOnAddress.cpp) (libkernel) - the only known caller passes a null timeout and a name string as the fourth argument; the timeout is assumed to point to microseconds like the other kernel waits, and the name is ignored
 - [sceKernelSyncOnAddressWait8 / sceKernelSyncOnAddressWait16 / sceKernelSyncOnAddressWait32 / sceKernelSyncOnAddressWait64](../../core/libs/prx/libkernel/SyncOnAddress/src/SyncOnAddress.cpp) (libkernel) - libc's `__std_atomic_wait_direct_8/16/32/64` pass a microsecond timeout or null and ignore the result; the by-value comparand, return values and the 2- and 8-byte alignment of Wait16 and Wait64 are assumed
 - [scePthreadRwlockDestroy, pthread_rwlock_destroy](../../core/libs/prx/libkernel/Pthread/src/Rwlock.cpp) (libkernel) - a rwlock held for reading or writing returns `EBUSY` and keeps its handle, as [KytyPS5](https://github.com/KytyPS5/KytyPS5) does; FreeBSD's [libthr](https://github.com/freebsd/freebsd-src/blob/main/lib/libthr/thread/thr_rwlock.c) and shadPS4 free it without checking, and the console's answer is unverified. Destroying a rwlock while another thread is acquiring it is not guarded
-- [scePthreadCondWait / scePthreadCondTimedwait](../../core/libs/prx/libkernel/Pthread/src/Cond.cpp) (libkernel) - a recursive mutex locked several times is released completely for the wait and relocked to the same depth afterwards, as FreeBSD libthr does (`_mutex_cv_unlock`, `_mutex_cv_lock`); unverified on hardware
+- [scePthreadCondWait / scePthreadCondTimedwait](../../core/libs/prx/libkernel/Pthread/src/Cond.cpp) (libkernel) - before waiting, FreeBSD libthr's [`cond_wait_common`](https://github.com/freebsd/freebsd-src/blob/main/lib/libthr/thread/thr_cond.c#L1992-L2029) checks ownership through [`_mutex_owned`](https://github.com/freebsd/freebsd-src/blob/main/lib/libthr/thread/thr_mutex.c#L1166-L1178): an unowned mutex returns `EPERM`, while its destroyed marker returns `EINVAL`. An untouched adaptive static initializer (slot value 1) therefore returns `EPERM`; the test covers this, an uninitialized slot, a mutex owned by another thread, and the destroyed marker for both timed and untimed sce and POSIX waits. A recursive mutex locked several times is released completely for the wait and relocked to the same depth afterwards, as FreeBSD libthr does (`_mutex_cv_unlock`, `_mutex_cv_lock`). These FreeBSD-derived behaviors remain unverified on console hardware
 - [sceKernelAioSubmitReadCommandsMultiple / sceKernelAioSubmitWriteCommandsMultiple / sceKernelAioWaitRequests / sceKernelAioPollRequests / sceKernelAioCancelRequest / sceKernelAioCancelRequests / sceKernelAioDeleteRequests](../../core/libs/prx/libkernel/Aio/src/Aio.cpp) (libkernel) - behaviour from shadPS4 (one id per request for the Multiple submits, cancel turns any request aborted and id 0 reports processing, wait mode 2 returns once one request completed, poll copies every state without waiting); the 128 requests or ids per batch limit is from SharpProspero, with an unknown error, so larger batches throw; wait modes other than 1 (and) and 2 (or) throw; null pointers, negative counts and invalid ids return the codes the single-request siblings use, checked before any state is written
 - [sceKernelGetAvailableCpumask](../../core/libs/prx/libkernel/Pthread/src/Thread.cpp) (libkernel) - returns the default thread affinity, 0x1FFF (CPUs 0 to 12), as KytyPS5 does. Titles pin threads within it (PPSA02664 passes 0x1FFB, Hades II PPSA36082 0x3 and 0x3F); whether CPU 13 is available to a title is not confirmed
 - [sceKernelGetCurrentCpu](../../core/libs/prx/libkernel/System/src/Process.cpp) (libkernel) - guest affinities are not applied to host threads, so the host CPU index is mapped into the calling thread's affinity clipped to `sceKernelGetAvailableCpumask` (the available mask when they do not overlap): host CPU n reports the (n modulo the CPU count)-th CPU of that mask, so a thread pinned to one CPU reports that CPU, as a FreeBSD `cpuset` guarantees on the console. Which CPU the console reports for a thread allowed on several is not known; shadPS4 returns 0