patacongo commented on code in PR #9923:
URL: https://github.com/apache/nuttx/pull/9923#discussion_r1279522289
##########
sched/signal/sig_procmask.c:
##########
@@ -187,8 +187,27 @@ int nxsig_procmask(int how, FAR const sigset_t *set, FAR
sigset_t *oset)
int sigprocmask(int how, FAR const sigset_t *set, FAR sigset_t *oset)
{
+ sigset_t nset;
int ret;
+ /* SIGKILL and SIGSTOP should not be added to signal mask */
+
+ if (set != NULL)
+ {
+ nset = *set;
+ if (nxsig_ismember(&nset, SIGKILL))
+ {
+ nxsig_delset(&nset, SIGKILL);
+ }
+
+ if (nxsig_ismember(&nset, SIGSTOP))
+ {
+ nxsig_delset(&nset, SIGSTOP);
+ }
+
Review Comment:
> Where the new sigprocmask is set? Should I change nxsig_procmask()? I find
some functions directly call nxsig_procmask() rather than sig_procmask() or
pthread_procmask(). But ... we shouldn't limit kernel function to handle
sigstop/sigkill, right?
Many of my comments are things to think about and talk about. Not
necessarily requirements for merging.
For sigprocmask(), the sigprocmaks mask in the TCB and is modified in
nxsig_procmask(). This is like:
121 case SIG_BLOCK:
122 sigorset(&rtcb->sigprocmask, &rtcb->sigprocmask, set);
123 break;
124
129 case SIG_UNBLOCK:
130 nxsig_nandset(&rtcb->sigprocmask, &rtcb->sigprocmask,
set);
131 break;
132
135 case SIG_SETMASK:
136 rtcb->sigprocmask = *set;
137
This seems wrong to me, however. The sigprocmask is a global mask, not a
per-thread mask! When the sigprocmask is set, shouldn't signals sent to the
entire task group be blocked? Not just for a single thread. This needs some
thought.
The primary purpose of the per-thread sigprocmask is controlling which
threads can handle which signals. Disabling a signal in the per-thread
sigprocmask should only effect that thread; other threads should still be able
to handle the signal.
Disabling a signal in the global sigprocmask should disable the receipt of
the signal by all threads in the task group. [UPDATE: The behavior of
sigprocmask() is actually undefined in a multi-thread process (aka task group)].
For pthread_sigmask(), the sigprocmask is also currently set in
nxsig_procmask(). This is correct, but means that pthread_sigprocmask() and
sigprocmask() are identical. That can't be right.
I submitted this as Issue #9978
The behavior of sigprocmask() is actually undefined in a multi-threaded
process:
References:
https://pubs.opengroup.org/onlinepubs/009695299/functions/pthread_sigmask.html
> The pthread_sigmask() function shall examine or change (or both) the
calling thread's signal mask, regardless of the number of threads in the
process. The function shall be equivalent to sigprocmask(), without the
restriction that the call be made in a single-threaded process.
https://pubs.opengroup.org/onlinepubs/7908799/xsh/sigprocmask.html
> In a single-threaded process, the sigprocmask() function allows the
calling process to examine or change (or both) the signal mask of the calling
thread.
> The use of the sigprocmask() function is unspecified in a multi-threaded
process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
