Browse Source

Reimplemented mypopen() function family (#6339)

* Reimplementd mypopen() family based on posix_spawn() instead of fork()
and execl().

The problem with fork() is that if the parent process has a large address
space then the fork() may fail due to insufficient free memory in the
system if memory overcommit is not enabled.

posix_spawn() does not call fork() and does not suffer from this problem.
It is also more portable than vfork() which is deprecated and clone()
which is linux only.

* Removed dead code
Markos Fountoulakis 5 years ago
parent
commit
0707fbaaac
3 changed files with 72 additions and 104 deletions
  1. 0 14
      collectors/tc.plugin/plugin_tc.c
  2. 1 0
      libnetdata/libnetdata.h
  3. 71 90
      libnetdata/popen/popen.c

+ 0 - 14
collectors/tc.plugin/plugin_tc.c

@@ -874,9 +874,6 @@ void *tc_main(void *ptr) {
     uint32_t SETDEVICEGROUP_HASH = simple_hash("SETDEVICEGROUP");
     uint32_t SETCLASSNAME_HASH = simple_hash("SETCLASSNAME");
     uint32_t WORKTIME_HASH = simple_hash("WORKTIME");
-#ifdef DETACH_PLUGINS_FROM_NETDATA
-    uint32_t MYPID_HASH = simple_hash("MYPID");
-#endif
     uint32_t first_hash;
 
     snprintfz(command, TC_LINE_MAX, "%s/tc-qos-helper.sh", netdata_configured_primary_plugins_dir);
@@ -1119,17 +1116,6 @@ void *tc_main(void *ptr) {
                 rrdset_done(sttime);
 
             }
-#ifdef DETACH_PLUGINS_FROM_NETDATA
-            else if(unlikely(first_hash == MYPID_HASH && (strcmp(words[0], "MYPID") == 0))) {
-                // debug(D_TC_LOOP, "MYPID line '%s'", words[1]);
-                char *id = words[1];
-                pid_t pid = atol(id);
-
-                if(likely(pid)) tc_child_pid = pid;
-
-                debug(D_TC_LOOP, "TC: Child PID is %d.", tc_child_pid);
-            }
-#endif
             //else {
             //  debug(D_TC_LOOP, "IGNORED line");
             //}

+ 1 - 0
libnetdata/libnetdata.h

@@ -81,6 +81,7 @@
 #include <time.h>
 #include <unistd.h>
 #include <uuid/uuid.h>
+#include <spawn.h>
 
 #ifdef HAVE_NETINET_IN_H
 #include <netinet/in.h>

+ 71 - 90
libnetdata/popen/popen.c

@@ -45,110 +45,91 @@ static void mypopen_del(FILE *fp) {
 #define PIPE_READ 0
 #define PIPE_WRITE 1
 
-FILE *mypopen(const char *command, volatile pid_t *pidptr)
-{
-    int pipefd[2];
-
-    if(pipe(pipefd) == -1) return NULL;
+static inline FILE *custom_popene(const char *command, volatile pid_t *pidptr, char **env) {
+    FILE *fp;
+    int pipefd[2], error;
+    pid_t pid;
+    char *const spawn_argv[] = {
+            "sh",
+            "-c",
+            (char *)command,
+            NULL
+    };
+    posix_spawnattr_t attr;
+    posix_spawn_file_actions_t fa;
 
-    int pid = fork();
-    if(pid == -1) {
-        close(pipefd[PIPE_READ]);
-        close(pipefd[PIPE_WRITE]);
+    if(pipe(pipefd) == -1)
         return NULL;
+    if ((fp = fdopen(pipefd[PIPE_READ], "r")) == NULL) {
+        goto error_after_pipe;
     }
-    if(pid != 0) {
-        // the parent
-        *pidptr = pid;
-        close(pipefd[PIPE_WRITE]);
-        FILE *fp = fdopen(pipefd[PIPE_READ], "r");
-        /*mypopen_add(fp, pid);*/
-        return(fp);
-    }
-    // the child
 
-    // close all files
+    // 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--)
-        if(i != STDIN_FILENO && i != STDERR_FILENO && i != pipefd[PIPE_WRITE]) close(i);
-
-    // move the pipe to stdout
-    if(pipefd[PIPE_WRITE] != STDOUT_FILENO) {
-        dup2(pipefd[PIPE_WRITE], STDOUT_FILENO);
-        close(pipefd[PIPE_WRITE]);
+        if(i != STDIN_FILENO && i != STDERR_FILENO)
+            fcntl(i, F_SETFD, FD_CLOEXEC);
+
+    if (!posix_spawn_file_actions_init(&fa)) {
+        // move the pipe to stdout in the child
+        if (posix_spawn_file_actions_adddup2(&fa, pipefd[PIPE_WRITE], STDOUT_FILENO)) {
+            error("posix_spawn_file_actions_adddup2() failed");
+            goto error_after_posix_spawn_file_actions_init;
+        }
+    } else {
+        error("posix_spawn_file_actions_init() failed.");
+        goto error_after_pipe;
     }
-
-#ifdef DETACH_PLUGINS_FROM_NETDATA
-    // this was an attempt to detach the child and use the suspend mode charts.d
-    // unfortunatelly it does not work as expected.
-
-    // fork again to become session leader
-    pid = fork();
-    if(pid == -1)
-        error("pre-execution of command '%s' on pid %d: Cannot fork 2nd time.", command, getpid());
-
-    if(pid != 0) {
-        // the parent
-        exit(0);
+    if (!(error = posix_spawnattr_init(&attr))) {
+        // reset all signals in the child
+        sigset_t mask;
+
+        if (posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETSIGMASK | POSIX_SPAWN_SETSIGDEF))
+            error("posix_spawnattr_setflags() failed.");
+        sigemptyset(&mask);
+        if (posix_spawnattr_setsigmask(&attr, &mask))
+            error("posix_spawnattr_setsigmask() failed.");
+    } else {
+        error("posix_spawnattr_init() failed.");
     }
-
-    // set a new process group id for just this child
-    if( setpgid(0, 0) != 0 )
-        error("pre-execution of command '%s' on pid %d: Cannot set a new process group.", command, getpid());
-
-    if( getpgid(0) != getpid() )
-        error("pre-execution of command '%s' on pid %d: Cannot set a new process group. Process group set is incorrect. Expected %d, found %d", command, getpid(), getpid(), getpgid(0));
-
-    if( setsid() != 0 )
-        error("pre-execution of command '%s' on pid %d: Cannot set session id.", command, getpid());
-
-    fprintf(stdout, "MYPID %d\n", getpid());
-    fflush(NULL);
-#endif
-
-    // reset all signals
-    signals_unblock();
-    signals_reset();
-
-    debug(D_CHILDS, "executing command: '%s' on pid %d.", command, getpid());
-    execl("/bin/sh", "sh", "-c", command, NULL);
-    exit(1);
-}
-
-FILE *mypopene(const char *command, volatile pid_t *pidptr, char **env) {
-    int pipefd[2];
-
-    if(pipe(pipefd) == -1)
-        return NULL;
-
-    int pid = fork();
-    if(pid == -1) {
+    if (!posix_spawn(&pid, "/bin/sh", &fa, &attr, spawn_argv, env)) {
+        *pidptr = pid;
+        debug(D_CHILDS, "Spawned command: '%s' on pid %d from parent pid %d.", command, pid, getpid());
+    } else {
+        error("Failed to spawn command: '%s' from parent pid %d.", command, getpid());
         close(pipefd[PIPE_READ]);
-        close(pipefd[PIPE_WRITE]);
-        return NULL;
+        fp = NULL;
     }
-    if(pid != 0) {
-        // the parent
-        *pidptr = pid;
-        close(pipefd[PIPE_WRITE]);
-        FILE *fp = fdopen(pipefd[PIPE_READ], "r");
-        return(fp);
+    close(pipefd[PIPE_WRITE]);
+
+    if (!error) {
+        // posix_spawnattr_init() succeeded
+        if (posix_spawnattr_destroy(&attr))
+            error("posix_spawnattr_destroy");
     }
-    // the child
+    if (posix_spawn_file_actions_destroy(&fa))
+        error("posix_spawn_file_actions_destroy");
+
+    return fp;
+
+error_after_posix_spawn_file_actions_init:
+    if (posix_spawn_file_actions_destroy(&fa))
+        error("posix_spawn_file_actions_destroy");
+error_after_pipe:
+    close(pipefd[PIPE_READ]);
+    close(pipefd[PIPE_WRITE]);
+    return NULL;
+}
 
-    // close all files
-    int i;
-    for(i = (int) (sysconf(_SC_OPEN_MAX) - 1); i >= 0; i--)
-        if(i != STDIN_FILENO && i != STDERR_FILENO && i != pipefd[PIPE_WRITE]) close(i);
+// See man environ
+extern char **environ;
 
-    // move the pipe to stdout
-    if(pipefd[PIPE_WRITE] != STDOUT_FILENO) {
-        dup2(pipefd[PIPE_WRITE], STDOUT_FILENO);
-        close(pipefd[PIPE_WRITE]);
-    }
+FILE *mypopen(const char *command, volatile pid_t *pidptr) {
+    return custom_popene(command, pidptr, environ);
+}
 
-    execle("/bin/sh", "sh", "-c", command, NULL, env);
-    exit(1);
+FILE *mypopene(const char *command, volatile pid_t *pidptr, char **env) {
+    return custom_popene(command, pidptr, env);
 }
 
 int mypclose(FILE *fp, pid_t pid) {