Repository navigation
fix(libkernel): return EFAULT from pthread_getschedparam and pthread_setschedparam for invalid memory - #13506
pushkarreddyy wants to merge 4 commits into
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The guest-pointer fix is consistent with the surrounding checked-copy code, but this PR also removes an unrelated sanitizer timing-test safeguard from the workflow.
What I ran
Reviewed head 3bc8cb8c90f88f35653f88ab4012f69c8c81a492 against base a026562629da7bcc7af781639a11ab0647a524dd; the repository precheck reports its dependency and mergeability preview passed. Environment: Linux x86_64, Python 3.11.2, Git 2.39.5. No compiler or C++ test binary was run.
git diff --check a026562629da7bcc7af781639a11ab0647a524dd 3bc8cb8c90f88f35653f88ab4012f69c8c81a492
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13506
python3 tools/check_conventions.py --base a026562629da7bcc7af781639a11ab0647a524dd --head 3bc8cb8c90f88f35653f88ab4012f69c8c81a492 --title 'fix(libkernel): return EFAULT from pthread_getschedparam and pthread_setschedparam for invalid memory' --body /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13506-3bc8cb8c90f8/pr-body.md
python3 tools/pr_stack_check.py 13506 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13506-3bc8cb8c90f8/stack
python3 tools/pr_abi_audit.py --repo /marc/AnyPS5 --base a026562629da7bcc7af781639a11ab0647a524dd --head 3bc8cb8c90f88f35653f88ab4012f69c8c81a492
python3 tools/pr_test_quality.py --root /tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13506-3bc8cb8c90f8/head-3bc8cb8c90f8-ed67bed4 --json core/libs/tests/GuestSchedParams.cpp
gh pr checks 13506 --repo boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Diff and conventions | Passed. |
| PR precheck | Diff, conventions, and live dependency/merge checks passed. Its isolated copy could not find the two local-only audit helpers; I reran both from this workspace, with results below. |
| Dependency/mergeability | Precheck passed; the exact head previews cleanly against the PR base. |
| Export/ABI audit | Passed with no findings; lexical audit only, not a binary ABI check. |
| Test-quality heuristic | Flagged GuestSchedParams.cpp as possibly unregistered, a false positive: the exact-head core/libs/prx/libkernel/CMakeLists.txt registers it as guest_sched_params. |
| C++ test registration | Source inspection confirmed CMake registers guest_sched_params; the test was not run. |
| Hosted CI | At final refresh, all six checks (conventions, intake, labels, eligibility, library-portability, report) were pending; none is a C++ test/build job. |
guest_sched_params runtime test |
Not run locally. The PR description reports 3/3 runs on Windows and Linux and a negative control on unmodified main; I did not independently reproduce these results or verify their exact tested SHA. |
Unedited output from local checks
git diff --check exited 0 and emitted no output.
All conventions checks passed.
The precheck’s helper lookup errors were:
/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13506-3bc8cb8c90f8/head-3bc8cb8c90f8-ed67bed4/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-13506-3bc8cb8c90f8/head-3bc8cb8c90f8-ed67bed4/tools/pr_test_quality.py': [Errno 2] No such file or directory
ABI audit: passed
JoinThread_nid_no_patch: NID=unverified declarations=1 definitions=1 aliases=0
cpuset_getaffinity_nid_postfix: NID=Pdgml4rbxYk declarations=1 definitions=1 aliases=0
cpuset_setaffinity_nid_postfix: NID=0Ma-LQjv6So declarations=1 definitions=1 aliases=0
pthread_cancel_nid_postfix: NID=0D4-FVvEikw declarations=1 definitions=1 aliases=0
pthread_create_name_np_nid_postfix: NID=Jmi+9w9u0E4 declarations=1 definitions=1 aliases=0
pthread_create_nid_postfix: NID=OxhIB8LB-PQ declarations=1 definitions=1 aliases=0
pthread_detach_nid_postfix: NID=+U1R4WtXvoc declarations=1 definitions=1 aliases=0
pthread_equal_nid_postfix: NID=7Xl257M4VNI declarations=1 definitions=1 aliases=0
pthread_exit_nid_postfix: NID=FJrT5LuUBAU declarations=1 definitions=1 aliases=0
pthread_getaffinity_np_nid_postfix: NID=Jb2uGFMr688 declarations=1 definitions=1 aliases=0
pthread_getconcurrency_nid_postfix: NID=s+QHU9RLHS4 declarations=1 definitions=1 aliases=0
pthread_getcpuclockid_nid_postfix: NID=nnoxZUHK+SA declarations=1 definitions=1 aliases=0
pthread_getname_np_nid_postfix: NID=9HzfhdtESio declarations=1 definitions=1 aliases=0
pthread_getprio_nid_postfix: NID=aO+WS2xWuo4 declarations=1 definitions=1 aliases=0
pthread_getschedparam_nid_postfix: NID=FIs3-UQT9sg declarations=1 definitions=1 aliases=0
pthread_getthreadid_np_nid_postfix: NID=3eqs37G74-s declarations=1 definitions=1 aliases=0
pthread_join_nid_postfix: NID=h9CcP3J0oVM declarations=1 definitions=1 aliases=0
pthread_rename_np_nid_postfix: NID=9vyP6Z7bqzc declarations=1 definitions=1 aliases=0
pthread_self_nid_postfix: NID=EotR8a3ASf4 declarations=1 definitions=1 aliases=0
pthread_set_name_np_nid_postfix: NID=oxMp8uPqa+U declarations=1 definitions=1 aliases=0
pthread_setaffinity_np_nid_postfix: NID=5KWrg7-ZqvE declarations=1 definitions=1 aliases=0
pthread_setcancelstate_nid_postfix: NID=lZzFeSxPl08 declarations=1 definitions=1 aliases=0
pthread_setcanceltype_nid_postfix: NID=2dEhvvjlq30 declarations=1 definitions=1 aliases=0
pthread_setconcurrency_nid_postfix: NID=fBG1gHx1RlI declarations=1 definitions=1 aliases=0
pthread_setprio_nid_postfix: NID=a2P9wYGeZvc declarations=1 definitions=1 aliases=0
pthread_setschedparam_nid_postfix: NID=Xs9hdiD7sAA declarations=1 definitions=1 aliases=0
pthread_testcancel_nid_postfix: NID=nYBrkGDqxh8 declarations=1 definitions=1 aliases=0
pthread_timedjoin_np_nid_postfix: NID=PkS44IGrDkM declarations=1 definitions=1 aliases=0
pthread_yield_nid_postfix: NID=B5GmVDKwpn0 declarations=1 definitions=1 aliases=0
rtprio_thread_nid_postfix: NID=2I2RV6LyNng declarations=1 definitions=1 aliases=0
scePthreadGetschedparam: NID=P41kTWUS3EI declarations=1 definitions=1 aliases=0
scePthreadSetschedparam: NID=oIRFTjoILbg declarations=1 definitions=1 aliases=0
sched_getparam_nid_postfix: NID=O6gKl8uvGyE declarations=1 definitions=1 aliases=0
sched_getscheduler_nid_postfix: NID=SD7oNCIQWvE declarations=1 definitions=1 aliases=0
sched_setparam_nid_postfix: NID=yawdym+zDvw declarations=1 definitions=1 aliases=0
sched_yield_nid_postfix: NID=6XG4B33N09g declarations=1 definitions=1 aliases=0
sleep_nid_postfix: NID=0wu33hunNdE declarations=1 definitions=1 aliases=0
limit: This lexical audit does not parse C++ fully, prove calling-convention or binary ABI correctness, verify runtime version tables, or replace compilation.
limit: An absent source declaration or explicit alias can be intentional for assembly exports, conditional platform code, or generated interfaces; inspect findings before treating them as defects.
{
"findings": [
{
"path": "core/libs/tests/GuestSchedParams.cpp",
"line": 1,
"kind": "possible-unregistered-cpp-test",
"message": "Changed C++ test source path was not found literally in CMake files; inspect registration, generated source lists, and workflow coverage."
}
],
"negative_control_candidates": [],
"limits": [
"Heuristics can miss weak assertions and can flag valid tests; inspect every finding.",
"A test that passes against base is not by itself proof that the PR test is inadequate.",
"Negative controls are only candidates until the test actually runs against base and fails by assertion rather than import/setup error."
]
}conventions pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41096655764/job/124204044816
eligibility pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41096655754/job/124204042897
intake pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41096655550/job/124204043949
labels pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41096655492/job/124204043822
library-portability pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41096655730/job/124204042353
report pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41096655720/job/124204042084
Findings
Blocking
-
The PR removes
APS5_SKIP_TIMING_TESTS: 1from the sanitizer workflow even though the PR is about invalid-pointer handling inpthread_*schedparam. That setting was added immediately beforehand specifically to skip the strictprecise_waittiming bound in the hosted sanitizer run. Removing it reopens an unrelated timing-sensitive CI check and violates the repository's one-topic-per-PR rule. Please restore the setting in this PR (or split a separately justified workflow change into its own PR).APS5_SKIP_TIMING_TESTS: 1
Non-blocking
The checked guest-memory copies and EFAULT paths match the existing sched_getparam/sched_setparam pattern. I found no separate source-level defect in the changed pthread_* code.
Sources
- One-topic-per-PR rule at the reviewed base
- Sanitizer timing safeguard added in commit
1ceceab40eece374967f44c7921a0b1dee37e68b - Timing-test skip behavior at the reviewed head
- Sanitizer workflow at the reviewed head
Not verified
I did not build or run guest_sched_params, test Linux/Windows runtime behavior, or test console behavior. The exact-head hosted checks shown above are policy/status workflows, not the relevant C++ runtime tests.
To get this to a good state
Restore APS5_SKIP_TIMING_TESTS: 1 so this PR contains only the guest-pointer fix. I will rerun the same checks against the updated head.
|
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.
|
|
@pushkarreddyy thanks for the fix. The behaviour is good: a bad pointer now returns One correction for the debt entry. Adding |
3bc8cb8 to
0127b44
Compare
|
@oneandonlydean Thank you for the correction! I have updated @MarcZX10 Thanks for the review. The branch had branched before |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The current diff resolves the earlier workflow-scope concern and corrects the debt wording; I found no blocking issue in this update.
Reviewed head 0127b4406f80e665f7b68946b88a96eb32e936fc against PR base 9d05c9a072c05869d5a6cf436297de73fe530909; the merge preview against current main (4570612f405aa1817312a7b6de5c00969fb1ce2f) is clean. On Linux x86_64, git diff --check, the PR convention/template check, dependency/merge preview, and lexical ABI audit passed; source inspection confirmed guest_sched_params is registered. Five hosted policy/status checks were queued at the final refresh. The precheck's isolated snapshot lacked the local-only test-quality helper, so that heuristic did not run.
I did not build or run guest_sched_params, independently reproduce the author-reported Windows/Linux runs or negative control, or test console behavior. The updated guest-pointer code and debt note are visible in the reviewed source and debt entry.
github-actions
Bot
added
fix error
0127b44 to
2384b7a
Compare
|
Hi @adriadam10, All assigned PRs have been cleanly rebased onto latest Whenever convenient, please take a look at this PR and the others in your queue:
Thank you! |
0039f83 to
7e923a2
Compare
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
I rechecked head 7e923a22c70868dba9542dd0427636add07d2099 against base 828ac0b2ae74aac181ba128fb59b08a1534f2041; the guest-pointer fix and registered regression cases remain intact, and the update adds the corresponding debt entry.
git diff --check, conventions, and the current-main merge preview pass. The lexical ABI audit passes, and CMake registers guest_sched_params. The precheck’s ABI and test-quality helpers were unavailable in its isolated copy; direct ABI audit passed, and the registration warning was a heuristic false positive. Hosted conventions, labels, eligibility, portability, and report checks pass; Windows is pending, while Linux and specialized jobs are skipped. I did not build or run CTest locally. The PR description reports 3/3 focused runs and an unmodified-main negative control, but does not identify the tested SHA.
github-actions
Bot
added
libkernel
docs
needs rebase
7e923a2 to
b21d1a6
Compare
b21d1a6 to
fdaba2c
Compare
|
A Windows run with GCC 16, at the author's request: Windows 11 Pro 10.0.22631, i9-9900K, MSYS2 UCRT64 GCC 16.1.0, Release, this PR (
Looks good on Windows 👍 AI-assisted: yes (Claude Code) |
ad08bed to
51c5671
Compare
|
Thank you @bunbeans2-arch for running and verifying the guest test suite on Windows 11 with MSYS2 GCC 16 (5/5 passed)! The branch has been cleanly rebased onto latest |
39640ac to
9f5b609
Compare
|
Hi @adriadam10, Friendly follow-up on your assigned PR queue. All 5 branches are cleanly rebased onto current
Whenever you have a moment to take a look, they are ready for merge. Thank you! |
d79fa1e to
3c85668
Compare
…setschedparam for invalid memory
…ram and pthread_setschedparam
…am memory validation contract
… user space fault
3c85668 to
3166253
Compare
What
Safely copy
policyandparaminpthread_getschedparamandparaminpthread_setschedparamusingGuestCopyOut_nid_no_patchandGuestCopyIn_nid_no_patchso that passing unwritable or unreadable guest pointers returnsEFAULT(14) instead of faulting the host process (as FreeBSD 11.4's libthr would in user space, with console behaviour not measured).sched_getparamandsched_setparamkeep their existingEFAULTbehaviour per FreeBSD.Fixes #13497
Tested
Windows 11 / Linux x86_64:
main, callingpthread_getschedparam_nid_postfixwith unwritable memory orpthread_setschedparam_nid_postfixwith unreadable memory (reinterpret_cast<const KernelSchedParam*>(0x10)) crashes deterministically withSIGSEGV. With this fix applied, both safely returnGUEST_EFAULT(14).guest_sched_paramspasses 3 of 3 runs.python tools/check_conventions.py --base origin/mainreports zero findings across all commits.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)