Re: [hackers] Re: [dwm][PATCH v2] Use sigaction(SA_NOCLDWAIT) for SIGCHLD handling

From: Hiltjo Posthuma <hiltjo_AT_codemadness.org>
Date: Sat, 28 Jan 2023 00:11:44 +0100

On Thu, Jan 26, 2023 at 07:44:08PM +0600, NRK wrote:
> On Wed, Jan 25, 2023 at 09:17:53PM +0100, Hiltjo Posthuma wrote:
> > Using the new patch it does not handle zombie/defunct processes anymore on this
> > machine. To reproduce it, in .xinitrc, on a dusty machine:
> >
> > sleep 10 &
> > exec dwm
>
> For ease of testing I wrote this dummy program. Both SIG_DFL and SIG_IGN
> works on my system (glibc 2.36 & linux 5.15.13).
>
> [/tmp]~> cat test.c
> #include <signal.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <sys/wait.h>
>
> int main(void)
> {
> const struct sigaction sc = {
> .sa_handler = SIG_DFL,
> .sa_flags = SA_RESTART | SA_NOCLDWAIT | SA_NOCLDSTOP,
> };
> if (sigaction(SIGCHLD, &sc, NULL) < 0)
> abort();
> while (waitpid(-1, NULL, WNOHANG) > 0);
> puts("hello there");
> (void)getchar();
> }
> [/tmp]~> cc -o test test.c
> [/tmp]~> cat test.sh
> #!/bin/sh
>
> sleep 4 &
> exec ./test
> [/tmp]~> ./test.sh
>
> And the `signal(3p)` version also works:
>
> int main(void)
> {
> if (signal(SIGCHLD, SIG_IGN) < 0)
> abort();
> while (waitpid(-1, NULL, WNOHANG) > 0);
> puts("hello there");
> (void)getchar();
> }
>
> > Also, maybe it is better to not call sigprocmask? Since now it blocks all
> > signals, including SIGTERM in setup().
>
> _AT_Chris: I'm looking at the POSIX manpage for sigaction and I don't think
> it can fail due to interrupt. Only `EINVAL` is specified:
> https://www.man7.org/linux/man-pages/man3/sigaction.3p.html#ERRORS
>
> So getting rid of the `sigprocmask` and manually testing for EINTR when
> reaping inherited zombies should be fine, I think.
>
> - NRK
>

Hi,

I've looked at it a bit more and tested on more machines: OpenBSD, Slackware 11
and Void Linux. I think the simplified below patch should be fine. We do not
need to waitpid() on child processes. It is handled already by using
sigaction().

We should not (briefly) block all signals (including SIGTERM!) on setup I think.

diff --git a/dwm.c b/dwm.c
index 03baf42..371bada 100644
--- a/dwm.c
+++ b/dwm.c
_AT_@ -205,7 +205,6 @@ static void setmfact(const Arg *arg);
 static void setup(void);
 static void seturgent(Client *c, int urg);
 static void showhide(Client *c);
-static void sigchld(int unused);
 static void spawn(const Arg *arg);
 static void tag(const Arg *arg);
 static void tagmon(const Arg *arg);
_AT_@ -1540,12 +1539,16 @@ setmfact(const Arg *arg)
 void
 setup(void)
 {
+ struct sigaction sa;
         int i;
         XSetWindowAttributes wa;
         Atom utf8string;
 
- /* clean up any zombies immediately */
- sigchld(0);
+ /* do not transform children into zombies when they terminate. */
+ sigemptyset(&sa.sa_mask);
+ sa.sa_flags = SA_NOCLDSTOP | SA_NOCLDWAIT | SA_RESTART;
+ sa.sa_handler = SIG_IGN;
+ sigaction(SIGCHLD, &sa, NULL);
 
         /* init screen */
         screen = DefaultScreen(dpy);
_AT_@ -1638,14 +1641,6 @@ showhide(Client *c)
         }
 }
 
-void
-sigchld(int unused)
-{
- if (signal(SIGCHLD, sigchld) == SIG_ERR)
- die("can't install SIGCHLD handler:");
- while (0 < waitpid(-1, NULL, WNOHANG));
-}
-
 void
 spawn(const Arg *arg)
 {

-- 
Kind regards,
Hiltjo
Received on Sat Jan 28 2023 - 00:11:44 CET

This archive was generated by hypermail 2.3.0 : Sat Jan 28 2023 - 00:12:36 CET