Repository navigation
fix(libkernel): mark a destroyed condition variable with 1 and accept destroying a null one, as measured on a PS5 - #10177
Conversation
… destroying a null one, as measured on a PS5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| @@ -175,6 +175,8 @@ int APS5_VABI scePthreadCondDestroy(PthreadCond* cond) { | |||
| std::lock_guard lock(InitializationMutex()); | |||
| if (*cond == destroyedCond()) | |||
There was a problem hiding this comment.
I rechecked the lifetime path and need to correct my concern: resolveCond copies the shared_ptr<TimedWait::Condition> while holding the same InitializationMutex, so an in-flight signal/wait retains the condition state after the private wrapper is deleted. With every slot transition serialized by that mutex, I do not have evidence that the destroy path causes the use-after-free I initially described. Please disregard my earlier lifetime claim. The implementation question I still see is narrower: the slot check uses a plain *cond read while resolveCond uses atomic_ref; both currently hold InitializationMutex, so they are serialized with each other, but the mixed access is fragile if any slot access is later moved outside the lock. I do not consider that alone a blocking issue in this patch, and no change is required based on my earlier comment.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the exact submitted head a431d14efe8d970737861fc157ef36c47a0e4524. The destroyed marker now matches the reported firmware behavior, and null destroy leaves an uninitialized slot untouched; the regression covers both cases and verifies that signal/broadcast still lazily initialize null slots. The PR reports 17 condition/destroyed-object tests passing three times on Windows, and the policy checks (eligibility, conventions, intake, labels and report) pass. Product jobs are skipped by repository path policy, and I could not independently run the focused executable because this checkout has no configured build tree. I initially raised and then retracted a condition-lifetime concern after tracing the shared ownership and initialization mutex; that concern is not a blocker.
|
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.
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I checked the overlap warning against #9165. That PR adds a separate timed self-lock debt bullet; this PR updates the condition-wait entry and changes only Cond.cpp plus the destroyed-object regression. The implementation behavior is independent. If stale debt-file context causes a rebase conflict, preserve both entries; no Depends on relationship is needed.
What
Destroyed condition variables measured on a retail PS5 (firmware 13.60) with homebrew probes calling
libkernel_sys.sprxfrom a payload process. The sources and console logs are in this gist (pcond,pcond2). Split from #10130; @BunnyFl confirmed the same results against firmware 13.42 in #10130.Destroyed condition variables.
scePthreadCondDestroystores 1 in the slot, FreeBSD libthr'sTHR_COND_DESTROYED.scePthreadCondWait,scePthreadCondTimedwait,scePthreadCondSignal,scePthreadCondBroadcastandscePthreadCondDestroythen all returnEINVAL(0x80020016), and so does each of them for a slot that holds 1. AnyPS5 stored 2 and treated a slot holding 1 as a condition object, so it read through address 1.Destroying an uninitialized slot (null) returns 0 and leaves it null. Before, it stored the destroyed marker. Signal, broadcast and wait on a null slot initialize it, as before.
Tested
Windows 11, MinGW (winlibs).
guest_pthread_destroyed_objectschecks the stored 1 and a null slot through destroy, signal and broadcast. It fails onmainand passes with the change.tools/check_conventions.pypasses.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)