Skip to content

fix(libkernel): return EINTR from pselect when its mask unblocks a pending signal - #10501

Merged
oneandonlydean merged 3 commits into
boykopovar:mainfrom
harisibrar1:fix/pselect-pending-signal-eintr
Oct 26, 2026
Merged

oneandonlydean merged 3 commits into
boykopovar:mainfrom
harisibrar1:fix/pselect-pending-signal-eintr

Conversation

@harisibrar1

@harisibrar1 harisibrar1 commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

What

pselect (merged in #9273) never returned EINTR. With the canonical pattern of blocking a signal, raising it and waiting with a mask that unblocks it:

sigprocmask(SIG_BLOCK, {SIGUSR1}); raise(SIGUSR1);
pselect(0, NULL, NULL, NULL, &{0, 300 ms}, &empty);

FreeBSD 11.4 runs the handler and returns -1/EINTR at once. Here the handler ran inside the mask swap (_sigprocmask delivers a pending signal that SIG_SETMASK unblocks), and select then waited the full 300 ms and returned 0. With a NULL timeout it never returned. This was raised in the review of #9273.

In FreeBSD's kern_pselect the mask is installed and kern_select sleeps with PCATCH. Before it sleeps, kern_select checks nd, the descriptors and the timeout, then scans once: a ready descriptor wins, a zero timeout returns 0, and only then does a pending signal end the sleep with EINTR (sys/kern/sys_generic.c, release/11.4.0). On EINTR the descriptor sets are not copied out.

The change:

  • Signals.cpp: main's ChangeBlockedMask (from fix(libkernel): block a signal while its handler runs and honour SA_RESETHAND #9499) gains an optional bool* caught, and DeliverUnblockedPending returns whether a guest handler ran. SetSignalMaskCatching_nid_no_patch uses it with SIG_SETMASK, releasing holds like the sigprocmask call it replaces. _sigprocmask itself behaves as before.
  • SetSignalMaskCatching_nid_no_patch is declared once, in prx/libkernel/include/Api.hpp, which both Signals.cpp and Pselect.cpp include.
  • Pselect.cpp: when the swap ran a handler, pselect scans the descriptors once with a zero timeout instead of waiting. A ready descriptor, a zero timeout or an invalid argument (nfds, timeout) keep select's answer. Otherwise the call returns -1/EINTR and leaves the descriptor sets as they were.
  • The pselect debt entry is rewritten. The old text described a narrow race; the remaining gap is that a signal raised during the wait (another thread's pthread_kill, or raise from a handler) still doesn't interrupt it, because select has no EINTR path. It also notes that the mask is the process-wide one.

Tested

Windows 11, WinLibs GCC 15.2.0 r7 (UCRT), Release. Full build of 7784604 (current main 47f9cee merged in): 0 failed steps.

  • guest_kernel_sys_pselect passes 3 out of 3 on 7784604. The new cases cover: the pending-signal idiom returns EINTR in under 150 ms with the handler run once, the descriptor set unchanged and the mask restored; a NULL timeout returns EINTR instead of blocking; a ready descriptor still wins (returns 1); a zero timeout still returns 0; an invalid timeout still returns EINVAL; and a mask that keeps the signal blocked still waits the full timeout, with the handler running only when the signal is later unblocked.
  • Negative control: with main's Pselect.cpp and Signals.cpp, the test fails at the first EINTR check (line 129).
  • The related signal, timer and select tests pass (11 of 11, 3 runs), including guest_kernel_sys, guest_signals, guest_sigtimedwait, guest_getitimer, guest_itimer and guest_itimer_pending_default.
  • tools/check_conventions.py --base origin/main passes.
  • Not tested: Linux and macOS.

Overlap

Checklist

🤖 Generated with Claude Code

…nding signal

FreeBSD's kern_pselect installs the mask and kern_select sleeps with
PCATCH, so a pending signal that the new mask unblocks makes the call
return EINTR once the descriptors have been scanned. Here the handler ran
inside the mask swap and select then waited the full timeout, or forever
with no timeout. The swap now reports whether a handler ran, and pselect
then scans the descriptors once without waiting: a ready descriptor, a
zero timeout or an invalid argument keep select's answer, and otherwise
the call fails with EINTR and leaves the descriptor sets unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

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.

Pull request Conflicting files (lines on main) Same exports Same new files
PR 11273 core/libs/prx/libkernel/System/src/Signals.cpp L543, L578

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.

Pull request Shared files not compared
PR 3986 core/libs/prx/libkernel/System/src/Signals.cpp
PR 4630 docs/dev/debt/libkernel.md
PR 5021 docs/dev/debt/libkernel.md
PR 6149 docs/dev/debt/libkernel.md
PR 6740 docs/dev/debt/libkernel.md
PR 6830 docs/dev/debt/libkernel.md
PR 6936 docs/dev/debt/libkernel.md
PR 7088 docs/dev/debt/libkernel.md
PR 7105 docs/dev/debt/libkernel.md
PR 7121 docs/dev/debt/libkernel.md
PR 7149 docs/dev/debt/libkernel.md
PR 7192 docs/dev/debt/libkernel.md
PR 7201 core/libs/prx/libkernel/System/src/Signals.cpp
PR 7447 docs/dev/debt/libkernel.md
PR 7509 docs/dev/debt/libkernel.md
PR 7804 docs/dev/debt/libkernel.md
PR 8010 core/libs/prx/libkernel/include/Api.hpp
docs/dev/debt/libkernel.md
PR 8605 docs/dev/debt/libkernel.md
PR 9156 docs/dev/debt/libkernel.md
PR 9199 docs/dev/debt/libkernel.md
PR 9242 docs/dev/debt/libkernel.md
PR 9284 docs/dev/debt/libkernel.md
PR 9293 docs/dev/debt/libkernel.md
PR 9297 docs/dev/debt/libkernel.md
PR 9302 docs/dev/debt/libkernel.md
PR 9389 core/libs/prx/libkernel/System/src/Signals.cpp
docs/dev/debt/libkernel.md
PR 9509 docs/dev/debt/libkernel.md
PR 9557 docs/dev/debt/libkernel.md
PR 9566 docs/dev/debt/libkernel.md
PR 9575 docs/dev/debt/libkernel.md
PR 9669 docs/dev/debt/libkernel.md
PR 9693 core/libs/prx/libkernel/include/Api.hpp
docs/dev/debt/libkernel.md
PR 10061 docs/dev/debt/libkernel.md
PR 10319 docs/dev/debt/libkernel.md
PR 10442 docs/dev/debt/libkernel.md
PR 10621 docs/dev/debt/libkernel.md
PR 10666 docs/dev/debt/libkernel.md
PR 10898 docs/dev/debt/libkernel.md
PR 11002 core/libs/prx/libkernel/System/src/Signals.cpp
docs/dev/debt/libkernel.md
PR 11047 docs/dev/debt/libkernel.md

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pselect change is narrowly scoped and the negative control plus ready-descriptor, timeout, mask-restore, and signal-blocking cases are useful. The author reports the related Windows tests passing on an earlier base with an identical patch-id; current hosted PR build jobs are skipped, and I did not rerun the test suite locally.

Please resolve two merge-order declarations before approval. The body explicitly says #9499 remains open, changes the same Signals.cpp helper, and that #9499 should land first; however the checklist says Depends on: none. The body also says open #10310 changes the pselect debt text and needs a text merge, but no order is declared for that overlap. Please coordinate with those authors and list the agreed order(s) in the checklist, following CONTRIBUTING. The current head is mergeable with main, so I found no separate rebase blocker or code defect in this pass.

@harisibrar1

Copy link
Copy Markdown
Contributor Author

@MarcZX10 Thanks. I've updated the checklist with the order. @EagleBearerYS, @orezraey, could you confirm it works for you?

Reviewed with AI assistance (Claude Code).

@EagleBearerYS

Copy link
Copy Markdown
Contributor

@harisibrar1 that works for us: #9499 first, then #10501 on its ChangeBlockedMask. Thanks for taking the rebase.

@Dom3005

Dom3005 commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

Linux run (Ubuntu 26.04 container, GCC 15, glibc 2.43, CMake + Ninja, Release, -DBUILD_TESTING=ON, ctest --repeat until-fail:3)

  • c7cd45c0d merged onto main 39a36bbec: guest_kernel_sys_pselect, guest_kernel_sys, guest_signals, guest_sigtimedwait, guest_getitimer, guest_itimer and guest_itimer_pending_default pass, 7 of 7, 3 runs each.
  • Control, main 39a36bbec with only this PR's GuestKernelSysPselect.cpp: guest_kernel_sys_pselect fails with check failed at line 129, the same first EINTR check as on Windows.
  • main d53b52be7 on its own: the same seven tests pass.

This covers the Linux part of "Not tested: Linux and macOS".

@MarcZX10

Copy link
Copy Markdown
Contributor

Thanks, the #9499 author has now confirmed the proposed order, and the Linux follow-up supplies the missing platform run. The remaining coordination item is #10310: the checklist says its debt-text overlap can land in either order, but I do not see confirmation from its author yet. Please record that agreement before merge; the prior review request remains open until then.

@orezraey

Copy link
Copy Markdown
Contributor

@harisibrar1 Either order works for #10310. It only rewrites the select entry in docs/dev/debt/libkernel.md, so whichever of the two lands second will keep both the select sentence and this PR's pselect entry; if #10310 is second I will refresh it after this one merges.

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head c7cd45c. The previously missing merge-order agreements are now explicit: #9499 confirmed it lands first, and #10310 confirmed either order for the adjacent debt text; the checklist records both. The implementation and regression coverage are unchanged from my prior review. The author reports the Windows focused signal suite and the Linux follow-up reports seven related tests passing three consecutive runs, including a negative control where main fails the new EINTR case. I did not reproduce those runs locally. Hosted structural checks pass; product build jobs are skipped by eligibility. No remaining blocker found.

@harisibrar1

Copy link
Copy Markdown
Contributor Author

Merged main e7e3e9c into the branch (head b8b4214) to resolve the conflict from the libkernel header refactor (#10706):

  • Pselect.cpp now includes prx/libkernel/include/Api.hpp and prx/libc/include/Api.hpp, like main's version. It keeps only a local declaration of the internal SetSignalMaskCatching_nid_no_patch, typed with GuestSignalSet to match its definition in Signals.cpp.
  • GuestKernelSysPselect.cpp drops its local extern "C" block in favour of Api.hpp, and uses main's GuestHandler.

The behaviour is unchanged. Rerun on Windows 11, WinLibs GCC 15.2.0 r7, full Release build: no failed steps. guest_kernel_sys_pselect, the signal and timer tests (guest_signals, guest_sigtimedwait, guest_getitimer, guest_itimer, guest_itimer_pending_default) and the select tests pass, 11/11 in 3 runs. With main's Pselect.cpp and Signals.cpp, guest_kernel_sys_pselect still fails. check_conventions.py --base origin/main passes. #9499 is still open, so the agreed order stands.

Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 25, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

@harisibrar1 Thanks for this. One change before it can go in: the new APS5_VABI SetSignalMaskCatching_nid_no_patch bridge is only declared locally in Pselect.cpp. Please move that declaration into the shared libkernel header that the umbrella reaches, and include it from both translation units, so the definition and its caller can't drift apart. That's the repo's one-declaration-per-export rule. Please keep the agreed order with #9499 (it lands first), and keep the pending-signal, descriptor-priority and mask-restoration tests.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 25, 2026
…ignal-eintr

# Conflicts:
#	core/libs/prx/libkernel/System/src/Signals.cpp
@harisibrar1

Copy link
Copy Markdown
Contributor Author

@oneandonlydean done in 7784604b0b, which also merges current main (47f9cee):

  • SetSignalMaskCatching_nid_no_patch is now declared once, in prx/libkernel/include/Api.hpp next to _sigprocmask. The libkernel.hpp umbrella includes that header, and so do both Signals.cpp and Pselect.cpp. The local declaration in Pselect.cpp is gone.
  • Main had replaced the mask helper with ChangeBlockedMask (handler holds). Instead of keeping a second copy, the bridge now calls main's helper: it gained an optional bool* caught, and DeliverUnblockedPending returns whether a handler ran. The bridge releases holds like the sigprocmask(SIG_SETMASK) call it replaces.
  • The pending-signal, descriptor-priority and mask-restoration tests are unchanged, and fix(libkernel): block a signal while its handler runs and honour SA_RESETHAND #9499 stays first.

Windows 11, GCC 15.2.0: full build clean, and the 11 signal/pselect/select tests pass in 3 runs. With main's Pselect.cpp and Signals.cpp, guest_kernel_sys_pselect fails (2 runs). check_conventions passes. Could you take another look?

Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 26, 2026
@harisibrar1

Copy link
Copy Markdown
Contributor Author

Overlap note: #11273 (@Gruzzberry, EFAULT for unusable signal pointers) also adds a trailing optional parameter to ChangeBlockedMask (GuestSignalSet* guestPrevious), so the two conflict on its signature line and on the DeliverUnblockedPending line. Whichever lands second keeps both parameters; if #11273 lands first I'll merge main here and do that. Details on #11273.

Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.

@oneandonlydean
oneandonlydean merged commit a0c85ea into boykopovar:main Oct 26, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants