Handle SIGCHLD ourselves instead of through GLib

GLib is very careful not to "fetch" any children that it was not asked
to fetch. Thus, if awesome inherits some children, it does not reap them
and starts collecting zombies.

Thus, this commit makes awesome handle SIGCHLD directly: On SIGCHLD, a
byte is written to a pipe. The main loop reads from this pipe and
collects children via waitpid(-1). Unknown children cause a warning to
be printed. We might want to remove this warning if it turns out to be
annoying.

This commit adds 79 lines and removes 89 lines. Thus, this is a net
reduction in lines of codes. This is because previously, we gave the
list of still-running children that we know about to the next awesome
instance we a --reap command line argument. This was added so that
awesome does not get zombie children. However, this commit fixes this
problem and makes all the code for this 'feature' unnecessary.

Fixes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=886393
Signed-off-by: Uli Schlachter <psychon@znc.in>

awesome.c

@@ -67,6 +67,9 @@ static struct timeval last_wakeup;
 /** current limit for the main loop's runtime */
 static float main_loop_iteration_limit = 0.1;
 
+/** A pipe that is used to asynchronously handle SIGCHLD */
+static int sigchld_pipe[2];
+
 /** Call before exiting.
  */
 void
@@ -115,6 +118,9 @@ awesome_atexit(bool restart)
     /* Disconnect *after* closing lua */
     xcb_cursor_context_free(globalconf.cursor_ctx);
     xcb_disconnect(globalconf.connection);
+
+    close(sigchld_pipe[0]);
+    close(sigchld_pipe[1]);
 }
 
 /** Restore the client order after a restart */
@@ -448,6 +454,34 @@ signal_fatal(int signum)
     fatal("signal %d, dumping backtrace\n%s", signum, buf.s);
 }
 
+/* Signal handler for SIGCHLD. Causes reap_children() to be called. */
+static void
+signal_child(int signum)
+{
+    assert(signum == SIGCHLD);
+    int res = write(sigchld_pipe[1], " ", 1);
+    assert(res == 1);
+}
+
+/* There was a SIGCHLD signal. Read from sigchld_pipe and reap children. */
+static gboolean
+reap_children(GIOChannel *channel, GIOCondition condition, gpointer user_data)
+{
+    pid_t child;
+    int status;
+    char buffer[1024];
+    ssize_t result = read(sigchld_pipe[0], &buffer[0], sizeof(buffer));
+    if (result < 0)
+        fatal("Error reading from signal pipe: %s", strerror(errno));
+
+    while ((child = waitpid(-1, &status, WNOHANG)) > 0) {
+        spawn_child_exited(child, status);
+    }
+    if (child < 0 && errno != ECHILD)
+        warn("waitpid(-1) failed: %s", strerror(errno));
+    return TRUE;
+}
+
 /** Function to exit on some signals.
  * \param data currently unused
  */
@@ -462,7 +496,7 @@ void
 awesome_restart(void)
 {
     awesome_atexit(true);
-    execvp(awesome_argv[0], spawn_transform_commandline(awesome_argv));
+    execvp(awesome_argv[0], awesome_argv);
     fatal("execv() failed: %s", strerror(errno));
 }
 
@@ -577,7 +611,7 @@ main(int argc, char **argv)
             replace_wm = true;
             break;
           case '\1':
-            spawn_handle_reap(optarg);
+            /* Silently ignore --reap and its argument */
             break;
           default:
             exit_help(EXIT_FAILURE);
@@ -630,6 +664,16 @@ main(int argc, char **argv)
         }
     }
 
+    /* Setup pipe for SIGCHLD processing */
+    {
+        if (!g_unix_open_pipe(sigchld_pipe, FD_CLOEXEC, NULL))
+            fatal("Failed to create pipe");
+
+        GIOChannel *channel = g_io_channel_unix_new(sigchld_pipe[0]);
+        g_io_add_watch(channel, G_IO_IN, reap_children, NULL);
+        g_io_channel_unref(channel);
+    }
+
     /* register function for signals */
     g_unix_signal_add(SIGINT, exit_on_signal, NULL);
     g_unix_signal_add(SIGTERM, exit_on_signal, NULL);
@@ -644,6 +688,10 @@ main(int argc, char **argv)
     sigaction(SIGSEGV, &sa, 0);
     signal(SIGPIPE, SIG_IGN);
 
+    sa.sa_handler = signal_child;
+    sa.sa_flags = SA_NOCLDSTOP | SA_RESTART;
+    sigaction(SIGCHLD, &sa, 0);
+
     /* We have no clue where the input focus is right now */
     globalconf.focus.need_update = true;
 

spawn.c

@@ -62,12 +62,6 @@
 /** 20 seconds timeout */
 #define AWESOME_SPAWN_TIMEOUT 20.0
 
-static int
-compare_pids(const void *a, const void *b)
-{
-    return *(GPid *) a - *(GPid *)b;
-}
-
 /** Wrapper for unrefing startup sequence.
  */
 static inline void
@@ -81,9 +75,20 @@ DO_ARRAY(SnStartupSequence *, SnStartupSequence, a_sn_startup_sequence_unref)
 /** The array of startup sequence running */
 static SnStartupSequence_array_t sn_waits;
 
-DO_BARRAY(GPid, pid, DO_NOTHING, compare_pids)
+typedef struct {
+    GPid pid;
+    int exit_callback;
+} running_child_t;
 
-static pid_array_t running_children;
+static int
+compare_pids(const void *a, const void *b)
+{
+    return ((running_child_t *) a)->pid - ((running_child_t *) b)->pid;
+}
+
+DO_BARRAY(running_child_t, running_child, DO_NOTHING, compare_pids)
+
+static running_child_array_t running_children;
 
 /** Remove a SnStartupSequence pointer from an array and forget about it.
  * \param s The startup sequence to found, remove and unref.
@@ -264,20 +269,6 @@ spawn_start_notify(client_t *c, const char * startup_id)
     }
 }
 
-static void
-remove_running_child(GPid pid, gint status, gpointer user_data)
-{
-    (void) status;
-    (void) user_data;
-
-    GPid *pid_in_array = pid_array_lookup(&running_children, &pid);
-    if (pid_in_array != NULL) {
-        pid_array_remove(&running_children, pid_in_array);
-    } else {
-        warn("(Partially) unknown child %d exited?!", (int) pid);
-    }
-}
-
 /** Initialize program spawner.
  */
 void
@@ -291,61 +282,6 @@ spawn_init(void)
                                                   NULL, NULL);
 }
 
-void
-spawn_handle_reap(const char *arg)
-{
-    GPid pid = atoll(arg);
-    pid_array_insert(&running_children, pid);
-    g_child_watch_add((GPid) pid, remove_running_child, NULL);
-}
-
-/** Called right before exit, serialise state in case of a restart.
- */
-char * const *
-spawn_transform_commandline(char **argv)
-{
-    size_t offset = 0;
-    size_t length = 0;
-    while(argv[offset] != NULL)
-    {
-        if(A_STREQ(argv[offset], "--reap"))
-            offset += 2;
-        else {
-            length++;
-            offset++;
-        }
-    }
-
-    length += 2*running_children.len;
-
-    const char ** transformed = p_new(const char *, length+1);
-    size_t index = 0;
-    offset = 0;
-    while(argv[index + offset] != NULL)
-    {
-        if(A_STREQ(argv[index + offset], "--reap"))
-            offset += 2;
-        else {
-            transformed[index] = argv[index + offset];
-            index++;
-        }
-    }
-
-    foreach(pid, running_children)
-    {
-        buffer_t buffer;
-        buffer_init(&buffer);
-        buffer_addf(&buffer, "%d", (int) *pid);
-
-        transformed[index++] = "--reap";
-        transformed[index++] = buffer_detach(&buffer);
-        buffer_wipe(&buffer);
-    }
-    transformed[index++] = NULL;
-
-    return (char * const *) transformed;
-}
-
 static gboolean
 spawn_launchee_timeout(gpointer context)
 {
@@ -440,12 +376,20 @@ parse_command(lua_State *L, int idx, GError **error)
 }
 
 /** Callback for when a spawned process exits. */
-static void
-child_exit_callback(GPid pid, gint status, gpointer user_data)
+void
+spawn_child_exited(pid_t pid, int status)
 {
+    int exit_callback;
+    running_child_t needle = { .pid = pid };
     lua_State *L = globalconf_get_lua_State();
-    int exit_callback = GPOINTER_TO_INT(user_data);
-    remove_running_child(pid, status, user_data);
+
+    running_child_t *child = running_child_array_lookup(&running_children, &needle);
+    if (child == NULL) {
+        warn("Unknown child %d exited with status %d", (int) pid, status);
+        return;
+    }
+    exit_callback = child->exit_callback;
+    running_child_array_remove(&running_children, child);
 
     /* 'Decode' the exit status */
     if (WIFEXITED(status)) {
@@ -570,11 +514,10 @@ luaA_spawn(lua_State *L)
 
     if(flags & G_SPAWN_DO_NOT_REAP_CHILD)
     {
-        int exit_callback = LUA_REFNIL;
         /* Only do this down here to avoid leaks in case of errors */
-        luaA_registerfct(L, 6, &exit_callback);
-        pid_array_insert(&running_children, pid);
-        g_child_watch_add(pid, child_exit_callback, GINT_TO_POINTER(exit_callback));
+        running_child_t child = { .pid = pid, .exit_callback = LUA_REFNIL };
+        luaA_registerfct(L, 6, &child.exit_callback);
+        running_child_array_insert(&running_children, child);
     }
 
     /* push pid on stack */

spawn.h

@@ -27,10 +27,9 @@
 #include <lua.h>
 
 void spawn_init(void);
-void spawn_handle_reap(const char *);
-char * const * spawn_transform_commandline(char **);
 void spawn_start_notify(client_t *, const char *);
 int luaA_spawn(lua_State *);
+void spawn_child_exited(pid_t, int);
 
 #endif
 // vim: filetype=c:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:textwidth=80