Repository navigation
fix(libkernel): block a signal while its handler runs and honour SA_RESETHAND - #5312
EagleBearerYS wants to merge 5 commits into
Conversation
…ESETHAND A guest signal handler ran with the mask it was raised under, so a handler that raised its own signal again re-entered itself at once, sigprocmask inside the handler did not report the signal or the handler's sa_mask as blocked, and SA_RESETHAND left the handler installed for the next delivery. FreeBSD 11.4 forms the handler's frame and then, in postsig_done() (sys/kern/kern_sig.c), blocks the handler's sa_mask and, unless SA_NODEFER is set, the signal itself, and resets the disposition to SIG_DFL when SA_RESETHAND is set; sigreturn restores the previous mask, so a signal raised meanwhile stays pending until the handler returns. sigaction keeps reporting SA_RESETHAND afterwards, since sigdflt() does not clear ps_sigreset. The handler now runs with its signal (unless SA_NODEFER) and its mask added to the guest mask, which is restored when it returns, and pending signals that the restored mask unblocks are delivered when raise returns. SA_RESETHAND resets the handler to the default before it runs.
The sigaction entry said that the mask and flags of a running handler are not applied. It now says what is: the signal itself unless SA_NODEFER, and the first 32 signals of sa_mask, are added to the process-wide mask while the handler runs, and SA_RESETHAND resets the disposition first. It also notes what is not: SA_ONSTACK, and the delivery of a signal left pending by a handler that a host signal ran asynchronously, which waits for the next raise or _sigprocmask.
CnRJay
left a comment
There was a problem hiding this comment.
Blocking the signal and sa_mask during the handler matches postsig_done. One interaction this introduces: a handler that leaves by longjmp now leaves its signal blocked for good.
RunHandler restores the mask after handler(guest) returns. A handler that jumps out never returns there, and leaving a SIGSEGV/SIGBUS/SIGFPE handler through a jump is the classic pattern for guarded probes and crash recovery. What happens next depends on which jump the guest uses:
siglongjmpwith asigsetjmp(env, 1)buffer restores the saved mask through_sigprocmask(SIG_SETMASK)(SignalSets.cpp), so that case works.longjmp(JumpBuffer.cppin libc) doesn't touch the mask. On FreeBSD,setjmp/longjmpdo save and restore the signal mask (BSD semantics, unlike_setjmp/_longjmp). Onmainthat gap was invisible, because nothing was blocked during a handler. With this PR, a guest that leaves a handler withlongjmpkeeps that signal blocked, and the next fault of that kind is never delivered. Instead it stays pending, or the host kills the process on a synchronous fault it can't deliver.
So either setjmp/longjmp in libc should save and restore the mask as FreeBSD's do (the same _sigprocmask calls sigsetjmp(env, 1) makes), or this PR should land together with that change. A test where a handler leaves with longjmp and the same signal is then raised again would pin it.
Smaller: blockedMask.store(saved) after the handler overwrites any change another thread made to the process-wide mask while the handler ran. The description notes that #3986 moves to per-thread masks. Until then, fetch_and(~added) with only the bits this handler added (mask & ~saved) would avoid clobbering concurrent changes.
The blocked mask is still process-wide, so restoring the whole mask saved before the handler undid any change another thread made while it ran.
FreeBSD's setjmp saves the signal mask at 72 in the jump buffer and its longjmp restores it, unlike _setjmp and _longjmp. Ours kept only the registers, so a longjmp back from code that changed the mask, such as a signal handler, left the mask as it was. libc can't call libkernel, so libkernel registers its sigprocmask with libc when it loads.
A handler that leaves with longjmp never returns to unblock its signal, so longjmp has to bring back the mask setjmp saved before the signal.
|
Thanks, both points hold, and both are fixed now.
On GitHub's runners (run), the new |
|
Closing: no title is known to reach this. With the queue at 700+ open PRs, please hold off on new PRs that don't name a title needing the change until it's down. If a title stops on this on current main, reopen with the title ID and the log line. |
What
A guest signal handler ran with the mask its signal was raised under.
DispatchandDeliverPendingSignalinSignals.cppcalled it without blocking anything, so:sigprocmaskinside a handler did not report the signal or the handler'ssa_maskas blocked;SA_RESETHANDleft the handler installed for the next delivery.FreeBSD sets up the handler's frame and then, in
postsig_done, blockssa_maskand, unlessSA_NODEFERis set, the signal itself. It also resets the disposition toSIG_DFLwhenSA_RESETHANDis set (sigdflt), andsigreturnrestores the previous mask.sigactionkeeps reportingSA_RESETHANDafterwards (L707-L708), becausesigdfltdoes not clearps_sigreset, whichkern_sigactionsets from the flag.The handler now runs with its signal (unless
SA_NODEFER) and the first 32 signals of itssa_maskadded to the guest mask. When it returns, only the signals it added are unblocked again, so a change another thread makes to the mask meanwhile is kept. Pending signals that this unblocks are delivered whenraisereturns, as_sigprocmaskalready does.SA_RESETHANDresets the handler to the default before it runs.A handler that leaves with
longjmpnever returns there, so its signal would stay blocked. FreeBSD'ssetjmpsaves the signal mask in the jump buffer and itslongjmprestores it (setjmp.S), but ours kept only the registers. libc'ssetjmpnow saves the mask at 72 in the jump buffer, with the flagsigsetjmpkeeps at 88, andlongjmprestores it. libc doesn't link libkernel, so libkernel registers itssigprocmaskwith libc when it loads. Both changes follow CnRJay's review.The mask is still
main's process-wide 32-bit mask. #3986 makes it per thread; with that, the handler's mask belongs to the thread the signal is delivered on, so whichever of the two lands second adapts. The commits:SA_RESETHAND, withguest_signal_handler_mask;sigactionentry indocs/dev/debt/libkernel.md;setjmpandlongjmpsaving and restoring the mask, with adocs/dev/debt/libc.mdentry;longjmpand raises its signal again.No title is claimed.
Tested
On GitHub's runners from my fork, on
mainat 0214069 (run):-DANYPS5_ENABLE_SPIRV_TOOLS=ON.On both:
guest_signal_handler_mask. A handler raises its own signal, both onSIGUSR2, which has no host signal, and onSIGTERM, which goes through the host. The test also checksSA_NODEFER, the masksigprocmaskreports inside the handler, andSA_RESETHAND. Withmain'sSignals.cppit aborts at its first check, where the nestedraisere-enters the handler. With the change it passes 20 runs in a row.core/libs/prx/libcandcore/libs/prx/libkernelpasses: 69 and 124 on Linux, 90 and 127 on Windows.tools/check_conventions.py --base origin/mainpasses for both commits.After the review, on the same runners:
guest_signal_handler_maskgains a check where another thread blocksSIGTERMwhile the handler runs. It aborts with the first two commits and passes 20 runs in a row with the third.guest_signal_setsgainssetjmp/longjmpchecks (the mask comes back, saved at 72 with the flag at 88), andguest_signal_handler_maskleaves a handler withlongjmpand raises the signal again. Both abort with the first three commits and pass 20 runs in a row with all five.core/libs/prx/libcandcore/libs/prx/libkernelpasses: 69 and 124 on Linux, 90 and 127 on Windows.tools/check_conventions.pypasses for all five 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)