patacongo commented on code in PR #9923:
URL: https://github.com/apache/nuttx/pull/9923#discussion_r1277560626
##########
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:
The nxsig_delset() is rather expensive; nxsig_member not so much. The code
is fine here, but you might want to consider using a single nxsig_nandset. I
am not sure if that is an improvment or not.
##########
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:
In the event of an error in some other part of the signal handling logic, if
SIGKILL or SIGSTOP ever get set in the sigprocmask, they may never be cleared.
They can be cleared with SIG_SETMASK but that is not often used after
initialization.
Wouldn't it be safer and more robust to mask out SIGKILL and SIGSTOP _after_
the new sigprocmaks (or pthread sigprocmask) is set.
##########
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);
+ }
+
+ set = &nset;
+ }
+
/* Let nxsig_procmask do all of the work */
ret = nxsig_procmask(how, set, oset);
Review Comment:
NOTE: While a signal handler is running, all signals are blocked (as I
recall), including SIGKILL and SIGSTOP) but are restored when the signal
handler returns. This is a case where it is necessary to set even these
signals to avoid unhandled re-entrancy.
##########
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);
+ }
+
+ set = &nset;
+ }
+
/* Let nxsig_procmask do all of the work */
ret = nxsig_procmask(how, set, oset);
Review Comment:
Note that there are two different instances of sigprocmask: One common
sigprocmask in the task group structure, and one in each TCB. The latter are
used by pthread_kill(), pthread_sigprocmask(), etc.
Shouldn't the logic that removes SIGKILL and SIGSTOP also be in
pthread_sigprocmask()? The effect of sending SIGKILL (or SIGSTOP) via
kill(SIGKILL, pid) and pthread_kill(thread, SIGKILL) must be identical.
SIGKILL and SIGSTOP cannot be masked in either and both effect the whole
process/task group.
--
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]
