utils: fix lxc_popen()/lxc_pclose()

- rework and fix pipe fd leak

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

diff --git a/src/lxc/utils.c b/src/lxc/utils.c
index d867f714d1f4ab28d46d38dcb11456ff99872c67..e6a44a516156d61049a0e5ab528ba7744b91e15e 100644 (file)
--- a/src/lxc/utils.c
+++ b/src/lxc/utils.c
@@ -470,152 +470,105 @@ const char** lxc_va_arg_list_to_argv_const(va_list ap, size_t skip)
        return (const char**)lxc_va_arg_list_to_argv(ap, skip, 0);
 }
 
-extern struct lxc_popen_FILE *lxc_popen(const char *command)
+struct lxc_popen_FILE *lxc_popen(const char *command)
 {
        int ret;
-       struct lxc_popen_FILE *fp = NULL;
-       int parent_end = -1, child_end = -1;
        int pipe_fds[2];
        pid_t child_pid;
+       struct lxc_popen_FILE *fp = NULL;
 
-       int r = pipe2(pipe_fds, O_CLOEXEC);
-
-       if (r < 0) {
-               ERROR("pipe2 failure");
+       ret = pipe2(pipe_fds, O_CLOEXEC);
+       if (ret < 0)
                return NULL;
-       }
-
-       parent_end = pipe_fds[0];
-       child_end = pipe_fds[1];
 
        child_pid = fork();
+       if (child_pid < 0)
+               goto on_error;
 
-       if (child_pid == 0) {
-               /* child */
-
-               close(parent_end);
+       if (!child_pid) {
+               sigset_t mask;
 
-               if (child_end != STDOUT_FILENO) {
-                       /* dup2() doesn't dup close-on-exec flag */
-                       ret = dup2(child_end, STDOUT_FILENO);
-                       if (ret < 0)
-                               WARN("Failed to duplicate stdout fd");
-               } else {
-                       /*
-                        * The descriptor is already the one we will use.
-                        * But it must not be marked close-on-exec.
-                        * Undo the effects.
-                        */
-                       ret = fcntl(child_end, F_SETFD, 0);
-                       if (ret < 0) {
-                               SYSERROR("Failed to remove FD_CLOEXEC from fd.");
-                               exit(127);
-                       }
-               }
+               close(pipe_fds[0]);
 
-               if (child_end != STDERR_FILENO) {
-                       /* dup2() doesn't dup close-on-exec flag */
-                       ret = dup2(child_end, STDERR_FILENO);
-                       if (ret < 0)
-                               WARN("Failed to duplicate stdout fd");
-               } else {
-                       /*
-                        * The descriptor is already the one we will use.
-                        * But it must not be marked close-on-exec.
-                        * Undo the effects.
-                        */
-                       ret = fcntl(child_end, F_SETFD, 0);
-                       if (ret < 0) {
-                               SYSERROR("Failed to remove FD_CLOEXEC from fd.");
-                               exit(127);
-                       }
-               }
-
-               /*
-                * Unblock signals.
-                * This is the main/only reason
-                * why we do our lousy popen() emulation.
-                */
-               {
-                       sigset_t mask;
-                       sigfillset(&mask);
-                       sigprocmask(SIG_UNBLOCK, &mask, NULL);
+               /* duplicate stdout */
+               if (pipe_fds[1] != STDOUT_FILENO)
+                       ret = dup2(pipe_fds[1], STDOUT_FILENO);
+               else
+                       ret = fcntl(pipe_fds[1], F_SETFD, 0);
+               if (ret < 0) {
+                       close(pipe_fds[1]);
+                       exit(EXIT_FAILURE);
                }
 
-               execl("/bin/sh", "sh", "-c", command, (char *) NULL);
-               exit(127);
-       }
+               /* duplicate stderr */
+               if (pipe_fds[1] != STDERR_FILENO)
+                       ret = dup2(pipe_fds[1], STDERR_FILENO);
+               else
+                       ret = fcntl(pipe_fds[1], F_SETFD, 0);
+               close(pipe_fds[1]);
+               if (ret < 0)
+                       exit(EXIT_FAILURE);
 
-       /* parent */
+               /* unblock all signals */
+               ret = sigfillset(&mask);
+               if (ret < 0)
+                       exit(EXIT_FAILURE);
 
-       close(child_end);
+               ret = sigprocmask(SIG_UNBLOCK, &mask, NULL);
+               if (ret < 0)
+                       exit(EXIT_FAILURE);
 
-       if (child_pid < 0) {
-               ERROR("fork failure");
-               goto error;
+               execl("/bin/sh", "sh", "-c", command, (char *)NULL);
+               exit(127);
        }
 
-       fp = calloc(1, sizeof(*fp));
-       if (!fp) {
-               ERROR("failed to allocate memory");
-               goto error;
-       }
+       close(pipe_fds[1]);
+       pipe_fds[1] = -1;
 
-       fp->f = fdopen(parent_end, "r");
-       if (!fp->f) {
-               ERROR("fdopen failure");
-               goto error;
-       }
+       fp = malloc(sizeof(*fp));
+       if (!fp)
+               goto on_error;
 
        fp->child_pid = child_pid;
+       fp->pipe = pipe_fds[0];
 
-       return fp;
-
-error:
+       fp->f = fdopen(pipe_fds[0], "r");
+       if (!fp->f)
+               goto on_error;
 
-       if (fp) {
-               if (fp->f) {
-                       fclose(fp->f);
-                       parent_end = -1; /* so we do not close it second time */
-               }
+       return fp;
 
+on_error:
+       if (fp)
                free(fp);
-       }
 
-       if (parent_end != -1)
-               close(parent_end);
+       if (pipe_fds[0] >= 0)
+               close(pipe_fds[0]);
+
+       if (pipe_fds[1] >= 0)
+               close(pipe_fds[1]);
 
        return NULL;
 }
 
-extern int lxc_pclose(struct lxc_popen_FILE *fp)
+int lxc_pclose(struct lxc_popen_FILE *fp)
 {
-       FILE *f = NULL;
-       pid_t child_pid = 0;
-       int wstatus = 0;
        pid_t wait_pid;
+       int wstatus = 0;
 
-       if (fp) {
-               f = fp->f;
-               child_pid = fp->child_pid;
-               /* free memory (we still need to close file stream) */
-               free(fp);
-               fp = NULL;
-       }
-
-       if (!f || fclose(f)) {
-               ERROR("fclose failure");
+       if (!fp)
                return -1;
-       }
 
        do {
-               wait_pid = waitpid(child_pid, &wstatus, 0);
-       } while (wait_pid == -1 && errno == EINTR);
+               wait_pid = waitpid(fp->child_pid, &wstatus, 0);
+       } while (wait_pid < 0 && errno == EINTR);
+
+       close(fp->pipe);
+       fclose(fp->f);
+       free(fp);
 
-       if (wait_pid == -1) {
-               ERROR("waitpid failure");
+       if (wait_pid < 0)
                return -1;
-       }
 
        return wstatus;
 }

diff --git a/src/lxc/utils.h b/src/lxc/utils.h
index 41f3716e35cca3b0bc087d97594f77df616a34cf..e83ed49eba371c2b92ff96534b7c07aef0bbbe16 100644 (file)
--- a/src/lxc/utils.h
+++ b/src/lxc/utils.h
@@ -190,6 +190,7 @@ static inline int signalfd(int fd, const sigset_t *mask, int flags)
  * without additional wrappers.
  */
 struct lxc_popen_FILE {
+       int pipe;
        FILE *f;
        pid_t child_pid;
 };