Repository navigation
fix(libSceUserService): validate user login status in sceUserServiceGetNpAccountId - #12165
Conversation
|
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.
|
|
A Windows run, since the description lists none. Head
AI-assisted: yes (Claude Code). |
MarcZX10
left a comment
There was a problem hiding this comment.
The login-state validation is consistent with the existing user-service API pattern, and the tests cover logged-in, disconnected and absent users, including null outputs. Exact-head conventions and diff checks pass; merge preview with current main is clean. The author reports standalone unit tests, and a separate Windows run reports all three user-service tests passing 3/3 across three runs; hosted Windows passes, while hosted Linux is path-skipped.
Two repository requirements remain:
- Open #11999 also changes
docs/dev/debt/libSceUserService.md;git merge-treeconfirms a content conflict when combined with this PR. Please coordinate merge order and updateDepends on:to name the prerequisite, ornoneafter the authors agree there is no dependency. - This branch is based on
de004fe, 101 upstream commits behind currentmain.CONTRIBUTING.mdrequires branches to be based on and kept up to date withmain. Please rebase after resolving the overlap and rerun the focused tests/checks.
a413295 to
6f757ec
Compare
|
@MarcZX10 Rebased cleanly onto latest upstream/main (9fc181d). Regarding #11999: #11999 modifies the sceUserServiceInitialize entry whereas this PR updates the sceUserServiceGetNpAccountId entry. There is no logical dependency (Depends on: none confirmed in checklist), and any textual rebase on the debt doc upon merge order will be straightforward. Verified conventions with python tools/check_conventions.py --base upstream/main --head HEAD (0 findings). |
|
Windows result for head Host: Windows 11 26300, Intel i7-12700K, WinLibs GCC 15.2.0 (x86_64-ucrt-posix-seh, r6), Release, The tree also carried the open PRs #12290, #12288, #12285, #12269, #12265, #12232, #12206, #12201, #12199, #12197, #12146, all merged cleanly, so the run is a joint one; the tests of this PR are AI-assisted: yes (Claude Code), the runs and the numbers above are from this machine. |
6f757ec to
fe68a41
Compare
fe68a41 to
697fb7e
Compare
|
@MarcZX10 Rebased cleanly onto current |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — changes requested for debt-file overlap order
The branch is now current and its checks pass, but the open overlap with #11999 still has no agreed merge order, and combining the two heads currently conflicts in the debt file.
What I ran
- Reviewed exact head
697fb7e217accfc0b68295a3b799711b91999d76on currentmain3a2b91981651569163768854a1fcdc7193b6667b. git merge-tree --write-tree upstream/main upstream/pr-12165— clean current-main preview, tree2202b4ecb7df7275cf9894b8847054dd800a232a.git merge-tree --write-tree upstream/pr-11999 upstream/pr-12165— content conflict indocs/dev/debt/libSceUserService.md.git diff --check upstream/main...upstream/pr-12165andpython3 tools/check_conventions.py --base upstream/main --head upstream/pr-12165— passed.- Hosted eligibility, conventions, labels and report checks pass; Windows passes. Linux and specialized jobs are skipped. The author also reports clean CIs and conventions on this head.
Results
| Check | Result |
|---|---|
| Current-main base and merge preview | Current and clean |
| Convention and whitespace checks | Passed |
| Current hosted Windows | Passed |
| Combined merge with #11999 | Conflicts in docs/dev/debt/libSceUserService.md |
| Focused user-service CTest | Previously reported pass; not rerun locally on this head |
Findings
The code change and login-state assertions remain consistent with the existing user-service behavior. The new head resolves the previous stale-base concern.
The remaining issue is coordination: #11999 is still open, both PRs edit the same debt entry, and the current pairwise merge preview conflicts. The two code changes may be independent, but the contribution guide requires authors to agree on overlap order and record it. Depends on: none and a note that the textual rebase will be straightforward do not record which PR goes first. Please coordinate with #11999's author, then document the agreed order in both descriptions (and add a dependency only if that order requires one).
Sources
- Current PR head
- Open overlapping PR #11999
- Contribution rule on keeping branches current and resolving overlaps
Not verified
I did not rerun guest_user_service locally on this head or verify console behavior. The prior Windows run is from the earlier PR head; the current hosted Windows job is green.
To get this to a good state
Agree and document the merge sequence with #11999, preserve both debt entries when the second PR rebases, and request re-review.
697fb7e to
e4b1dae
Compare
|
@MarcZX10 #11999 has merged upstream into |
|
@slopgobblercob Console rows from a PS4 Pro (firmware 10.71): the system Before initialization the console answers 0x80960002 ( AI-assisted: yes (Claude Code) |
adriadam10
left a comment
There was a problem hiding this comment.
@ntamas94's console measurement (PS4 Pro, firmware 10.71, system libSceUserService.sprx) contradicts the error code this PR pins:
- For a user that is not logged in (
initial+1, 123, -1, 0, 255),sceUserServiceGetNpAccountIdreturns0x80960105(USER_SERVICE_ERROR_UNKNOWN_USER) and leaves the output untouched.sceUserServiceGetUserNamein the same run returns0x80960009for the same users, so the two calls differ on the console and "matching the existing pattern" does not hold for this one. - The PR body says "matching console error behavior"; the measurement says otherwise.
What would fix it:
core/libs/prx/libSceUserService/Export.cpp:355: returnUSER_SERVICE_ERROR_UNKNOWN_USERinstead ofUSER_SERVICE_ERROR_NOT_LOGGED_INwhen!IsUserLoggedIn(user_id), output untouched.core/libs/tests/GuestUserService.cpp:103,105,121,123,203,205,249,251: expectUSER_SERVICE_ERROR_UNKNOWN_USER(the0xdeadbeefuoutput checks stay).docs/dev/debt/libSceUserService.md:5: replace the0x80960009sentence with the measured0x80960105, and say the rows come from a PS4 Pro (fw 10.71) while PS5 is not measured for this call.- Update the
Whatsection so it no longer claims0x80960009.
Everything else checked is fine (rebase on top of #11999, Windows runs from @JJenkx and @maskym, the tests fail on main for user 123).
Re-checked — PS5 behavior remains unverifiedThe PS4 10.71 measurement is relevant counterevidence, but it does not establish behavior for this PS5 export; I retract my earlier characterization that the implementation matches an established console pattern. At head The PR is based on Sources
Not verifiedPS5 behavior for these user states has not been independently measured; the reported console probe is a PS4 Pro running firmware 10.71. The local test was not run on physical PS5 hardware. |
e4b1dae to
7702e8f
Compare
|
Thanks @adriadam10 and @MarcZX10! Updated head
|
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, changes requested
The branch is current and the #11999 overlap is resolved, but the new null-output assertions claim behavior the cited console probe did not measure.
What I ran
Reviewed exact head 7702e8f8701b79cfe5cbf8d3fd75846538fa791f against main at 673fa865f418876429bd871fdda2ab15ac1a0831; the merge preview was clean (tree 09609651c45f35940cd6ce2b4eda0af6ee3f651b). Environment: Linux 6.1.0-54-amd64, x86-64, Python 3.11.2; no compiler was used.
git merge-tree --write-tree 673fa865f418876429bd871fdda2ab15ac1a0831 refs/review/pr-12165-current
git diff --check 673fa865f418876429bd871fdda2ab15ac1a0831...refs/review/pr-12165-current
python3 tools/check_conventions.py --base 673fa865f418876429bd871fdda2ab15ac1a0831 --head refs/review/pr-12165-currentResults
| Check | Result |
|---|---|
| Current-base merge preview | Passed; clean merge |
git diff --check |
Passed (exit 0; no output) |
| Conventions | Passed |
| Hosted CI | Eligibility, conventions, labels, report, and Windows passed; Linux and specialized jobs were skipped. A newer conventions/labels run was queued at refresh. |
| Console evidence | The author-provided PS4 Pro 10.71 probe covers non-null outputs only; it explicitly says no null pointers were tested. |
Unedited local check output
09609651c45f35940cd6ce2b4eda0af6ee3f651b
All conventions checks passed.
Findings
Blocking
GuestUserService.cppnow asserts thatsceUserServiceGetNpAccountId(unloggedUser, nullptr)returnsUSER_SERVICE_ERROR_UNKNOWN_USER(lines 105 and 123). The cited probe explicitly used no null pointers, so it does not establish whether login-state or pointer validation takes precedence. The test therefore pins unmeasured behavior. Please add a console probe for these null-output cases, or remove the assertions and record the precedence as unresolved rather than treatingUNKNOWN_USERas confirmed for them.
Non-blocking
- The measured non-null-output result is from PS4 Pro firmware 10.71. PS5 behavior remains unverified; keep that distinction explicit in the debt entry and PR description.
Sources
- New null-output assertions at the reviewed head
- Implementation at the reviewed head
- Author-provided PS4 Pro probe report
Not verified
I did not build or run GuestUserService; the PR description's test result is author-provided. No PS5 hardware behavior or null-pointer behavior on console was independently tested.
To get this to a good state
Provide console evidence for null output with an unlogged user, or remove the unmeasured assertions and document the unresolved precedence. I will rerun these checks on the updated head.
|
@MarcZX10 @slopgobblercobbler PS5 system-module evidence for the null-output case: firmware 13.42's own
The run: the module's constructors ran in
The stand-in was not written to, and in the disassembly all three handlers ( AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The firmware-module evidence reported in the latest comment conflicts with the current null-output checks for users who are not logged in.
What I ran
Reviewed exact head 7702e8f8701b79cfe5cbf8d3fd75846538fa791f against recorded PR base 39fc186c978aa6332538fb1101d1722124979ab5; the current-main preview used 99b571f879cd1ef3de7b8af21261675826d75943 and merged cleanly to c95713558e1a5c00bae1938df1d297b24d9bc2e3. Environment: Linux workspace; no compiler, build, or test was run.
git merge-tree --write-tree refs/review/session-main refs/review/session-pr-12165
git diff --check refs/review/session-main...refs/review/session-pr-12165
python3 tools/check_conventions.py --base refs/review/session-main --head refs/review/session-pr-12165
gh pr checks 12165 --repo boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean |
git diff --check |
Passed; no output |
| Conventions | Passed |
| Hosted conventions, labels, eligibility, report | Passed |
| Hosted Windows | Job passed; product build and test execution were skipped (host-only test linking ran) |
| Hosted Linux and specialized jobs | Skipped |
| PR body test report | GuestUserService suite is reported passing, but no exact test head/output is given there |
Unedited command output
$ git merge-tree --write-tree refs/review/session-main refs/review/session-pr-12165
c95713558e1a5c00bae1938df1d297b24d9bc2e3
$ git diff --check refs/review/session-main...refs/review/session-pr-12165
[no output; exit status 0]
$ python3 tools/check_conventions.py --base refs/review/session-main --head refs/review/session-pr-12165
All conventions checks passed.
$ gh pr checks 12165 --repo boykopovar/AnyPS5
conventions pass 15s https://github.com/boykopovar/AnyPS5/actions/runs/40980297446/job/123818386241
labels pass 7s https://github.com/boykopovar/AnyPS5/actions/runs/40980297417/job/123818386412
windows pass 3m29s https://github.com/boykopovar/AnyPS5/actions/runs/40977650482/job/123811274954
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40977650482/job/123811277040
register-context skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40977650482/job/123811277420
relinker skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40977650482/job/123811277155
relinker-asan skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40977650482/job/123811276922
write-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/40977650482/job/123811276686
eligibility pass 6s https://github.com/boykopovar/AnyPS5/actions/runs/40977650482/job/123809542540
report pass 39s https://github.com/boykopovar/AnyPS5/actions/runs/40977650354/job/123809541760
Findings
Blocking
- Null-output precedence for an unlogged-in user is wrong. At this head,
sceUserServiceGetNpAccountIdcallsIsUserLoggedInbefore checkingaccount_id, so a logged-out user with a null output returnsUSER_SERVICE_ERROR_UNKNOWN_USER. The author-reported firmware 13.42 module probe says a null output returns0x80960005(USER_SERVICE_ERROR_INVALID_ARGUMENT) for every user after initialization, before the user/login check. Please checkaccount_id == nullptrfirst and update the null-output assertions in the absent-user and logged-out-user cases to expectUSER_SERVICE_ERROR_INVALID_ARGUMENT; retain the non-null absent-user assertions forUNKNOWN_USER.
Non-blocking
The firmware result is author-reported from disassembly and a native x86-64 harness with imports stubbed, not a physical-console run. The updated hosted Windows job did not execute the product tests.
Sources
- Implementation at this head
- Affected assertions at this head
- Firmware-module evidence reported by the author
Not verified
I did not independently run the firmware module, build the project, or run GuestUserService. Physical-console behavior is not independently confirmed.
To get this to a good state
Move the null-output check before the login-state check and update the affected assertions. I will rerun these source checks and the relevant test on the new head.
|
Checked the new head against the previous review: |
…er check Firmware 13.42 libSceUserService.sprx returns INVALID_ARGUMENT (0x80960005) from sceUserServiceGetNpAccountId for a null output before it looks at the user, as BunnyFl showed with its disassembly and a native run with stubbed imports on #12165. The UserService tests now expect INVALID_ARGUMENT for a null output with an unknown or logged-out user, and the debt entry names that source and the unmodelled not-initialized order. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
What
Validates user login status in
sceUserServiceGetNpAccountId.USER_SERVICE_ERROR_UNKNOWN_USER(0x80960105) when the user is not logged in, leaving destinationaccount_iduntouched per console measurement on PS4 Pro firmware 10.71.USER_SERVICE_ERROR_INVALID_ARGUMENTwhenaccount_idis null for a logged-in user.core/libs/tests/GuestUserService.cppensuringaccount_idremains unmodified on failure.docs/dev/debt/libSceUserService.md.Tested
python tools/check_conventions.py --base upstream/main --head HEAD: 0 findings.GuestUserServicepasses.Checklist