feat(reaper): Add process reaper support (#7059)

##### Summary
Add a child process reaper to the main netdata app if running as init (pid = 1).

This prevents zombie processes when a child is re-parented to netdata when its running in a container.

Also:
* Few style cleanups to match surrounding code.

Fixes: #6033

##### Component Name
netdata binary

##### Additional Information
This re-purposes old commented out code in `popen.c`, which already implemented part of the required process tracking.

Without this on a standard netdata docker install we saw at least one zombie `timeout` process straight after the container was started.

8 files changed, 240 insertions, 62 deletions

diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md
index 2cfdee4e38..8adf3a31d3 100644
--- a/CONTRIBUTORS.md
+++ b/CONTRIBUTORS.md
@@ -129,5 +129,6 @@ This is the list of contributors that have signed this agreement:
 |@skrzyp1|Jerzy S.||
 |@akwan|Alan Kwan||
 |@underhood|Timotej Šiškovič||
+|@stevenh|Steven Hartland|steven.hartland@multiplay.co.uk|
 
 [![analytics](https://www.google-analytics.com/collect?v=1&aip=1&t=pageview&_s=1&ds=github&dr=https%3A%2F%2Fgithub.com%2Fnetdata%2Fnetdata&dl=https%3A%2F%2Fmy-netdata.io%2Fgithub%2FCONTRIBUTORS&_u=MAC~&cid=5792dfd7-8dc4-476b-af31-da2fdb9f93d2&tid=UA-64295674-3)](<>)
diff --git a/collectors/plugins.d/plugins_d.c b/collectors/plugins.d/plugins_d.c
index 5e25e98239..edbdb57302 100644
--- a/collectors/plugins.d/plugins_d.c
+++ b/collectors/plugins.d/plugins_d.c
@@ -647,7 +647,7 @@ static void pluginsd_worker_thread_cleanup(void *arg) {
         if (cd->pid) {
             siginfo_t info;
             info("killing child process pid %d", cd->pid);
-            if (killpid(cd->pid, SIGTERM) != -1) {
+            if (killpid(cd->pid) != -1) {
                 info("waiting for child process pid %d to exit...", cd->pid);
                 waitid(P_PID, (id_t) cd->pid, &info, WEXITED);
             }
@@ -738,7 +738,7 @@ void *pluginsd_worker_thread(void *arg) {
         info("connected to '%s' running on pid %d", cd->fullfilename, cd->pid);
         count = pluginsd_process(localhost, cd, fp, 0);
         error("'%s' (pid %d) disconnected after %zu successful data collections (ENDs).", cd->fullfilename, cd->pid, count);
-        killpid(cd->pid, SIGTERM);
+        killpid(cd->pid);
 
         int worker_ret_code = mypclose(fp, cd->pid);
 
diff --git a/collectors/tc.plugin/plugin_tc.c b/collectors/tc.plugin/plugin_tc.c
index 50383f4ce3..9245b0857f 100644
--- a/collectors/tc.plugin/plugin_tc.c
+++ b/collectors/tc.plugin/plugin_tc.c
@@ -851,12 +851,11 @@ static void tc_main_cleanup(void *ptr) {
 
     if(tc_child_pid) {
         info("TC: killing with SIGTERM tc-qos-helper process %d", tc_child_pid);
-        if(killpid(tc_child_pid, SIGTERM) != -1) {
+        if(killpid(tc_child_pid) != -1) {
             siginfo_t info;
 
             info("TC: waiting for tc plugin child process pid %d to exit...", tc_child_pid);
             waitid(P_PID, (id_t) tc_child_pid, &info, WEXITED);
-            // info("TC: finished tc plugin child process pid %d.", tc_child_pid);
         }
 
         tc_child_pid = 0;
diff --git a/daemon/main.c b/daemon/main.c
index 4189ac7bd6..0e56654db2 100644
--- a/daemon/main.c
+++ b/daemon/main.c
@@ -146,46 +146,28 @@ void web_server_config_options(void) {
 }
 
 
-int killpid(pid_t pid, int signal)
-{
-    int ret = -1;
+// killpid kills pid with SIGTERM.
+int killpid(pid_t pid) {
+    int ret;
     debug(D_EXIT, "Request to kill pid %d", pid);
 
     errno = 0;
-    if(kill(pid, 0) == -1) {
+    ret = kill(pid, SIGTERM);
+    if (ret == -1) {
         switch(errno) {
             case ESRCH:
-                error("Request to kill pid %d, but it is not running.", pid);
-                break;
+                // We wanted the process to exit so just let the caller handle.
+                return ret;
 
             case EPERM:
-                error("Request to kill pid %d, but I do not have enough permissions.", pid);
+                error("Cannot kill pid %d, but I do not have enough permissions.", pid);
                 break;
 
             default:
-                error("Request to kill pid %d, but I received an error.", pid);
+                error("Cannot kill pid %d, but I received an error.", pid);
                 break;
         }
     }
-    else {
-        errno = 0;
-        ret = kill(pid, signal);
-        if(ret == -1) {
-            switch(errno) {
-                case ESRCH:
-                    error("Cannot kill pid %d, but it is not running.", pid);
-                    break;
-
-                case EPERM:
-                    error("Cannot kill pid %d, but I do not have enough permissions.", pid);
-                    break;
-
-                default:
-                    error("Cannot kill pid %d, but I received an error.", pid);
-                    break;
-            }
-        }
-    }
 
     return ret;
 }
diff --git a/daemon/main.h b/daemon/main.h
index 687155981c..9d9f4ef0f5 100644
--- a/daemon/main.h
+++ b/daemon/main.h
@@ -41,7 +41,7 @@ struct netdata_static_thread {
 };
 
 extern void cancel_main_threads(void);
-extern int killpid(pid_t pid, int signal);
+extern int killpid(pid_t pid);
 extern void netdata_cleanup_and_exit(int ret) NORETURN;
 extern void send_statistics(const char *action, const char *action_result, const char *action_data);
 
diff --git a/daemon/signals.c b/daemon/signals.c
index 71f271887c..5378b04e5e 100644
--- a/daemon/signals.c
+++ b/daemon/signals.c
@@ -2,6 +2,8 @@
 
 #include "common.h"
 
+static int reaper_enabled = 0;
+
 typedef enum signal_action {
     NETDATA_SIGNAL_END_OF_LIST,
     NETDATA_SIGNAL_IGNORE,
@@ -10,6 +12,7 @@ typedef enum signal_action {
     NETDATA_SIGNAL_LOG_ROTATE,
     NETDATA_SIGNAL_RELOAD_HEALTH,
     NETDATA_SIGNAL_FATAL,
+    NETDATA_SIGNAL_CHILD,
 } SIGNAL_ACTION;
 
 static struct {
@@ -26,6 +29,7 @@ static struct {
         { SIGUSR1, "SIGUSR1", 0, NETDATA_SIGNAL_SAVE_DATABASE },
         { SIGUSR2, "SIGUSR2", 0, NETDATA_SIGNAL_RELOAD_HEALTH },
         { SIGBUS,  "SIGBUS",  0, NETDATA_SIGNAL_FATAL         },
+        { SIGCHLD, "SIGCHLD", 0, NETDATA_SIGNAL_CHILD         },
 
         // terminator
         { 0,       "NONE",    0, NETDATA_SIGNAL_END_OF_LIST   }
@@ -42,7 +46,7 @@ static void signal_handler(int signo) {
                 char buffer[200 + 1];
                 snprintfz(buffer, 200, "\nSIGNAL HANLDER: received: %s. Oops! This is bad!\n", signals_waiting[i].name);
                 if(write(STDERR_FILENO, buffer, strlen(buffer)) == -1) {
-                    // nothing to do - we cannot write but there is no way to complaint about it
+                    // nothing to do - we cannot write but there is no way to complain about it
                     ;
                 }
             }
@@ -74,15 +78,33 @@ void signals_init(void) {
     struct sigaction sa;
     sa.sa_flags = 0;
 
+    // Enable process tracking / reaper if running as init (pid == 1).
+    // This prevents zombie processes when running in a container.
+    if (getpid() == 1) {
+        info("SIGNAL: Enabling reaper");
+        myp_init();
+        reaper_enabled = 1;
+    } else {
+        info("SIGNAL: Not enabling reaper");
+    }
+
     // ignore all signals while we run in a signal handler
     sigfillset(&sa.sa_mask);
 
     int i;
     for (i = 0; signals_waiting[i].action != NETDATA_SIGNAL_END_OF_LIST; i++) {
-        if(signals_waiting[i].action == NETDATA_SIGNAL_IGNORE)
+        switch (signals_waiting[i].action) {
+        case NETDATA_SIGNAL_IGNORE:
             sa.sa_handler = SIG_IGN;
-        else
+            break;
+        case NETDATA_SIGNAL_CHILD:
+            if (reaper_enabled == 0)
+                continue;
+            // FALLTHROUGH
+        default:
             sa.sa_handler = signal_handler;
+            break;
+        }
 
         if(sigaction(signals_waiting[i].signo, &sa, NULL) == -1)
             error("SIGNAL: Failed to change signal handler for: %s", signals_waiting[i].name);
@@ -100,6 +122,76 @@ void signals_reset(void) {
         if(sigaction(signals_waiting[i].signo, &sa, NULL) == -1)
             error("SIGNAL: Failed to reset signal handler for: %s", signals_waiting[i].name);
     }
+
+    if (reaper_enabled == 1)
+        myp_free();
+}
+
+// reap_child reaps the child identified by pid.
+static void reap_child(pid_t pid) {
+    siginfo_t i;
+
+    errno = 0;
+    debug(D_CHILDS, "SIGNAL: Reaping pid: %d...", pid);
+    if (waitid(P_PID, (id_t)pid, &i, WEXITED|WNOHANG) == -1) {
+        if (errno != ECHILD)
+            error("SIGNAL: Failed to wait for: %d", pid);
+        else
+            debug(D_CHILDS, "SIGNAL: Already reaped: %d", pid);
+        return;
+    } else if (i.si_pid == 0) {
+        // Process didn't exit, this shouldn't happen.
+        return;
+    }
+
+    switch (i.si_code) {
+    case CLD_EXITED:
+        debug(D_CHILDS, "SIGNAL: Child %d exited: %d", pid, i.si_status);
+        break;
+    case CLD_KILLED:
+        debug(D_CHILDS, "SIGNAL: Child %d killed by signal: %d", pid, i.si_status);
+        break;
+    case CLD_DUMPED:
+        debug(D_CHILDS, "SIGNAL: Child %d dumped core by signal: %d", pid, i.si_status);
+        break;
+    case CLD_STOPPED:
+        debug(D_CHILDS, "SIGNAL: Child %d stopped by signal: %d", pid, i.si_status);
+        break;
+    case CLD_TRAPPED:
+        debug(D_CHILDS, "SIGNAL: Child %d trapped by signal: %d", pid, i.si_status);
+        break;
+    case CLD_CONTINUED:
+        debug(D_CHILDS, "SIGNAL: Child %d continued by signal: %d", pid, i.si_status);
+        break;
+    default:
+        debug(D_CHILDS, "SIGNAL: Child %d gave us a SIGCHLD with code %d and status %d.", pid, i.si_code, i.si_status);
+    }
+}
+
+// reap_children reaps all pending children which are not managed by myp.
+static void reap_children() {
+    siginfo_t i;
+
+    while (1 == 1) {
+        // Identify which process caused the signal so we can determine
+        // if we need to reap a re-parented process.
+        i.si_pid = 0;
+        if (waitid(P_ALL, (id_t)0, &i, WEXITED|WNOHANG|WNOWAIT) == -1) {
+            if (errno != ECHILD) // This shouldn't happen with WNOHANG but does.
+                error("SIGNAL: Failed to wait");
+            return;
+        } else if (i.si_pid == 0) {
+            // No child exited.
+            return;
+        } else if (myp_reap(i.si_pid) == 0) {
+            // myp managed, sleep for a short time to avoid busy wait while
+            // this is handled by myp.
+            usleep(10000);
+        } else {
+            // Unknown process, likely a re-parented child, reap it.
+            reap_child(i.si_pid);
+        }
+    }
 }
 
 void signals_handle(void) {
@@ -157,6 +249,11 @@ void signals_handle(void) {
                             case NETDATA_SIGNAL_FATAL:
                                 fatal("SIGNAL: Received %s. netdata now exits.", name);
 
+                            case NETDATA_SIGNAL_CHILD:
+                                debug(D_CHILDS, "SIGNAL: Received %s. Reaping...", name);
+                                reap_children();
+                                break;
+
                             default:
                                 info("SIGNAL: Received %s. No signal handler configured. Ignoring it.", name);
                                 break;
diff --git a/libnetdata/popen/popen.c b/libnetdata/popen/popen.c
index 906b105357..1c4ae64d6c 100644
--- a/libnetdata/popen/popen.c
+++ b/libnetdata/popen/popen.c
@@ -2,46 +2,79 @@
 
 #include "../libnetdata.h"
 
-/*
+static pthread_mutex_t myp_lock;
+static int myp_tracking = 0;
+
 struct mypopen {
     pid_t pid;
-    FILE *fp;
     struct mypopen *next;
     struct mypopen *prev;
 };
 
 static struct mypopen *mypopen_root = NULL;
 
-static void mypopen_add(FILE *fp, pid_t *pid) {
-    struct mypopen *mp = malloc(sizeof(struct mypopen));
-    if(!mp) {
-        fatal("Cannot allocate %zu bytes", sizeof(struct mypopen))
+// myp_add_lock takes the lock if we're tracking.
+static void myp_add_lock(void) {
+    if (myp_tracking == 0)
         return;
-    }
 
-    mp->fp = fp;
+    netdata_mutex_lock(&myp_lock);
+}
+
+// myp_add_unlock release the lock if we're tracking.
+static void myp_add_unlock(void) {
+    if (myp_tracking == 0)
+        return;
+
+    netdata_mutex_unlock(&myp_lock);
+}
+
+// myp_add_locked adds pid if we're tracking.
+// myp_add_lock must have been called previously.
+static void myp_add_locked(pid_t pid) {
+    struct mypopen *mp;
+
+    if (myp_tracking == 0)
+        return;
+
+    mp = mallocz(sizeof(struct mypopen));
     mp->pid = pid;
-    mp->next = popen_root;
+
+    mp->next = mypopen_root;
     mp->prev = NULL;
-    if(mypopen_root) mypopen_root->prev = mp;
+    if (mypopen_root != NULL)
+        mypopen_root->prev = mp;
     mypopen_root = mp;
+    netdata_mutex_unlock(&myp_lock);
 }
 
-static void mypopen_del(FILE *fp) {
+// myp_del deletes pid if we're tracking.
+static void myp_del(pid_t pid) {
     struct mypopen *mp;
 
-    for(mp = mypopen_root; mp; mp = mp->next)
-        if(mp->fd == fp) break;
+    if (myp_tracking == 0)
+        return;
 
-    if(!mp) error("Cannot find mypopen() file pointer in open childs.");
-    else {
-        if(mp->next) mp->next->prev = mp->prev;
-        if(mp->prev) mp->prev->next = mp->next;
-        if(mypopen_root == mp) mypopen_root = mp->next;
-        free(mp);
+    netdata_mutex_lock(&myp_lock);
+    for (mp = mypopen_root; mp != NULL; mp = mp->next) {
+        if (mp->pid == pid) {
+            if (mp->next != NULL)
+                mp->next->prev = mp->prev;
+            if (mp->prev != NULL)
+                mp->prev->next = mp->next;
+            if (mypopen_root == mp)
+                mypopen_root = mp->next;
+            freez(mp);
+            break;
+        }
     }
+
+    if (mp == NULL)
+        error("Cannot find pid %d.", pid);
+
+    netdata_mutex_unlock(&myp_lock);
 }
-*/
+
 #define PIPE_READ 0
 #define PIPE_WRITE 1
 
@@ -58,7 +91,7 @@ static inline FILE *custom_popene(const char *command, volatile pid_t *pidptr, c
     posix_spawnattr_t attr;
     posix_spawn_file_actions_t fa;
 
-    if(pipe(pipefd) == -1)
+    if (pipe(pipefd) == -1)
         return NULL;
     if ((fp = fdopen(pipefd[PIPE_READ], "r")) == NULL) {
         goto error_after_pipe;
@@ -66,7 +99,7 @@ static inline FILE *custom_popene(const char *command, volatile pid_t *pidptr, c
 
     // Mark all files to be closed by the exec() stage of posix_spawn()
     int i;
-    for(i = (int) (sysconf(_SC_OPEN_MAX) - 1); i >= 0; i--)
+    for (i = (int) (sysconf(_SC_OPEN_MAX) - 1); i >= 0; i--)
         if(i != STDIN_FILENO && i != STDERR_FILENO)
             (void)fcntl(i, F_SETFD, FD_CLOEXEC);
 
@@ -92,10 +125,16 @@ static inline FILE *custom_popene(const char *command, volatile pid_t *pidptr, c
     } else {
         error("posix_spawnattr_init() failed.");
     }
+
+    // Take the lock while we fork to ensure we don't race with SIGCHLD
+    // delivery on a process which exits quickly.
+    myp_add_lock();
     if (!posix_spawn(&pid, "/bin/sh", &fa, &attr, spawn_argv, env)) {
         *pidptr = pid;
+        myp_add_locked(pid);
         debug(D_CHILDS, "Spawned command: '%s' on pid %d from parent pid %d.", command, pid, getpid());
     } else {
+        myp_add_unlock();
         error("Failed to spawn command: '%s' from parent pid %d.", command, getpid());
         fclose(fp);
         fp = NULL;
@@ -128,6 +167,60 @@ error_after_pipe:
 // See man environ
 extern char **environ;
 
+// myp_init should be called by apps which act as init
+// (pid 1) so that processes created by mypopen and mypopene
+// are tracked. This enables the reaper to ignore processes
+// which will be handled internally, by calling myp_reap, to
+// avoid issues with already reaped processes during wait calls.
+//
+// Callers should call myp_free() to clean up resources.
+void myp_init(void) {
+    info("process tracking enabled.");
+    myp_tracking = 1;
+
+    if (netdata_mutex_init(&myp_lock) != 0) {
+	fatal("myp_init() mutex init failed.");
+    }
+}
+
+// myp_free cleans up any resources allocated for process
+// tracking.
+void myp_free(void) {
+    struct mypopen *mp, *next;
+
+    if (myp_tracking == 0)
+        return;
+
+    netdata_mutex_lock(&myp_lock);
+    for (mp = mypopen_root; mp != NULL; mp = next) {
+        next = mp->next;
+        freez(mp);
+    }
+
+    mypopen_root = NULL;
+    myp_tracking = 0;
+    netdata_mutex_unlock(&myp_lock);
+}
+
+// myp_reap returns 1 if pid should be reaped, 0 otherwise.
+int myp_reap(pid_t pid) {
+    struct mypopen *mp;
+
+    if (myp_tracking == 0)
+        return 0;
+
+    netdata_mutex_lock(&myp_lock);
+    for (mp = mypopen_root; mp != NULL; mp = mp->next) {
+        if (mp->pid == pid) {
+            netdata_mutex_unlock(&myp_lock);
+            return 0;
+        }
+    }
+    netdata_mutex_unlock(&myp_lock);
+
+    return 1;
+}
+
 FILE *mypopen(const char *command, volatile pid_t *pidptr) {
     return custom_popene(command, pidptr, environ);
 }
@@ -137,9 +230,10 @@ FILE *mypopene(const char *command, volatile pid_t *pidptr, char **env) {
 }
 
 int mypclose(FILE *fp, pid_t pid) {
-    debug(D_EXIT, "Request to mypclose() on pid %d", pid);
+    int ret;
+    siginfo_t info;
 
-    /*mypopen_del(fp);*/
+    debug(D_EXIT, "Request to mypclose() on pid %d", pid);
 
     // close the pipe fd
     // this is required in musl
@@ -151,9 +245,11 @@ int mypclose(FILE *fp, pid_t pid) {
 
     errno = 0;
 
-    siginfo_t info;
-    if(waitid(P_PID, (id_t) pid, &info, WEXITED) != -1) {
-        switch(info.si_code) {
+    ret = waitid(P_PID, (id_t) pid, &info, WEXITED);
+    myp_del(pid);
+
+    if (ret != -1) {
+        switch (info.si_code) {
             case CLD_EXITED:
                 if(info.si_status)
                     error("child pid %d exited with code %d.", info.si_pid, info.si_status);
diff --git a/libnetdata/popen/popen.h b/libnetdata/popen/popen.h
index 90d4b829b0..32f64e460b 100644
--- a/libnetdata/popen/popen.h
+++ b/libnetdata/popen/popen.h
@@ -11,6 +11,9 @@
 extern FILE *mypopen(const char *command, volatile pid_t *pidptr);
 extern FILE *mypopene(const char *command, volatile pid_t *pidptr, char **env);
 extern int mypclose(FILE *fp, pid_t pid);
+extern void myp_init(void);
+extern void myp_free(void);
+extern int myp_reap(pid_t pid);
 
 extern void signals_unblock(void);
 extern void signals_reset(void);