Repository navigation
fix(libkernel): reject ended threads in pthread_attr_get_np - #9482
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed the lifetime guard and failure-path tests. The function validates null arguments first, holds LockLiveGuestThread for the full attribute copy, and returns POSIX ESRCH when the registry no longer contains a live thread; this prevents detached-thread cleanup from racing the read. The test covers joined, finished-unjoined and detached threads, self-query from a thread-specific-data destructor, null inputs, unchanged destination attributes and unchanged guest errno. The author reports the focused pthread group passing 27/27 and 25 repeat runs, with full-suite failures reproduced on unchanged main and unrelated to this change. I compared the combined PR tree with #8041: the guard-size assignment and liveness check compose cleanly in the shared query, and both PRs’ separate focused tests remain present. Conventions, intake, eligibility, labels and report checks pass; hosted product jobs were skipped.
|
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.
|
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Linux run of the current change, since the description lists only Windows ("No console, title, Linux or macOS testing").
Machine: Linux x86_64, i7-8700 (6 cores / 12 threads), Ubuntu 26.04, GCC 15.2, Release, BUILD_TESTING=ON. Head 892138ed5 merged onto main a6dcea927 (the merge touches only Attr.cpp, GuestPthreadAttr.cpp and the debt doc).
- Build ok.
guest_pthread_attrpasses with the change (joined, finished-but-unjoined and detached threads, null arguments, preserved errno and the self-query cases). - Restoring
libkernel/Pthread/Posix/Attr.cpptomainand keeping the test abortsguest_pthread_attr: onmainpthread_attr_get_npreturns 0 for an ended thread where the test expects guestESRCH(3). So the regression reproduces on Linux as well and the test covers the change.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Rechecked the lifetime guard and latest follow-up test report. LockLiveGuestThread remains held for the full attribute copy, returns POSIX ESRCH for ended threads, and the new tests preserve validation order, guest errno and output attributes on failure. The author reports the Windows 27-test pthread group and 25 repeats; a later reviewer also reports a Linux build/test pass and a main-runtime negative control that reproduces the bug. Full-suite GPU failures are reported as reproducible on main and unrelated. Hosted policy checks pass; product jobs are skipped. I did not run local tests.
|
Linux data point for
|
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The lifetime fix and tests are well covered: LockLiveGuestThread stays held for the attribute read, ended threads return ESRCH, and tests include joined, finished-unjoined, detached, TSD-destructor, null-input and output/errno preservation cases. The Windows focused pthread group/repeats and WSL red-green reports are detailed; unrelated GPU failures are clearly distinguished and reproduced on the main baseline. The branch/test base is a6dcea9, while current upstream main is ab347d1. Please rebase onto current main, rerun the focused guest_pthread_attr group and conventions/overlap checks, preserving the #6044 test relocation and unrelated debt edits. Hosted product jobs are skipped; I did not run local tests.
892138e to
6298429
Compare
|
@MarcZX10 Addressed the rebase/retest request: rebased onto Windows 11 x86-64 / WinLibs GCC 15.2.0 r7 / Release /
Manual overlap verification: #6044's combined merge-tree places this test at Updated the description to separate these new focused results from the historical full-suite run. Full suite, title execution, Linux and macOS were not rerun by me on the rebased head. Working tree clean. Ready for another review; hosted product builds still require a maintainer's manual run. AI-assisted execution and report. |
6298429 to
271d428
Compare
|
@MarcZX10 Follow-up audit refreshed this once more to Rechecked #6044: the relocated test remains byte-identical; unrelated conflicts in that PR are not fixed here. #9573's created-suspended field copy composes with the lifetime guard. The #9684 author has now agreed to this PR first and to preserving both adjacent debt updates on their rebase: #9684 (comment). Description updated accordingly; no runtime dependency. The rebase/retest review request is addressed; awaiting re-review. Full suite and Linux/macOS/title testing were not rerun on this head. AI-assisted. |
271d428 to
37ba4a9
Compare
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.
|
Rebased onto current Checked: |
What
Fix the documented ended-thread gap in
pthread_attr_get_np: acquire the existingLockLiveGuestThreadguard before reading the thread, and return guestESRCH(3) when it is no longer live. Holding the registry lock through the attribute copy prevents detached-thread destruction from freeing the object during the read. The SCE sibling already uses this guard; validation order and live-thread attribute values are unchanged.Extend
GuestPthreadAttr.cppto cover joined, finished-but-unjoined and detached threads, null arguments, unchanged stack-size attributes on failure, preserved guest errno, and successful self-queries during thread-specific-data destruction. Trim the resolved debt statement while retaining the unverified console behavior and Windows adopted-thread limitation.The behavior follows the FreeBSD 11.4 implementation already cited by the debt entry:
_pthread_attr_get_npcalls_thr_find_threadwith dead threads excluded. No new console measurement or title fix is claimed.Refreshed head
271d428b2d1e55d9f38fdaa6af4b92876e83817c, based on audit-start mainf57ceaad6bceeb00a7f4f09975f5932b2f20e28e. This supersedes the earlier rebase tob7d1d47a5f/head6298429beb;git range-diffconfirms the patch is unchanged. Unrelated upstream changes are preserved.2026-10-24 status audit (no code update): head remains
271d428b2d; it still merges cleanly with fetched main379b1dca32. No new implementation review request was posted. The earlier rebase/retest request was addressed in the comments below; re-review and maintainer-triggered runtime CI remain outstanding. No new execution result or unnecessary rebase is claimed.The current overlap report still has debt-only hunks with #9684 and #9938. The #9684 author already agreed this PR first. The #9938 author documents either order as possible, specifically requiring its new joined-thread query case to expect
ESRCHafter this lifetime fix. Keeping #9482 first is consistent with its existing #9684 ordering; there is no new runtime dependency. Their supplied-stack implementation is not imported here, and no combined runtime test is claimed.Previous audit's overlap review (historical integration observations):
core/libs/tests/threads/GuestPthreadAttr.cpp, byte-for-byte identical to this PR's test, without a conflict in that file. The relocation is not merged into this PR prematurely; test: organize library tests and label CTest suites #6044 still has unrelated pre-existing conflicts.Depends on: noneremains correct.Pthread/Posix/Attr.cpp; its debt edit concerns a separate exception entry. fix(libkernel): report the creation guard size from the thread attribute queries #8041 and feat(libkernel_sys): identify the initial pthread #9160 concern other attribute fields, not this guard.Tested
Windows 11 x86-64 (build 26300.9711), WinLibs GCC 15.2.0 r7, Release,
BUILD_TESTING=ON.Last executed validation of
271d428b2d, basef57ceaad6b(not a new run against 2026-10-24 main):guest_pthread*_testsexecutable targets, including current libkernel/libc. These register 27 pthread test cases.ctest --test-dir build -R '^guest_pthread' --output-on-failure --parallel 4: 27/27 passed.ctest --test-dir build -R '^guest_pthread_attr$' --repeat until-fail:25 --output-on-failure: 25 consecutive passes.git diff --exit-code main -- core/libs/prx, rebuilt, and reproducedpthread_attr_get_np returned 0, expected 3(CTest exit 8). Restored the guard, rebuilt and reran the entire pthread group: 27/27 passed. Working tree clean afterward.origin/main,git diff --check main...HEAD, and overlap checker pass, with the limitations and manual review above.Historical full-suite validation, old head
892138ed5c, basea6dcea9279(not results for this rebased head):ImageArrayWindowPastLayerLimit.cpp:33: alignment 65536 exceeded the Windows object-file maximum 8192. That source has since changed upstream; this is not a claim about its current build status.agc_driver_image_array_window_past_layer_limit. Failures wereagc_driver_sample_order,agc_driver_sample_order_grid_reference, andagc_driver_sample_order_recipe_depth_locations(depth/clear-value checks on the NVIDIA GeForce RTX 4060 Laptop GPU). All three reproduced with identical diagnostics after removing this fix and confirming runtime-library sources matched that main. No GPU code was changed.No author-performed console, title, Linux or macOS testing for this PR. Independent Linux and WSL reviewers report red/green checks on old head
892138ed5c; they are not retests of the latest head. Hosted product builds require a maintainer-triggered run.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)