Repository navigation
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The review found four reproducible source-level defects in buffer handling, I/O error reporting and callback reentrancy.
What I ran
Reviewed all four changed files at head da8313becb61538f182f1e937dd80967bd36c7a7 against base f2d874c1fdccf781c0fa5d86619bb5dd8894b958. The merge preview against current main is clean. Environment: Debian GNU/Linux 12, Python 3.11.2, Git 2.39.5; no compiler or build was run.
git diff --check f2d874c1fdccf781c0fa5d86619bb5dd8894b958...da8313becb61538f182f1e937dd80967bd36c7a7
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14829
python3 tools/pr_abi_audit.py --base f2d874c1fdccf781c0fa5d86619bb5dd8894b958 --head da8313becb61538f182f1e937dd80967bd36c7a7 --json
git merge-tree --write-tree --messages --name-only da8313becb61538f182f1e937dd80967bd36c7a7 85ae7b6218a435318f54141b51348f905480f4e1
git merge-tree --write-tree --messages --name-only da8313becb61538f182f1e937dd80967bd36c7a7 753030c31c4abfe0529dd0ed573ca0af9eb341e3
git merge-tree --write-tree --messages --name-only da8313becb61538f182f1e937dd80967bd36c7a7 9c55d9117c0e8058170c5508351b096f3d8919bb
python3 tools/pr_overlap.py --branch refs/pull/14829/head --repo boykopovar/AnyPS5 --jobs 8Results
| Check | Result | Note |
|---|---|---|
| Diff and conventions | passed | No whitespace errors; hosted conventions passed |
| Export ABI audit | passed | Local audit: 577 symbols, zero findings |
Merge preview against main |
passed | Clean at the reviewed head |
| Pairwise PR previews | failed | Conflicts with #12900, #9345 and #11087 |
| Automated overlap scan | failed | Parser rejected a large GitHub response |
| Hosted checks | passed | Windows, conventions, intake, labels and library portability |
| Local compile and tests | not run | Author reports syntax-only compilation; no tests were run |
Commands and raw results
git diff --check ...: no output; exit status 0.
Unedited precheck table from report.md:
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | The precheck deliberately does not configure, build, or compile AnyPS5. |
| diff_check | passed | git diff --check f2d874c1fdccf781c0fa5d86619bb5dd8894b958...da8313becb61538f182f1e937dd80967bd36c7a7 |
| conventions_and_pr_template | passed | /usr/bin/python3 tools/check_conventions.py --base f2d874c1fdccf781c0fa5d86619bb5dd8894b958 --head da8313becb61538f182f1e937dd80967bd36c7a7 --title feat(libSceUserService): implement recovered missing exports --body /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14829-da8313becb61/pr-body.md |
| changed_export_abi_audit | failed | /usr/bin/python3 tools/pr_abi_audit.py --base f2d874c1fdccf781c0fa5d86619bb5dd8894b958 --head da8313becb61538f182f1e937dd80967bd36c7a7 --json |
| python_syntax | not_applicable | No changed Python implementation files. |
| related_python_unit_tests | not_applicable | No changed tools/ module has an existing sibling tools/tests/test_<module>.py suite. |
| python_test_registration | not_applicable | No changed test_*.py files. |
| live_dependency_and_mergeability_check | passed | /usr/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 14829 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14829-da8313becb61/stack |
| build_dependent_cpp_tests | not_applicable | Not run: C++ tests need exact-head binaries, which require a build; the precheck does not build. |
| test_quality_heuristics | failed | /usr/bin/python3 /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14829-da8313becb61/head-da8313becb61-f47fa5dc/tools/pr_test_quality.py --root /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14829-da8313becb61/head-da8313becb61-f47fa5dc --json core/libs/prx/libSceUserService/Export.cpp core/libs/prx/libSceUserService/InventoryExports.cpp core/libs/prx/libSceUserService/UserService.hpp docs/dev/debt/libSceUserService.md |
| negative_controls | not_applicable | No changed Python test module in tools/ can be run as a no-build base-source control. |
My local ABI audit printed status= passed symbol_count= 577 finding_count= 0.
The overlap scan returned:
pr_overlap: Invalid \uXXXX escape: line 1 column 1582916 (char 1582915)
The exact-head pairwise merge previews returned conflicts in Export.cpp for #12900, #9345 and #11087; #12900 also conflicts in the debt file, and #9345 also conflicts in UserService.hpp and docs/user/USAGE.md. These are integration previews only; no branches were changed.
Raw pairwise merge output:
$ git merge-tree --write-tree --messages --name-only da8313becb61538f182f1e937dd80967bd36c7a7 85ae7b6218a435318f54141b51348f905480f4e1
c6b626685f3d93cf33764cd6e606170cdb6d2b1b
core/libs/prx/libSceUserService/Export.cpp
docs/dev/debt/libSceUserService.md
Auto-merging core/libs/prx/libSceUserService/Export.cpp
CONFLICT (content): Merge conflict in core/libs/prx/libSceUserService/Export.cpp
Auto-merging docs/dev/debt/libSceUserService.md
CONFLICT (content): Merge conflict in docs/dev/debt/libSceUserService.md
$ git merge-tree --write-tree --messages --name-only da8313becb61538f182f1e937dd80967bd36c7a7 753030c31c4abfe0529dd0ed573ca0af9eb341e3
aabd29df7dd66a9f9d06aa068df7ddbf175ac143
core/libs/prx/libSceUserService/Export.cpp
core/libs/prx/libSceUserService/UserService.hpp
docs/user/USAGE.md
Auto-merging core/libs/prx/libSceUserService/Export.cpp
CONFLICT (content): Merge conflict in core/libs/prx/libSceUserService/Export.cpp
Auto-merging core/libs/prx/libSceUserService/UserService.hpp
CONFLICT (content): Merge conflict in core/libs/prx/libSceUserService/UserService.hpp
Auto-merging core/libs/tests/GuestUserServiceUserName.cpp
Auto-merging docs/dev/ENVIRONMENT.md
Auto-merging docs/user/USAGE.md
CONFLICT (content): Merge conflict in docs/user/USAGE.md
$ git merge-tree --write-tree --messages --name-only da8313becb61538f182f1e937dd80967bd36c7a7 9c55d9117c0e8058170c5508351b096f3d8919bb
425ccb2d2aafa225b49817399fd29653b9cb39a8
core/libs/prx/libSceUserService/Export.cpp
Auto-merging core/libs/prx/libSceUserService/Export.cpp
CONFLICT (content): Merge conflict in core/libs/prx/libSceUserService/Export.cpp
Auto-merging docs/dev/debt/libSceUserService.md
gh pr checks 14829 --repo boykopovar/AnyPS5 output at final refresh:
windows pass 10m40s https://github.com/boykopovar/AnyPS5/actions/runs/41339872424/job/124947445317
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41339872424/job/124947446274
register-context skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41339872424/job/124947446294
relinker skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41339872424/job/124947447016
relinker-asan skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41339872424/job/124947446911
write-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41339872424/job/124947446559
conventions pass 15s https://github.com/boykopovar/AnyPS5/actions/runs/41339872294/job/124947420250
eligibility pass 4s https://github.com/boykopovar/AnyPS5/actions/runs/41339872424/job/124947420140
intake pass 5s https://github.com/boykopovar/AnyPS5/actions/runs/41339872311/job/124947420314
labels pass 8s https://github.com/boykopovar/AnyPS5/actions/runs/41339872295/job/124947420292
library-portability pass 1m20s https://github.com/boykopovar/AnyPS5/actions/runs/41339872401/job/124947419612
report pass 1m27s https://github.com/boykopovar/AnyPS5/actions/runs/41339872430/job/124947419832
Negative control
Not run. This PR changes no test sources. The buffer-boundary, short-read, invalid-output-buffer and reentrant-callback cases need focused regression coverage.
Findings
Blocking
-
A sufficient 37-byte path buffer is rejected. The formatted sandbox path is 36 bytes, so it needs 37 bytes including NUL. The current
size <= 0x25check rejects exactly that capacity. Change the condition tosize < 0x25:if (size < 0x25) { -
GetCurrentUserGroupIndexreports success after a failed or short read. It initializes*indexto zero, ignoressceKernelRead’s byte count/error, closes the descriptor and returns success. Propagate negative read errors and reject a short read instead of returning a potentially partial index. -
Home-directory getters can return success with an unusable path.
GetHomeDirectoryandGetSecureHomeDirectorypass the caller’s pointer and size tosnprintf, then return zero without checking for a null destination or truncation. Validate the output pointer and usesnprintf’s result to return an error when the path does not fit. -
A callback can deadlock when it unregisters itself or terminates the service.
NotifyUserEventholds the callback entry’s shared mutex while invoking user code. Self-unregistration and termination synchronously request the same mutex exclusively, so the callback cannot return and release the shared lock. Avoid waiting synchronously on the currently executing callback, or otherwise define and enforce a safe reentrancy contract; add focused coverage.
Non-blocking
The current heads of #12900, #9345 and #11087 are open and pairwise merge previews with this branch conflict. Agree on landing order with the relevant authors and declare Depends on when this PR needs one of them first, as the repository contribution rules require.
The debt file also no longer lists sceUserServiceGetSystemLoggerHashedAccountIdParam, though the export remains a throwing stub; please restore its debt entry.
Sources
- Buffer-size guard at the reviewed head
- Fixed sandbox word at the reviewed head
- Ignored group-index read result
sceKernelReadreturns a byte count or error- Home-directory output functions · secure variant
- Callback dispatch · callback removal · termination cleanup
- Repository overlap/dependency rule at the reviewed base
- Open #12900 change at its exact head · open #9345 change · open #11087 change
Not verified
No local build or tests, no Windows/macOS/Linux runtime test beyond the listed hosted checks, no console/title behavior, and no independent validation of the recovered signatures against firmware. The syntax-only compile in the PR description is author-reported.
To get this to a good state
Fix the four blocking issues and restore the missing debt entry; add focused regression cases for the boundary, I/O and callback behaviors. Coordinate the conflicting PR landing order. I will rerun the no-build precheck, ABI audit, diff check and relevant integration checks on the new head.
|
Thanks for this: recovering the settings model is substantial work, the export set and NIDs are unchanged, and the debt file documents the in-memory limits. The new callbacks and setting writes need changes before it can land, and the size needs a split:
Reviewed at |
da8313b to
615853b
Compare
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The string setter reads a guest string to its terminator before it checks the storage size, and the keyboard debt entry describes storage and error codes the code no longer has.
What I ran
- Head
97ccc8a3bd7f2700066b4a235cca8f39a22560a4(the head moved from615853b763during the review; the new commitfix(libSceUserService): recover remaining settings ABIschangesExport.cpp,UserService.hppand the debt file). PR basee0caf13600. 3 files changed by the new commit; the full PR is 4 files, about +1.7k/-2.3k, which is large under the v4 rule. - Split the PR into four read-only subagent parts (
Export.cpplines 1-1000;Export.cpplines 1001 to the end;UserService.hpp,GuestUserService.cpp; the debt file and build wiring). The subagents read the head615853b763, which is the parent of the new commit. I re-checked the blocking lines below at97ccc8a3bd; the non-blocking items come from the subagents and were not all re-checked at the new head. - Blocking items, verified in the source at
97ccc8a3bd:Export.cpplines 192-195:WriteUserStringSettingcallsstd::strlen(value)before the storage-size check, so a guest string without a terminator inside the buffer is read to its end.docs/dev/debt/libSceUserService.mdline 101: the keyboard-type entry says AnyPS5 stores no per-user keyboard setting and that any other user id returns UNKNOWN_USER. At this headsceUserServiceSetKeyboardTypestores a per-user value (Export.cpplines 1620-1622), andReadKeyboardSettingreturns UNKNOWN_USER only whenUserIndexis below zero (lines 287-293), so a valid but disconnected slot returns NOT_LOGGED_IN.
- No MarcZX10 review exists at this head.
- Precheck report at this exact head (head and base unchanged at completion). Its ABI and test-quality rows failed because their helpers are not in the head tree. The live dependency check failed; see the non-blocking items.
- Hosted: conventions, eligibility, labels, library portability and report passed; the windows job was in progress; linux, relinker, relinker-asan, register-context and write-watch skipped.
Results
| Check | Result | Note |
|---|---|---|
| Diff check | passed | |
| Conventions and PR template | passed | precheck and hosted conventions passed |
| Mergeability and dependencies | failed | the PR depends on #9345, #12752 and #14212, which are not in its branch and overlap its paths; the PR itself merges cleanly against the base |
| String setter bounds | failed | strlen runs before the storage-size check (Export.cpp lines 192-195) |
| Keyboard debt entry | failed | the entry describes storage and error codes the code no longer has (debt line 101) |
Test declaration of ErTMkmSho4E |
not run | a test declares an export with a signature that does not match its definition (non-blocking) |
| Build and runtime tests | not run | no build |
| Negative controls | not run | no build |
| Hosted CI | not run | the windows job was in progress; the other jobs passed or were skipped |
Commands and unedited precheck output
Commands:
- Precheck:
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14829 --output-dir <scratch>/precheck/14829c(report.md and stack/stack-report.md in that directory). - Head fetched by URL (objects only):
git fetch https://github.com/boykopovar/AnyPS5.git pull/14829/head. - Hosted checks:
gh api repos/boykopovar/AnyPS5/commits/97ccc8a3bd7f2700066b4a235cca8f39a22560a4/check-runs.
Precheck table (unedited):
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | The precheck deliberately does not configure, build, or compile AnyPS5. |
| diff_check | passed | git diff --check e0caf1360035286bd22b9f2a3030e97b880ffb28...97ccc8a3bd7f2700066b4a235cca8f39a22560a4 |
| conventions_and_pr_template | passed | /marc/Conlyse-VIEJO/.venv/bin/python3 tools/check_conventions.py --base e0caf1360035286bd22b9f2a3030e97b880ffb28 --head 97ccc8a3bd7f2700066b4a235cca8f39a22560a4 --title feat(libSceUserService): implement recovered user settings --body /dev/shm/reviewer-run/precheck/14829c/pr-body.md |
| changed_export_abi_audit | failed | /marc/Conlyse-VIEJO/.venv/bin/python3 tools/pr_abi_audit.py --base e0caf1360035286bd22b9f2a3030e97b880ffb28 --head 97ccc8a3bd7f2700066b4a235cca8f39a22560a4 --json |
| python_syntax | not_applicable | No changed Python implementation files. |
| related_python_unit_tests | not_applicable | No changed tools/ module has an existing sibling tools/tests/test_<module>.py suite. |
| python_test_registration | not_applicable | No changed test_*.py files. |
| live_dependency_and_mergeability_check | failed | /marc/Conlyse-VIEJO/.venv/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 14829 --repo boykopovar/AnyPS5 --output-dir /dev/shm/reviewer-run/precheck/14829c/stack |
| build_dependent_cpp_tests | not_applicable | Not run: C++ tests need exact-head binaries, which require a build; the precheck does not build. |
| test_quality_heuristics | failed | /marc/Conlyse-VIEJO/.venv/bin/python3 /dev/shm/reviewer-run/precheck/14829c/head-97ccc8a3bd7f-78aaed6e/tools/pr_test_quality.py --root /dev/shm/reviewer-run/precheck/14829c/head-97ccc8a3bd7f-78aaed6e --json core/libs/prx/libSceUserService/Export.cpp core/libs/prx/libSceUserService/UserService.hpp core/libs/tests/GuestUserService.cpp docs/dev/debt/libSceUserService.md |
| negative_controls | not_applicable | No changed Python test module in tools/ can be run as a no-build base-source control. |
Negative control
Not run here. The string-setter fix needs a test with an unterminated guest string, and the test needs a build.
Findings
Blocking
- The string setter reads an unbounded guest string.
Export.cppline 192 callsstd::strlen(value)on the guest pointer, and the storage-size check follows on lines 193-195. A guest buffer with no terminator inside its storage is read past its end beforeBUFFER_TOO_SHORTis returned. Every string setter goes through this helper. Smallest fix: scan at moststorageSizebytes (for example with a bounded search for a NUL inside the storage size), returnBUFFER_TOO_SHORTwhen none is found within it, and add a test with an unterminated buffer. - The keyboard debt entry is wrong at this head.
docs/dev/debt/libSceUserService.mdline 101 says "AnyPS5 stores no per-user keyboard setting" and "any other user id returnsUSER_SERVICE_ERROR_UNKNOWN_USER". The setters now store per-user values, and a valid but disconnected slot returns NOT_LOGGED_IN. Smallest fix: rewrite the entry to describe the stored values, the default for the initial user, and the error codes for invalid ids (UNKNOWN_USER outside the four slots) and disconnected slots (NOT_LOGGED_IN).
Non-blocking
- Dependencies: the PR body declares #9345, #12752 and #14212, but their heads are not in this branch. The stack check reports that #9345 and #14212 conflict with the current base on their own, and that all three overlap paths this PR changes. Please state the merge order, or rebase after those land.
GuestUserService.cppline 10 declareslibSceUserServiceUnknown_ErTMkmSho4E_nid_no_patchwith parameters, but the definition inInventoryExports.cppline 87 takes none and the test never calls it. A test should not declare an export with a mismatching signature (CONTRIBUTING line 6). Please delete the line, or call the export with its real signature.- Key aliasing (not re-checked at the new head): the debt and code use the same key for differently named settings (
0x0780a040for TriggerEffectStrength and TriggerEffectEnabled;0x0780a005for the vibration pair;0x0780a03bfor the gaze pair), and0x0780b004is used both as an integer (SetNpAuthErrorFlag) and as a string (GetNpOfflineAccountId, which has no setter). If these IDs are not truly shared on the console, one write overwrites the other. Please verify each alias against the recovered evidence. - Behaviour change not in the PR body: the keyboard getters now return NOT_LOGGED_IN for a disconnected slot, where the base returned the system-language value.
sceUserServiceGetSparkEqProfilesucceeds without a login check for the default profile indices, but returns NOT_LOGGED_IN for the custom profile.sceUserServiceSetGazeCalibDatamakes two separate writes (2000 bytes each), each with its own login check, so a logout between them leaves half the data written.sceUserServiceGetGamePresetsreturns zeroed fields with success and reads no state; it is not listed under Silent stubs.- The PR's debt entries describe some recovered keys and defaults that depend on a local Ghidra project not in the repository; those are not verified here.
Sources
- String setter:
AnyPS5/core/libs/prx/libSceUserService/Export.cpp
Lines 186 to 197 in 97ccc8a
- Keyboard setter and reader: and #L1620-L1622
AnyPS5/core/libs/prx/libSceUserService/Export.cpp
Lines 287 to 295 in 97ccc8a
- Keyboard debt entry:
AnyPS5/docs/dev/debt/libSceUserService.md
Line 101 in 97ccc8a
- Test declaration:
- Dependency report: /dev/shm/reviewer-run/precheck/14829c/stack/stack-report.md
Not verified
- No build or test run here.
- The new commit's changes to stub signatures (
UserService.hpp,Export.cpp) were not checked against any firmware evidence. - The non-blocking items were read at
615853b763; the new commit changedExport.cppand the debt file, so their line numbers may differ at97ccc8a3bd. - The windows job was still in progress.
To get this to a good state
- Bound the string read to the storage size and add an unterminated-buffer test; rewrite the keyboard debt entry to match the code. Re-run the precheck and the user-service tests on the new head.
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, changes requested
The new head addresses the earlier source-level buffer, callback and readback findings, but the settings implementation still has unsupported behavior and incomplete firmware evidence.
What I ran
Reviewed all four changed files at head 97ccc8a3bd7f2700066b4a235cca8f39a22560a4 against PR base e0caf1360035286bd22b9f2a3030e97b880ffb28. The stack preview also tested the head against current main 5bae1a4bc2cffc66c13e37ce8e63ae48b1b5dd1e and found the PR itself clean. Environment: Debian 12, x86_64, Python 3.11.2, Git 2.39.5. No local build or C++ tests were run.
git diff --check e0caf1360035286bd22b9f2a3030e97b880ffb28...97ccc8a3bd7f2700066b4a235cca8f39a22560a4
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14829
python3 tools/pr_abi_audit.py --base e0caf1360035286bd22b9f2a3030e97b880ffb28 --head 97ccc8a3bd7f2700066b4a235cca8f39a22560a4 --json
python3 tools/pr_stack_check.py 14829 --repo boykopovar/AnyPS5
python3 tools/pr_test_quality.py --root /marc/AnyPS5 --json core/libs/tests/GuestUserService.cpp
python3 tools/check_test_registration.py --scripts
gh pr checks 14829 --repo boykopovar/AnyPS5The PR description reports a Windows build and focused guest test run; I did not run those. CMake source inspection confirms that GuestUserService.cpp is registered in guest_user_service_tests.
Results
| Check | Result | Note |
|---|---|---|
| Diff and conventions | passed | No whitespace errors; conventions passed. |
| Export ABI audit | passed | 541 symbols, no findings. |
| Guest test registration | passed | CMake registers the changed test source. |
| Test-quality heuristic | failed | Its unregistered-test warning is a false positive; registration was confirmed in CMake. |
| Stack preview | passed | This PR merges cleanly; declared dependency heads are not all included yet. |
| Hosted checks | not run | Windows remains pending; policy checks passed, other jobs skipped. |
| Local build and C++ tests | not run | The author reports a Windows run; not independently verified. |
Captured check output
$ git diff --check e0caf1360035286bd22b9f2a3030e97b880ffb28...97ccc8a3bd7f2700066b4a235cca8f39a22560a4
(no output)
$ python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14829
/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14829-97ccc8a3bd7f/report.md
/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14829-97ccc8a3bd7f/report.json
Conclusion: checks failed
Unedited precheck rows:
| diff_check | passed | `git diff --check e0caf1360035286bd22b9f2a3030e97b880ffb28...97ccc8a3bd7f2700066b4a235cca8f39a22560a4` |
| conventions_and_pr_template | passed | `/usr/bin/python3 tools/check_conventions.py --base e0caf1360035286bd22b9f2a3030e97b880ffb28 --head 97ccc8a3bd7f2700066b4a235cca8f39a22560a4 --title feat(libSceUserService): implement recovered user settings --body /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14829-97ccc8a3bd7f/pr-body.md` |
| changed_export_abi_audit | failed | `/usr/bin/python3 tools/pr_abi_audit.py --base e0caf1360035286bd22b9f2a3030e97b880ffb28 --head 97ccc8a3bd7f2700066b4a235cca8f39a22560a4 --json` |
| live_dependency_and_mergeability_check | failed | `/usr/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 14829 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14829-97ccc8a3bd7f/stack` |
| test_quality_heuristics | failed | `/usr/bin/python3 /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14829-97ccc8a3bd7f/head-97ccc8a3bd7f-00a90815/tools/pr_test_quality.py --root /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14829-97ccc8a3bd7f/head-97ccc8a3bd7f-00a90815 --json core/libs/prx/libSceUserService/Export.cpp core/libs/prx/libSceUserService/UserService.hpp core/libs/tests/GuestUserService.cpp docs/dev/debt/libSceUserService.md` |
Unedited missing-helper stderr:
/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14829-97ccc8a3bd7f/head-97ccc8a3bd7f-00a90815/tools/pr_abi_audit.py': [Errno 2] No such file or directory
/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14829-97ccc8a3bd7f/head-97ccc8a3bd7f-00a90815/tools/pr_test_quality.py': [Errno 2] No such file or directory
The standalone ABI audit returned status= passed, symbol_count= 541, finding_count= 0. The standalone stack report says PR #14829 is clean against current main; #9345 and #14212 conflict with main, and their current heads are not included in this dependent branch. No rebase is requested while those dependencies remain open.
$ python3 tools/check_test_registration.py --scripts
232 test script(s), each run by an add_test or a workflow
$ gh pr checks 14829 --repo boykopovar/AnyPS5
labels pass 6s https://github.com/boykopovar/AnyPS5/actions/runs/41346108717/job/124966208296
windows pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41346109469/job/124966189677
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41346109469/job/124966191341
register-context skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41346109469/job/124966191645
relinker skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41346109469/job/124966191435
relinker-asan skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41346109469/job/124966191432
write-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41346109469/job/124966191132
conventions pass 23s https://github.com/boykopovar/AnyPS5/actions/runs/41346108696/job/124966159645
eligibility pass 5s https://github.com/boykopovar/AnyPS5/actions/runs/41346109469/job/124966158268
library-portability pass 1m48s https://github.com/boykopovar/AnyPS5/actions/runs/41346109377/job/124966158030
report pass 1m25s https://github.com/boykopovar/AnyPS5/actions/runs/41346109302/job/124966157471
Negative control
Not run by me. The PR description reports a focused guest test run, but no base-versus-change negative control was provided or independently run.
Findings
Blocking
-
The Registry-backed behavior is only process-local. The debt entry says these firmware settings use Registry access paths, but the new helpers keep values in process-local maps and return success after updating those maps. The same entry explicitly says values do not survive process restarts. That is not the Registry-backed behavior the PR says it implements, and the repository requires exports to implement their behavior or throw. Please connect these settings to the persistent backend, or keep the affected exports throwing until that behavior is implemented.
-
The recovered mappings are not cited at the source locations required by the repository. The debt entry identifies three example integer setter sites, then attributes hundreds of keys, defaults and field widths to a local Ghidra project without giving the Registry access sites and what each establishes. It also admits that defaults for some settings are AnyPS5 model values. Please add grouped module offsets and explain which sites establish the keys, widths and defaults, and keep getters whose defaults remain unknown throwing until supported by evidence.
Non-blocking
The added test checks representative integer, string and binary round trips and per-user isolation. Shared helper boundaries such as null or undersized buffers, wrong-sized binary writes and writes for disconnected users are not covered; adding those cases would strengthen the test suite.
Sources
- Process-local storage helpers and success returns · integer and string helpers
- Debt statement for integer, string and binary settings
- Required behavior when an export cannot be implemented
- Required evidence for reverse-engineered behavior
- New representative settings tests · CTest registration
Not verified
No local build, C++ tests, Windows/macOS/Linux runtime test or console/title behavior was independently verified. The author's Windows test run and private Ghidra research were not independently inspected. The hosted Windows check was still pending at the last refresh.
To get this to a good state
Provide the source-site citations and implement persistent settings behavior; leave unresolved defaults unsupported until their behavior is established. I will repeat the diff, convention, ABI, stack and relevant test checks on the updated head.
|
I compared this change with the PS5 firmware 3.20 and 4.03 modules (libSceUserService on both, SceShellCore on 3.20). Summary:
Keys. Of the 398 functions implemented through a setting key:
The remaining stubs. The 75 stubs are exported by libSceUserService 3.20 and 4.03 with identical code in the two. The 40 in The 40 older settings. Their query ids (for example the file browser and selector 0x702 to 0x707, share button 0x701, IME 0x400 to 0x405, theme 0x800 to 0x834) have no entry in SceShellCore 3.20's user-setting table of 223 query id to registry key pairs, and no other 3.20 binary I looked at contains that table. I found no server-side handler for them in 3.20. Defaults. The enable-HRTF default 1 matches the 3.20 registry initial value. The 3.20 registry module lists initial values by entry name, without each user entry's numeric key. |
|
I took the explicit fail-fast option from the review. The process-local integer/string/binary maps, invented defaults and surrogate round-trip tests are removed. All upstream function bodies are preserved; previously unimplemented settings continue to throw. The retained 482 declarations are now explicitly provisional call shapes, with no claim that compilation establishes a recovered firmware ABI. The PR is retitled and rewritten around that narrower result. The debt entry retains example forwarding locations and explains precisely what they do not establish. The library builds, the existing UserService test passes, and public-header/conventions checks pass. Persistent settings behavior and per-export verification remain future work rather than unsupported success paths in this PR. AI-assisted: yes. |
|
Thanks for the independent 3.20/4.03 comparison. The 209 confirmed key mappings and the HRTF default are useful evidence; your distinction between Registry keys, wrapper query IDs and unserved legacy queries is important. The revised PR takes the requested fail-fast route while persistence and per-export behavior remain unresolved: it preserves upstream bodies and retains provisional declarations without claiming operational settings support. Your source comparison remains part of the review evidence and does not get reclassified as proof of complete 13.60 behavior. AI-assisted: yes. |
|
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
There was a problem hiding this comment.
Reviewed — no blocking findings; the settings exports get typed signatures, the bodies keep their throwing behaviour, and the Windows run passed
This PR gives the (void) placeholder prototypes for the settings exports of libSceUserService their parameter lists, and renames the parameters to snake_case. The debt entry says the affected exports keep their upstream throwing bodies. The diff is large, so I read a sample of it; the rest is not verified in this pass.
What I ran
- Head
7caefcaf3a7be3025547084b904bb3dbe5be617f; basee0caf1360035286bd22b9f2a3030e97b880ffb28. 3 files:libSceUserService/Export.cpp(964 changed lines),libSceUserService/UserService.hpp(1004 changed lines) anddocs/dev/debt/libSceUserService.md(+5). About 987 added and 984 removed lines remain when whitespace is ignored, so the change is a real signature change, not a reformat. - Read a sample: the first 25 added lines of
Export.cppand the first 60 added lines ofUserService.hpp. Read the debt entry in full. The rest of the changed lines were not read. - The debt entry says the declarations are provisional, that every affected export keeps its upstream throwing body, and that previously implemented getters keep their behaviour. It cites a firmware 13.60
libSceUserService.sprx(SHA-256 given in the entry) as evidence. I did not check that binary. - Hosted, Windows CTest: 2589 of 2589 passed.
- Read-only checks from the trusted main checkout: diff check, conventions and PR template passed. ABI audit passed: 541 symbols. Stack check: clean, no declared dependencies, no errors.
- No MarcZX10 review at this head.
Results
| Check | Result | Note |
|---|---|---|
| Diff check | passed | |
| Conventions and PR template | passed | conventions check from the main checkout |
| Mergeability and dependencies | passed | stack check: clean; no declared dependencies |
| ABI audit (main-checkout tool) | passed | 541 symbols |
| Windows CTest (hosted) | passed | 2589 of 2589 |
| Typed signatures (sample read) | passed by reading | the sample matches the debt entry; most changed lines not read |
| Affected exports keep their throwing bodies | not verified | taken from the debt entry; Export.cpp bodies not read in full |
| Firmware 13.60 evidence cited in the debt entry | not verified | not checked here |
| Negative controls | not run | no local build |
Read-only check output
Precheck was not run: no PR-head code is run here. Diff check: passed. Conventions: passed. ABI audit: passed (541 symbols). Stack check: clean, no declared dependencies, no errors. Hosted: gh api repos/boykopovar/AnyPS5/commits/7caefcaf3a7be3025547084b904bb3dbe5be617f/check-runs, and the Windows job log (2589 of 2589).
Negative control
Not run here. A negative control needs a local build, and code from this head is not run here.
Findings
Blocking
None from the code read.
Non-blocking
- The debt entry says these declarations are provisional, and that the layouts, storage, validation and error behaviour are not yet established. This review adds no evidence either way on those points.
Sources
- Header: https://github.com/boykopovar/AnyPS5/blob/7caefcaf3a7be3025547084b904bb3dbe5be617f/core/libs/prx/libSceUserService/UserService.hpp
- Export: https://github.com/boykopovar/AnyPS5/blob/7caefcaf3a7be3025547084b904bb3dbe5be617f/core/libs/prx/libSceUserService/Export.cpp
- Debt entry: https://github.com/boykopovar/AnyPS5/blob/7caefcaf3a7be3025547084b904bb3dbe5be617f/docs/dev/debt/libSceUserService.md
- Windows job log: check-run
windowson 7caefca
Not verified
- Most of the changed lines in
Export.cppandUserService.hpp. - The throwing bodies of the affected exports.
- The firmware 13.60 evidence cited in the debt entry.
To get this to a good state
- Nothing blocking.
Upstream reconciliationPushed This merges upstream The reconciliation preserves current upstream implementation bodies while carrying forward:
Each signature replacement was checked against the original PR changes and the current upstream source. No existing upstream function bodies were replaced. The PR remains limited to provisional call-shape declarations, not operational settings support. GitHub now reports the PR as mergeable. New-head CI is pending; no fresh build or runtime result is claimed. Requesting re-review against this reconciled head. AI-assisted: yes. |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for narrowing this. Remaining points:
- The PR is conflicting with
mainagain (core/libs/prx/libSceUserService/UserService.hpp,docs/dev/debt/libSceUserService.md), and it overlaps #14896 onUserService.hpp. A ~1000-line signature rewrite of one header will keep conflicting; please split it. - ~480 of the new signatures (
userId,value,sizetypes) are stated in the PR and in the debt entry to be unverified hypotheses. The bodies still throw, so nothing is gained at runtime, and a guessed prototype is the thing a later real implementation has to unlearn. Only the three call shapes with firmware evidence (lAR1nkEoMBo,ZfUouUx2h8w,dCdhOJIOtR4) should change here; leave the rest as they are onmainuntil each has its own evidence (the type, size and direction of each argument, from the call site). - The debt entry should then list only what is actually recovered and what is still unknown for those exports.
Please rebase on current main after trimming.
What
Retain 482 provisional settings call shapes while preserving every upstream function body. Removed the process-local settings maps, assumed defaults, Registry-key claims and surrogate round-trip tests. Newly declared settings remain explicitly unimplemented; previously implemented upstream getters keep their behavior.
These declarations are research hypotheses, not verified console source-language signatures. Firmware 13.60 libSceUserService.sprx forwarding examples at 0x5f20 (lAR1nkEoMBo), 0x5ee0 (ZfUouUx2h8w) and 0x5ea0 (dCdhOJIOtR4) dispatch queries 0x500, 0x501 and 0x502. They establish query identity/argument storage, not persistent Registry behavior, defaults or a complete per-export ABI. The debt entry records this limitation and the module digest. Static inspection has not been confirmed by console execution.
The full settings implementation requires a persistent backend and per-export evidence. This narrowed PR adds no operational settings behavior and depends on no user-name, formatting, language, callback or group implementation. Shared-file reconciliation remains necessary if adjacent PRs land first.
Tested
Windows x86_64, WinLibs GCC, CMake/Ninja Release: built libSceUserService and guest_user_service_tests; guest_user_service passed. Public-header audit, conventions and diff whitespace checks passed. A source comparison confirms upstream function bodies and the existing guest test were restored. Build success checks host consistency only. No settings persistence, firmware behavior or console test is claimed; new-head hosted checks are pending.
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)