Repository navigation
fix(libSceNpAuth): return INVALID_ARGUMENT from async poll and wait for unwritable pointers - #13985
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — looks good
At head 735af9208028310c03b761cf5c8fbeffee748570, the new guarded-copy failures return ORBIS_NP_AUTH_ERROR_INVALID_ARGUMENT, consistent with the existing null-output checks, and the regression assertions cover both APIs (Export.cpp, GuestNpAuth.cpp). Hosted Windows, conventions, library portability, intake, labels, eligibility, and report checks passed. The author reports the focused CTest passed; I did not run a local build or test, and product CI jobs were skipped.
|
@pushkarreddyy reviewed at
|
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, changes requested
At head 735af9208028310c03b761cf5c8fbeffee748570 against base 73f4f7e06e4f210a8c6570e335fd654dc0a86134, the implementation’s first-poll path returns NP_POLL_ASYNC_RUNNING before it attempts the result copy, so the debt entry overstates the invalid-pointer behavior.
What I ran
I inspected the exact PR source and test sequence, the existing review thread, and the no-build precheck for this head. git diff --check, conventions/template validation, and the current-base merge preview passed. Hosted Windows passed; Linux and specialized product jobs were skipped. Environment: Linux x86_64, Python 3.11.2, GCC 12.2.0. I did not build or run tests locally.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13985
gh pr diff 13985 -R boykopovar/AnyPS5
gh pr checks 13985 -R boykopovar/AnyPS5Findings
Blocking
The debt entry says sceNpAuthPollAsync and sceNpAuthWaitAsync return ORBIS_NP_AUTH_ERROR_INVALID_ARGUMENT on null or unwritable result pointers. For a processed async request’s first non-null PollAsync call, the implementation sets polled and returns NP_POLL_ASYNC_RUNNING before calling GuestCopyOut_nid_no_patch; invalid request state also returns its request-state error before any copy. Narrow the debt statement to the paths that actually attempt to write the result, and record that the unmapped-pointer result is a host-safety fallback rather than a console measurement (poll implementation, debt entry).
Non-blocking
The test checks both bad-pointer errors but does not retry with a valid output pointer afterward. A follow-up sceNpAuthWaitAsync assertion would verify that a failed copy leaves the result available, as the current implementation appears to do.
Not verified
I did not build or run the focused test locally. The Windows CI job passed; the Linux and specialized product jobs were skipped.
To get this to a good state
Narrow the debt entry to the result-copy paths and distinguish the host pointer-safety fallback from measured console behavior. I will rerun the same relevant checks on the updated head.
|
@oneandonlydean Thanks for the thorough review! All three requests have been addressed:
|
|
@MarcZX10 Thanks for the review! The findings you noted have been fully resolved in head
|
|
Hi @oneandonlydean, All review feedback across the recent pull requests (#13950, #13953, #13956, #13960, #13968, #13970, #13974, #13979, #13985) has been addressed with atomic commits, pre-verified against conventions (0 findings), and is passing 100% of CI checks. We have a number of older PRs that have been thoroughly reviewed, verified across platforms, and have been waiting for final maintainer verification and merge:
Whenever you have a moment, could you please take a look to verify and merge the approved ones? Thank you very much for your time and continued guidance! |
c81f0b7 to
c0c64df
Compare
|
Hi @bunbeans2-arch @jotitadev, If you have a moment to test on Windows or Linux, could you run the focused test for libSceNpAuth? ctest --test-dir build -R '^guest_np_auth' --output-on-failureWe recently updated |
|
A Windows run with GCC 16: Windows 11 Pro 10.0.22631, i9-9900K, MSYS2 UCRT64 GCC 16.1.0, Release, this PR (
Looks good on Windows, no more segfaults at login 😄👍 AI-assisted: yes (Claude Code) |
af6c6fd to
13758cb
Compare
|
Hi @MarcZX10, The request retry verification and mutex release before copying out the result were added, and @bunbeans2-arch kindly tested the suite on Windows 11 with GCC 16.1.0 (MSYS2 UCRT64, Release), confirming 5/5 passed with the patch and 5/5 crashed in negative control. The branch has also been cleanly rebased onto latest |
b61d98a to
f69e9c6
Compare
✅ AMD GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (4, not counted against this PR)Validation IDs already present on main (4 IDs in 468 tests)Skipped (6)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
2ddb4b1 to
6f0ce5f
Compare
…or unwritable pointers
…in async poll and wait
… async poll and wait
… async query failure
…n async poll and wait
6f0ce5f to
31733f5
Compare
Addressed at 31733f5: the debt entry now limits INVALID_ARGUMENT to non-null unmapped pointers as a host-safety fallback (console unmeasured) and says the first sceNpAuthPollAsync returns running status without reading the pointer; GuestNpAuth.cpp retries with a valid pointer after the failed calls and requires the same result.
What
sceNpAuthPollAsyncandsceNpAuthWaitAsyncinlibSceNpAuthdirectly dereference the caller'sresultoutput pointer without verifying memory write accessibility. If an unwritable or unmapped guest address is passed, the process halts on memory violation instead of returningORBIS_NP_AUTH_ERROR_INVALID_ARGUMENT(0x80550301).This PR:
resultinsceNpAuthPollAsyncandsceNpAuthWaitAsyncusingGuestCopyOut_nid_no_patch.ORBIS_NP_AUTH_ERROR_INVALID_ARGUMENT(0x80550301) if writing the request status to guest memory fails.core/libs/tests/GuestNpAuth.cppassertingORBIS_NP_AUTH_ERROR_INVALID_ARGUMENTon invalid output pointers in both poll and wait.docs/dev/debt/libSceNpAuth.md.Closes #13980
Tested
ctest --test-dir build -R GuestNpAuth -Vpasses.python tools/check_conventions.py --base origin/mainreports 0 findings and 0 errors.Checklist
docs/dev/debt/libSceNpAuth.md.