pipecmd: don't close stderr/stdout/stdin in parent when &pipecmd_preserve
authorRusty Russell <rusty@rustcorp.com.au>
Thu, 7 Feb 2019 03:56:58 +0000 (14:26 +1030)
committerRusty Russell <rusty@rustcorp.com.au>
Thu, 7 Feb 2019 03:56:58 +0000 (14:26 +1030)
This was reported and fixed by @m-schmoock for stderr, but I decided to
rework to make it clearer and cover the other cases:

https://github.com/ElementsProject/lightning/pull/2321/commits/67dad01549f876e5e620eeffe2f1e78c35bd0f03

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
ccan/pipecmd/pipecmd.c

index c2b514df5e97189230dbc282f3b673293a77028c..d45713b6f8015db478592f5795fdfda474605c3c 100644 (file)
@@ -47,68 +47,76 @@ pid_t pipecmdarr(int *fd_tochild, int *fd_fromchild, int *fd_errfromchild,
                 char *const *arr)
 {
        int tochild[2], fromchild[2], errfromchild[2], execfail[2];
                 char *const *arr)
 {
        int tochild[2], fromchild[2], errfromchild[2], execfail[2];
+       /* fds for parent to close */
+       int par_close[4], num_par_close = 0;
+       /* fds for child to close */
+       int child_close[4], num_child_close = 0;
        pid_t childpid;
        int err;
 
        if (fd_tochild) {
                if (fd_tochild == &pipecmd_preserve) {
                        tochild[0] = STDIN_FILENO;
        pid_t childpid;
        int err;
 
        if (fd_tochild) {
                if (fd_tochild == &pipecmd_preserve) {
                        tochild[0] = STDIN_FILENO;
-                       fd_tochild = NULL;
-               } else if (pipe(tochild) != 0)
+               } else if (pipe(tochild) == 0) {
+                       par_close[num_par_close++] = tochild[0];
+                       child_close[num_child_close++] = tochild[1];
+               } else
                        goto fail;
        } else {
                tochild[0] = open("/dev/null", O_RDONLY);
                if (tochild[0] < 0)
                        goto fail;
                        goto fail;
        } else {
                tochild[0] = open("/dev/null", O_RDONLY);
                if (tochild[0] < 0)
                        goto fail;
+               par_close[num_par_close++] = tochild[0];
        }
        if (fd_fromchild) {
                if (fd_fromchild == &pipecmd_preserve) {
                        fromchild[1] = STDOUT_FILENO;
        }
        if (fd_fromchild) {
                if (fd_fromchild == &pipecmd_preserve) {
                        fromchild[1] = STDOUT_FILENO;
-                       fd_fromchild = NULL;
-               } else if (pipe(fromchild) != 0)
-                       goto close_tochild_fail;
+               } else if (pipe(fromchild) == 0) {
+                       par_close[num_par_close++] = fromchild[1];
+                       child_close[num_child_close++] = fromchild[0];
+               } else
+                       goto fail;
        } else {
                fromchild[1] = open("/dev/null", O_WRONLY);
                if (fromchild[1] < 0)
        } else {
                fromchild[1] = open("/dev/null", O_WRONLY);
                if (fromchild[1] < 0)
-                       goto close_tochild_fail;
+                       goto fail;
+               par_close[num_par_close++] = fromchild[1];
        }
        if (fd_errfromchild) {
                if (fd_errfromchild == &pipecmd_preserve) {
                        errfromchild[1] = STDERR_FILENO;
        }
        if (fd_errfromchild) {
                if (fd_errfromchild == &pipecmd_preserve) {
                        errfromchild[1] = STDERR_FILENO;
-                       fd_errfromchild = NULL;
                } else if (fd_errfromchild == fd_fromchild) {
                        errfromchild[0] = fromchild[0];
                        errfromchild[1] = fromchild[1];
                } else if (fd_errfromchild == fd_fromchild) {
                        errfromchild[0] = fromchild[0];
                        errfromchild[1] = fromchild[1];
-               } else {
-                       if (pipe(errfromchild) != 0)
-                               goto close_fromchild_fail;
-               }
+               } else if (pipe(errfromchild) == 0) {
+                       par_close[num_par_close++] = errfromchild[1];
+                       child_close[num_child_close++] = errfromchild[0];
+               } else
+                       goto fail;
        } else {
                errfromchild[1] = open("/dev/null", O_WRONLY);
                if (errfromchild[1] < 0)
        } else {
                errfromchild[1] = open("/dev/null", O_WRONLY);
                if (errfromchild[1] < 0)
-                       goto close_fromchild_fail;
+                       goto fail;
+               par_close[num_par_close++] = errfromchild[1];
        }
 
        if (pipe(execfail) != 0)
        }
 
        if (pipe(execfail) != 0)
-               goto close_errfromchild_fail;
+               goto fail;
+
+       par_close[num_par_close++] = execfail[1];
+       child_close[num_child_close++] = execfail[0];
 
        if (fcntl(execfail[1], F_SETFD, fcntl(execfail[1], F_GETFD)
                  | FD_CLOEXEC) < 0)
 
        if (fcntl(execfail[1], F_SETFD, fcntl(execfail[1], F_GETFD)
                  | FD_CLOEXEC) < 0)
-               goto close_execfail_fail;
+               goto fail;
 
        childpid = fork();
        if (childpid < 0)
 
        childpid = fork();
        if (childpid < 0)
-               goto close_execfail_fail;
+               goto fail;
 
        if (childpid == 0) {
 
        if (childpid == 0) {
-               if (fd_tochild)
-                       close(tochild[1]);
-               if (fd_fromchild)
-                       close(fromchild[0]);
-               if (fd_errfromchild && fd_errfromchild != fd_fromchild)
-                       close(errfromchild[0]);
-
-               close(execfail[0]);
+               for (int i = 0; i < num_child_close; i++)
+                       close(child_close[i]);
 
                // Child runs command.
                if (tochild[0] != STDIN_FILENO) {
 
                // Child runs command.
                if (tochild[0] != STDIN_FILENO) {
@@ -140,10 +148,9 @@ pid_t pipecmdarr(int *fd_tochild, int *fd_fromchild, int *fd_errfromchild,
                exit(127);
        }
 
                exit(127);
        }
 
-       close(tochild[0]);
-       close(fromchild[1]);
-       close(errfromchild[1]);
-       close(execfail[1]);
+       for (int i = 0; i < num_par_close; i++)
+               close(par_close[i]);
+
        /* Child will close this without writing on successful exec. */
        if (read(execfail[0], &err, sizeof(err)) == sizeof(err)) {
                close(execfail[0]);
        /* Child will close this without writing on successful exec. */
        if (read(execfail[0], &err, sizeof(err)) == sizeof(err)) {
                close(execfail[0]);
@@ -152,30 +159,17 @@ pid_t pipecmdarr(int *fd_tochild, int *fd_fromchild, int *fd_errfromchild,
                return -1;
        }
        close(execfail[0]);
                return -1;
        }
        close(execfail[0]);
-       if (fd_tochild)
+       if (fd_tochild && fd_tochild != &pipecmd_preserve)
                *fd_tochild = tochild[1];
                *fd_tochild = tochild[1];
-       if (fd_fromchild)
+       if (fd_fromchild && fd_fromchild != &pipecmd_preserve)
                *fd_fromchild = fromchild[0];
                *fd_fromchild = fromchild[0];
-       if (fd_errfromchild)
+       if (fd_errfromchild && fd_errfromchild != &pipecmd_preserve)
                *fd_errfromchild = errfromchild[0];
        return childpid;
 
                *fd_errfromchild = errfromchild[0];
        return childpid;
 
-close_execfail_fail:
-       close_noerr(execfail[0]);
-       close_noerr(execfail[1]);
-close_errfromchild_fail:
-       if (fd_errfromchild)
-               close_noerr(errfromchild[0]);
-       close_noerr(errfromchild[1]);
-close_fromchild_fail:
-       if (fd_fromchild)
-               close_noerr(fromchild[0]);
-       close_noerr(fromchild[1]);
-close_tochild_fail:
-       close_noerr(tochild[0]);
-       if (fd_tochild)
-               close_noerr(tochild[1]);
 fail:
 fail:
+       for (int i = 0; i < num_par_close; i++)
+               close_noerr(par_close[i]);
        return -1;
 }
 
        return -1;
 }