Libguestfs - Sep 2015 - [PATCH 0/7] copy-in/copy-out: Capture errors from tar subprocess (RHBZ#1267032).

Commits 3c27f3d91e1566854747bbe844186783fc84f3a8 and
1b6f0daa9ae7fcc94e389232d0c397816cda973d added an internal API for
running commands asynchronously.  It is only used by the copy-in and
copy-out APIs.

Unfortunately this made the command code very complex: it was almost
impossible to redirect stderr to a file, and there were a lot of
long-range dependencies through the file.  It was also buggy: it set
up stderr of the child process pointing pipe, but never read from the
pipe, so if the stderr output of the child process was sufficiently
large then libguestfs would deadlock (possibly this can be triggered
from a malicious filesystem too).

This patch series first reverts these commits, then adds a simpler way
to run a child process without waiting (modelled on popen()), allows
stderr errors to be captured, then reimplements copy-in/copy-out using
these new internal APIs.

Note that the patch series breaks bisection, but I can't really think
of a clearer way to reorganize it that would preserve bisection.

Rich.

This reverts commit db055262d4a859cf5d38fef235a117e1fd0b7aff.
---
 src/command.c          | 43 +++++--------------------------------------
 src/guestfs-internal.h |  2 +-
 2 files changed, 6 insertions(+), 39 deletions(-)

diff --git a/src/command.c b/src/command.c
index 5b45e62..a92012d 100644
--- a/src/command.c
+++ b/src/command.c
@@ -154,9 +154,6 @@ struct command
 
   /* Optional child limits. */
   struct child_rlimits *child_rlimits;
-
-  /* Optional stdin forwarding to the child. */
-  int infd;
 };
 
 /* Create a new command handle. */
@@ -171,7 +168,6 @@ guestfs_int_new_command (guestfs_h *g)
   cmd->close_files = true;
   cmd->errorfd = -1;
   cmd->outfd = -1;
-  cmd->infd = -1;
   return cmd;
 }
 
@@ -413,14 +409,12 @@ debug_command (struct command *cmd)
 }
 
 static int
-run_command (struct command *cmd, bool get_stdin_fd, bool get_stdout_fd,
-             bool get_stderr_fd)
+run_command (struct command *cmd, bool get_stdout_fd, bool get_stderr_fd)
 {
   struct sigaction sa;
   int i, fd, max_fd, r;
   int errorfd[2] = { -1, -1 };
   int outfd[2] = { -1, -1 };
-  int infd[2] = { -1, -1 };
   char status_string[80];
 #ifdef HAVE_SETRLIMIT
   struct child_rlimits *child_rlimit;
@@ -430,14 +424,6 @@ run_command (struct command *cmd, bool get_stdin_fd, bool
get_stdout_fd,
   get_stdout_fd = get_stdout_fd || cmd->stdout_callback != NULL;
   get_stderr_fd = get_stderr_fd || cmd->capture_errors;
 
-  /* Set up a pipe to forward the stdin to the command. */
-  if (get_stdin_fd) {
-    if (pipe2 (infd, O_CLOEXEC) == -1) {
-      perrorf (cmd->g, "pipe2");
-      goto error;
-    }
-  }
-
   /* Set up a pipe to capture command output and send it to the error log. */
   if (get_stderr_fd) {
     if (pipe2 (errorfd, O_CLOEXEC) == -1) {
@@ -476,13 +462,6 @@ run_command (struct command *cmd, bool get_stdin_fd, bool
get_stdout_fd,
       outfd[0] = -1;
     }
 
-    if (get_stdin_fd) {
-      close (infd[0]);
-      infd[0] = -1;
-      cmd->infd = infd[1];
-      infd[1] = -1;
-    }
-
     return 0;
   }
 
@@ -501,12 +480,6 @@ run_command (struct command *cmd, bool get_stdin_fd, bool
get_stdout_fd,
     close (outfd[1]);
   }
 
-  if (get_stdin_fd) {
-    close (infd[1]);
-    dup2 (infd[0], 0);
-    close (infd[0]);
-  }
-
   if (cmd->stderr_to_stdout)
     dup2 (1, 2);
 
@@ -720,7 +693,7 @@ guestfs_int_cmd_run (struct command *cmd)
   if (cmd->g->verbose)
     debug_command (cmd);
 
-  if (run_command (cmd, false, false, false) == -1)
+  if (run_command (cmd, false, false) == -1)
     return -1;
 
   if (loop (cmd) == -1)
@@ -730,7 +703,7 @@ guestfs_int_cmd_run (struct command *cmd)
 }
 
 /* Fork, run the command, and returns the pid of the command,
- * and its stdin, stdout and stderr file descriptors.
+ * and its stdout and stderr file descriptors.
  *
  * Returns the exit status.  Test it using WIF* macros.
  *
@@ -738,21 +711,18 @@ guestfs_int_cmd_run (struct command *cmd)
  */
 int
 guestfs_int_cmd_run_async (struct command *cmd, pid_t *pid,
-                         int *stdin_fd, int *stdout_fd, int *stderr_fd)
+                           int *stdout_fd, int *stderr_fd)
 {
   finish_command (cmd);
 
   if (cmd->g->verbose)
     debug_command (cmd);
 
-  if (run_command (cmd, stdin_fd != NULL, stdout_fd != NULL,
-                   stderr_fd != NULL) == -1)
+  if (run_command (cmd, stdout_fd != NULL, stderr_fd != NULL) == -1)
     return -1;
 
   if (pid)
     *pid = cmd->pid;
-  if (stdin_fd)
-    *stdin_fd = cmd->infd;
   if (stdout_fd)
     *stdout_fd = cmd->outfd;
   if (stderr_fd)
@@ -803,9 +773,6 @@ guestfs_int_cmd_close (struct command *cmd)
   if (cmd->outfd >= 0)
     close (cmd->outfd);
 
-  if (cmd->infd >= 0)
-    close (cmd->infd);
-
   free (cmd->outbuf.buffer);
 
   if (cmd->pid > 0)
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index f0eb28a..665dc93 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -878,7 +878,7 @@ extern void guestfs_int_cmd_clear_capture_errors (struct
command *);
 extern void guestfs_int_cmd_clear_close_files (struct command *);
 extern void guestfs_int_cmd_set_child_callback (struct command *,
cmd_child_callback child_callback, void *data);
 extern int guestfs_int_cmd_run (struct command *);
-extern int guestfs_int_cmd_run_async (struct command *, pid_t *pid, int
*stdin_fd, int *stdout_fd, int *stderr_fd);
+extern int guestfs_int_cmd_run_async (struct command *, pid_t *pid, int
*stdout_fd, int *stderr_fd);
 extern int guestfs_int_cmd_wait (struct command *);
 extern void guestfs_int_cmd_close (struct command *);
 
-- 
2.5.0

This reverts commit c5d3d198dcbceac928f430e2a9450ca58d6c0890.
---
 src/command.c          | 64 +++++++-------------------------------------------
 src/guestfs-internal.h |  3 ---
 2 files changed, 9 insertions(+), 58 deletions(-)

diff --git a/src/command.c b/src/command.c
index a92012d..126cc49 100644
--- a/src/command.c
+++ b/src/command.c
@@ -409,7 +409,7 @@ debug_command (struct command *cmd)
 }
 
 static int
-run_command (struct command *cmd, bool get_stdout_fd, bool get_stderr_fd)
+run_command (struct command *cmd)
 {
   struct sigaction sa;
   int i, fd, max_fd, r;
@@ -421,11 +421,8 @@ run_command (struct command *cmd, bool get_stdout_fd, bool
get_stderr_fd)
   struct rlimit rlimit;
 #endif
 
-  get_stdout_fd = get_stdout_fd || cmd->stdout_callback != NULL;
-  get_stderr_fd = get_stderr_fd || cmd->capture_errors;
-
   /* Set up a pipe to capture command output and send it to the error log. */
-  if (get_stderr_fd) {
+  if (cmd->capture_errors) {
     if (pipe2 (errorfd, O_CLOEXEC) == -1) {
       perrorf (cmd->g, "pipe2");
       goto error;
@@ -433,7 +430,7 @@ run_command (struct command *cmd, bool get_stdout_fd, bool
get_stderr_fd)
   }
 
   /* Set up a pipe to capture stdout for the callback. */
-  if (get_stdout_fd) {
+  if (cmd->stdout_callback) {
     if (pipe2 (outfd, O_CLOEXEC) == -1) {
       perrorf (cmd->g, "pipe2");
       goto error;
@@ -448,14 +445,14 @@ run_command (struct command *cmd, bool get_stdout_fd, bool
get_stderr_fd)
 
   /* In parent, return to caller. */
   if (cmd->pid > 0) {
-    if (get_stderr_fd) {
+    if (cmd->capture_errors) {
       close (errorfd[1]);
       errorfd[1] = -1;
       cmd->errorfd = errorfd[0];
       errorfd[0] = -1;
     }
 
-    if (get_stdout_fd) {
+    if (cmd->stdout_callback) {
       close (outfd[1]);
       outfd[1] = -1;
       cmd->outfd = outfd[0];
@@ -466,15 +463,15 @@ run_command (struct command *cmd, bool get_stdout_fd, bool
get_stderr_fd)
   }
 
   /* Child process. */
-  if (get_stderr_fd) {
+  if (cmd->capture_errors) {
     close (errorfd[0]);
-    if (!get_stdout_fd)
+    if (!cmd->stdout_callback)
       dup2 (errorfd[1], 1);
     dup2 (errorfd[1], 2);
     close (errorfd[1]);
   }
 
-  if (get_stdout_fd) {
+  if (cmd->stdout_callback) {
     close (outfd[0]);
     dup2 (outfd[1], 1);
     close (outfd[1]);
@@ -693,7 +690,7 @@ guestfs_int_cmd_run (struct command *cmd)
   if (cmd->g->verbose)
     debug_command (cmd);
 
-  if (run_command (cmd, false, false) == -1)
+  if (run_command (cmd) == -1)
     return -1;
 
   if (loop (cmd) == -1)
@@ -702,49 +699,6 @@ guestfs_int_cmd_run (struct command *cmd)
   return wait_command (cmd);
 }
 
-/* Fork, run the command, and returns the pid of the command,
- * and its stdout and stderr file descriptors.
- *
- * Returns the exit status.  Test it using WIF* macros.
- *
- * On error: Calls error(g) and returns -1.
- */
-int
-guestfs_int_cmd_run_async (struct command *cmd, pid_t *pid,
-                           int *stdout_fd, int *stderr_fd)
-{
-  finish_command (cmd);
-
-  if (cmd->g->verbose)
-    debug_command (cmd);
-
-  if (run_command (cmd, stdout_fd != NULL, stderr_fd != NULL) == -1)
-    return -1;
-
-  if (pid)
-    *pid = cmd->pid;
-  if (stdout_fd)
-    *stdout_fd = cmd->outfd;
-  if (stderr_fd)
-    *stderr_fd = cmd->errorfd;
-
-  return 0;
-}
-
-/* Wait for the command to finish.
- *
- * The command MUST have been started with guestfs_int_cmd_run_async.
- *
- * Returns the exit status.  Test it using WIF* macros.
- *
- * On error: Calls error(g) and returns -1.
- */
-int
-guestfs_int_cmd_wait (struct command *cmd)
-{
-  return wait_command (cmd);
-}
-
 void
 guestfs_int_cmd_close (struct command *cmd)
 {
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 665dc93..22e8e06 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -20,7 +20,6 @@
 #define GUESTFS_INTERNAL_H_
 
 #include <stdbool.h>
-#include <sys/types.h>
 
 #include <libintl.h>
 
@@ -878,8 +877,6 @@ extern void guestfs_int_cmd_clear_capture_errors (struct
command *);
 extern void guestfs_int_cmd_clear_close_files (struct command *);
 extern void guestfs_int_cmd_set_child_callback (struct command *,
cmd_child_callback child_callback, void *data);
 extern int guestfs_int_cmd_run (struct command *);
-extern int guestfs_int_cmd_run_async (struct command *, pid_t *pid, int
*stdout_fd, int *stderr_fd);
-extern int guestfs_int_cmd_wait (struct command *);
 extern void guestfs_int_cmd_close (struct command *);
 
 #ifdef HAVE_ATTRIBUTE_CLEANUP
-- 
2.5.0

---
 src/command.c     | 14 +++++++-------
 src/copy-in-out.c |  6 ++++--
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/command.c b/src/command.c
index 126cc49..893653a 100644
--- a/src/command.c
+++ b/src/command.c
@@ -276,8 +276,8 @@ guestfs_int_cmd_add_string_quoted (struct command *cmd,
const char *str)
  */
 void
 guestfs_int_cmd_set_stdout_callback (struct command *cmd,
-                                   cmd_stdout_callback stdout_callback,
-                                   void *stdout_data, unsigned flags)
+                                     cmd_stdout_callback stdout_callback,
+                                     void *stdout_data, unsigned flags)
 {
   cmd->stdout_callback = stdout_callback;
   cmd->stdout_data = stdout_data;
@@ -334,8 +334,8 @@ guestfs_int_cmd_clear_close_files (struct command *cmd)
  */
 void
 guestfs_int_cmd_set_child_callback (struct command *cmd,
-                                  cmd_child_callback child_callback,
-                                  void *data)
+                                    cmd_child_callback child_callback,
+                                    void *data)
 {
   cmd->child_callback = child_callback;
   cmd->child_callback_data = data;
@@ -550,8 +550,8 @@ run_command (struct command *cmd)
       _exit (WEXITSTATUS (r));
     fprintf (stderr, "%s\n",
              guestfs_int_exit_status_to_string (r, cmd->string.str,
-                                              status_string,
-                                              sizeof status_string));
+                                                status_string,
+                                                sizeof status_string));
     _exit (EXIT_FAILURE);
 
   case COMMAND_STYLE_NOT_SELECTED:
@@ -613,7 +613,7 @@ loop (struct command *cmd)
       n = read (cmd->errorfd, buf, sizeof buf);
       if (n > 0)
         guestfs_int_call_callbacks_message (cmd->g, GUESTFS_EVENT_APPLIANCE,
-                                          buf, n);
+                                            buf, n);
       else if (n == 0) {
         if (close (cmd->errorfd) == -1)
           perrorf (cmd->g, "close: errorfd");
diff --git a/src/copy-in-out.c b/src/copy-in-out.c
index dc9e7b7..975c217 100644
--- a/src/copy-in-out.c
+++ b/src/copy-in-out.c
@@ -36,7 +36,8 @@
 static int split_path (guestfs_h *g, char *buf, size_t buf_size, const char
*path, const char **dirname, const char **basename);
 
 int
-guestfs_impl_copy_in (guestfs_h *g, const char *localpath, const char
*remotedir)
+guestfs_impl_copy_in (guestfs_h *g,
+                      const char *localpath, const char *remotedir)
 {
   CLEANUP_CMD_CLOSE struct command *cmd = guestfs_int_new_command (g);
   int fd;
@@ -118,7 +119,8 @@ child_setup (guestfs_h *g, void *data)
 }
 
 int
-guestfs_impl_copy_out (guestfs_h *g, const char *remotepath, const char
*localdir)
+guestfs_impl_copy_out (guestfs_h *g,
+                       const char *remotepath, const char *localdir)
 {
   struct stat statbuf;
   int r;
-- 
2.5.0

Allow this useful function to be called from elsewhere.

This commit is almost, but not completely refactoring.  I made a minor
change to the function so that it \0-terminates the returned data,
which is convenient for some callers.
---
 po/POTFILES            |  1 +
 src/Makefile.am        |  1 +
 src/guestfs-internal.h |  3 ++
 src/inspect-icon.c     | 72 +++------------------------------------
 src/whole-file.c       | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 101 insertions(+), 67 deletions(-)
 create mode 100644 src/whole-file.c

diff --git a/po/POTFILES b/po/POTFILES
index bb68183..9d92690 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -349,6 +349,7 @@ src/structs-free.c
 src/test-utils.c
 src/tmpdirs.c
 src/utils.c
+src/whole-file.c
 test-tool/test-tool.c
 v2v/domainxml-c.c
 v2v/kvmuid-c.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 38a4f50..f5ecaa6 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -126,6 +126,7 @@ libguestfs_la_SOURCES = \
 	structs-copy.c \
 	structs-free.c \
 	tmpdirs.c \
+	whole-file.c \
 	libguestfs.syms
 
 libguestfs_la_CPPFLAGS = \
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 22e8e06..75ca71c 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -754,6 +754,9 @@ extern int guestfs_int_lazy_make_tmpdir (guestfs_h *g);
 extern void guestfs_int_remove_tmpdir (guestfs_h *g);
 extern void guestfs_int_recursive_remove_dir (guestfs_h *g, const char *dir);
 
+/* whole-file.c */
+extern int guestfs_int_read_whole_file (guestfs_h *g, const char *filename,
char **data_r, size_t *size_r);
+
 /* drives.c */
 extern size_t guestfs_int_checkpoint_drives (guestfs_h *g);
 extern void guestfs_int_rollback_drives (guestfs_h *g, size_t);
diff --git a/src/inspect-icon.c b/src/inspect-icon.c
index 33d0aa7..15c721b 100644
--- a/src/inspect-icon.c
+++ b/src/inspect-icon.c
@@ -25,7 +25,6 @@
 #include <unistd.h>
 #include <fcntl.h>
 #include <string.h>
-#include <sys/stat.h>
 #include <errno.h>
 #include <sys/wait.h>
 
@@ -43,8 +42,6 @@
 #define CAN_DO_WINDOWS 1
 #endif
 
-static int read_whole_file (guestfs_h *g, const char *filename, char **data_r,
size_t *size_r);
-
 /* All these icon_*() functions return the same way.  One of:
  *
  *   ret == NULL:
@@ -272,7 +269,7 @@ get_png (guestfs_h *g, struct inspect_fs *fs, const char
*filename,
     return NOT_FOUND;
 
   /* Successfully passed checks and downloaded.  Read it into memory. */
-  if (read_whole_file (g, local, &ret, size_r) == -1)
+  if (guestfs_int_read_whole_file (g, local, &ret, size_r) == -1)
     return NULL;
 
   return ret;
@@ -419,7 +416,7 @@ icon_cirros (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
     return NOT_FOUND;
 
   /* Read it into memory. */
-  if (read_whole_file (g, pngfile, &ret, size_r) == -1)
+  if (guestfs_int_read_whole_file (g, pngfile, &ret, size_r) == -1)
     return NULL;
 
   return ret;
@@ -488,7 +485,7 @@ icon_windows_xp (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
   if (!WIFEXITED (r) || WEXITSTATUS (r) != 0)
     return NOT_FOUND;
 
-  if (read_whole_file (g, pngfile, &ret, size_r) == -1)
+  if (guestfs_int_read_whole_file (g, pngfile, &ret, size_r) == -1)
     return NULL;
 
   return ret;
@@ -540,7 +537,7 @@ icon_windows_7 (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
   if (!WIFEXITED (r) || WEXITSTATUS (r) != 0)
     return NOT_FOUND;
 
-  if (read_whole_file (g, pngfile, &ret, size_r) == -1)
+  if (guestfs_int_read_whole_file (g, pngfile, &ret, size_r) == -1)
     return NULL;
 
   return ret;
@@ -577,7 +574,7 @@ icon_windows_8 (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
   if (filename_downloaded == NULL)
     return NOT_FOUND;
 
-  if (read_whole_file (g, filename_downloaded, &ret, size_r) == -1)
+  if (guestfs_int_read_whole_file (g, filename_downloaded, &ret, size_r) ==
-1)
     return NULL;
 
   return ret;
@@ -606,62 +603,3 @@ icon_windows (guestfs_h *g, struct inspect_fs *fs, size_t
*size_r)
 }
 
 #endif /* CAN_DO_WINDOWS */
-
-/* Read the whole file into a memory buffer and return it.  The file
- * should be a regular, local, trusted file.
- */
-static int
-read_whole_file (guestfs_h *g, const char *filename,
-                 char **data_r, size_t *size_r)
-{
-  int fd;
-  char *data;
-  off_t size;
-  off_t n;
-  ssize_t r;
-  struct stat statbuf;
-
-  fd = open (filename, O_RDONLY|O_CLOEXEC);
-  if (fd == -1) {
-    perrorf (g, "open: %s", filename);
-    return -1;
-  }
-
-  if (fstat (fd, &statbuf) == -1) {
-    perrorf (g, "stat: %s", filename);
-    close (fd);
-    return -1;
-  }
-
-  size = statbuf.st_size;
-  data = safe_malloc (g, size);
-
-  n = 0;
-  while (n < size) {
-    r = read (fd, &data[n], size - n);
-    if (r == -1) {
-      perrorf (g, "read: %s", filename);
-      free (data);
-      close (fd);
-      return -1;
-    }
-    if (r == 0) {
-      error (g, _("read: %s: unexpected end of file"), filename);
-      free (data);
-      close (fd);
-      return -1;
-    }
-    n += r;
-  }
-
-  if (close (fd) == -1) {
-    perrorf (g, "close: %s", filename);
-    free (data);
-    return -1;
-  }
-
-  *data_r = data;
-  *size_r = size;
-
-  return 0;
-}
diff --git a/src/whole-file.c b/src/whole-file.c
new file mode 100644
index 0000000..38050d9
--- /dev/null
+++ b/src/whole-file.c
@@ -0,0 +1,91 @@
+/* libguestfs
+ * Copyright (C) 2011-2015 Red Hat Inc.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <fcntl.h>
+#include <sys/stat.h>
+
+#include "guestfs.h"
+#include "guestfs-internal.h"
+
+/* Read the whole file into a memory buffer and return it.  The file
+ * should be a regular, local, trusted file.
+ */
+int
+guestfs_int_read_whole_file (guestfs_h *g, const char *filename,
+                             char **data_r, size_t *size_r)
+{
+  int fd;
+  char *data;
+  off_t size;
+  off_t n;
+  ssize_t r;
+  struct stat statbuf;
+
+  fd = open (filename, O_RDONLY|O_CLOEXEC);
+  if (fd == -1) {
+    perrorf (g, "open: %s", filename);
+    return -1;
+  }
+
+  if (fstat (fd, &statbuf) == -1) {
+    perrorf (g, "stat: %s", filename);
+    close (fd);
+    return -1;
+  }
+
+  size = statbuf.st_size;
+  data = safe_malloc (g, size + 1);
+
+  n = 0;
+  while (n < size) {
+    r = read (fd, &data[n], size - n);
+    if (r == -1) {
+      perrorf (g, "read: %s", filename);
+      free (data);
+      close (fd);
+      return -1;
+    }
+    if (r == 0) {
+      error (g, _("read: %s: unexpected end of file"), filename);
+      free (data);
+      close (fd);
+      return -1;
+    }
+    n += r;
+  }
+
+  if (close (fd) == -1) {
+    perrorf (g, "close: %s", filename);
+    free (data);
+    return -1;
+  }
+
+  /* For convenience of callers, \0-terminate the data. */
+  data[size] = '\0';
+
+  *data_r = data;
+  if (size_r != NULL)
+    *size_r = size;
+
+  return 0;
+}
-- 
2.5.0

Anything that doesn't involve pipes [parent-child communication] is
moved into the run_child function, so we can reuse the same code when
writing the new pipe APIs (see following commit).  Anything involving
setting up pipes stays in the existing run_command function.

This is just refactoring.
---
 src/command.c | 50 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/src/command.c b/src/command.c
index 893653a..8fb0714 100644
--- a/src/command.c
+++ b/src/command.c
@@ -408,18 +408,13 @@ debug_command (struct command *cmd)
   }
 }
 
+static void run_child (struct command *cmd) __attribute__((noreturn));
+
 static int
 run_command (struct command *cmd)
 {
-  struct sigaction sa;
-  int i, fd, max_fd, r;
   int errorfd[2] = { -1, -1 };
   int outfd[2] = { -1, -1 };
-  char status_string[80];
-#ifdef HAVE_SETRLIMIT
-  struct child_rlimits *child_rlimit;
-  struct rlimit rlimit;
-#endif
 
   /* Set up a pipe to capture command output and send it to the error log. */
   if (cmd->capture_errors) {
@@ -480,6 +475,33 @@ run_command (struct command *cmd)
   if (cmd->stderr_to_stdout)
     dup2 (1, 2);
 
+  run_child (cmd);
+  /*NOTREACHED*/
+
+ error:
+  if (errorfd[0] >= 0)
+    close (errorfd[0]);
+  if (errorfd[1] >= 0)
+    close (errorfd[1]);
+  if (outfd[0] >= 0)
+    close (outfd[0]);
+  if (outfd[1] >= 0)
+    close (outfd[1]);
+
+  return -1;
+}
+
+static void
+run_child (struct command *cmd)
+{
+  struct sigaction sa;
+  int i, fd, max_fd, r;
+  char status_string[80];
+#ifdef HAVE_SETRLIMIT
+  struct child_rlimits *child_rlimit;
+  struct rlimit rlimit;
+#endif
+
   /* Remove all signal handlers.  See the justification here:
    * https://www.redhat.com/archives/libvir-list/2008-August/msg00303.html
    * We don't mask signal handlers yet, so this isn't completely
@@ -557,19 +579,9 @@ run_command (struct command *cmd)
   case COMMAND_STYLE_NOT_SELECTED:
     abort ();
   }
+
   /*NOTREACHED*/
-
- error:
-  if (errorfd[0] >= 0)
-    close (errorfd[0]);
-  if (errorfd[1] >= 0)
-    close (errorfd[1]);
-  if (outfd[0] >= 0)
-    close (outfd[0]);
-  if (outfd[1] >= 0)
-    close (outfd[1]);
-
-  return -1;
+  abort ();
 }
 
 /* The loop which reads errors and output and directs it either
-- 
2.5.0

Roughly equivalent to popen(...,"r"|"w") the new internal
API allows
you to run a subprocess and either read its stdout, or write to its
stdin.
---
 src/command.c          | 149 +++++++++++++++++++++++++++++++++++++++++++++++++
 src/guestfs-internal.h |   3 +
 2 files changed, 152 insertions(+)

diff --git a/src/command.c b/src/command.c
index 8fb0714..0547111 100644
--- a/src/command.c
+++ b/src/command.c
@@ -133,6 +133,9 @@ struct command
   bool capture_errors;
   int errorfd;
 
+  /* When using the pipe_* APIs, stderr is pointed to a temporary file. */
+  char *error_file;
+
   /* Close file descriptors (defaults to true). */
   bool close_files;
 
@@ -711,6 +714,147 @@ guestfs_int_cmd_run (struct command *cmd)
   return wait_command (cmd);
 }
 
+/* Fork and run the command, but don't wait.  Roughly equivalent to
+ * popen (..., "r"|"w").
+ *
+ * Returns the file descriptor of the pipe, connected to stdout ("r")
+ * or stdin ("w") of the child process.
+ *
+ * After reading/writing to this pipe, call guestfs_int_cmd_pipe_wait
+ * to wait for the status of the child.
+ *
+ * Errors from the subcommand cannot be captured to the error log
+ * using this interface.  Instead the caller should call
+ * guestfs_int_cmd_get_pipe_errors (after guestfs_int_cmd_pipe_wait
+ * returns an error).
+ */
+int
+guestfs_int_cmd_pipe_run (struct command *cmd, const char *mode)
+{
+  int fd[2] = { -1, -1 };
+  int errfd = -1;
+  int r_mode;
+  int ret;
+
+  finish_command (cmd);
+
+  /* Various options cannot be used here. */
+  assert (!cmd->capture_errors);
+  assert (!cmd->stdout_callback);
+  assert (!cmd->stderr_to_stdout);
+
+  if (STREQ (mode, "r"))      r_mode = 1;
+  else if (STREQ (mode, "w")) r_mode = 0;
+  else abort ();
+
+  if (pipe2 (fd, O_CLOEXEC) == -1) {
+    perrorf (cmd->g, "pipe2");
+    goto error;
+  }
+
+  /* We can't easily capture errors from the child process, so instead
+   * we write them into a temporary file and provide a separate
+   * function for the caller to read the error messages.
+   */
+  if (guestfs_int_lazy_make_tmpdir (cmd->g) == -1)
+    goto error;
+
+  cmd->error_file +    safe_asprintf (cmd->g, "%s/cmderr.%d",
cmd->g->tmpdir, ++cmd->g->unique);
+  errfd = open (cmd->error_file,
+                O_WRONLY|O_CREAT|O_NOCTTY|O_TRUNC|O_CLOEXEC, 0600);
+  if (errfd == -1) {
+    perrorf (cmd->g, "open: %s", cmd->error_file);
+    goto error;
+  }
+
+  cmd->pid = fork ();
+  if (cmd->pid == -1) {
+    perrorf (cmd->g, "fork");
+    goto error;
+  }
+
+  /* Parent. */
+  if (cmd->pid > 0) {
+    close (errfd);
+    errfd = -1;
+
+    if (r_mode) {
+      close (fd[1]);
+      ret = fd[0];
+    }
+    else {
+      close (fd[0]);
+      ret = fd[1];
+    }
+
+    return ret;
+  }
+
+  /* Child. */
+  dup2 (errfd, 2);
+  close (errfd);
+
+  if (r_mode) {
+    close (fd[0]);
+    dup2 (fd[1], 1);
+    close (fd[1]);
+  }
+  else {
+    close (fd[1]);
+    dup2 (fd[0], 0);
+    close (fd[0]);
+  }
+
+  run_child (cmd);
+  /*NOTREACHED*/
+
+ error:
+  if (errfd >= 0)
+    close (errfd);
+  if (fd[0] >= 0)
+    close (fd[0]);
+  if (fd[1] >= 0)
+    close (fd[1]);
+  return -1;
+}
+
+/* Wait for a subprocess created by guestfs_int_cmd_pipe_run to
+ * finish.  On error (eg. failed syscall) this returns -1 and sets the
+ * error.  If the subcommand fails, then use WIF* macros to check
+ * this, and call guestfs_int_cmd_get_pipe_errors to read the error
+ * messages printed by the child.
+ */
+int
+guestfs_int_cmd_pipe_wait (struct command *cmd)
+{
+  return wait_command (cmd);
+}
+
+/* Read the error messages printed by the child.  The caller must free
+ * the returned buffer after use.
+ */
+char *
+guestfs_int_cmd_get_pipe_errors (struct command *cmd)
+{
+  char *ret;
+  size_t len;
+
+  assert (cmd->error_file != NULL);
+
+  if (guestfs_int_read_whole_file (cmd->g, cmd->error_file, &ret,
NULL) == -1)
+    return NULL;
+
+  /* If the file ends with \n characters, trim them. */
+  len = strlen (ret);
+  while (len > 0 && ret[len-1] == '\n') {
+    ret[len-1] = '\0';
+    len--;
+  }
+
+  return ret;
+}
+
 void
 guestfs_int_cmd_close (struct command *cmd)
 {
@@ -733,6 +877,11 @@ guestfs_int_cmd_close (struct command *cmd)
     break;
   }
 
+  if (cmd->error_file != NULL) {
+    unlink (cmd->error_file);
+    free (cmd->error_file);
+  }
+
   if (cmd->errorfd >= 0)
     close (cmd->errorfd);
 
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 75ca71c..9c7175f 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -881,6 +881,9 @@ extern void guestfs_int_cmd_clear_close_files (struct
command *);
 extern void guestfs_int_cmd_set_child_callback (struct command *,
cmd_child_callback child_callback, void *data);
 extern int guestfs_int_cmd_run (struct command *);
 extern void guestfs_int_cmd_close (struct command *);
+extern int guestfs_int_cmd_pipe_run (struct command *cmd, const char *mode);
+extern int guestfs_int_cmd_pipe_wait (struct command *cmd);
+extern char *guestfs_int_cmd_get_pipe_errors (struct command *cmd);
 
 #ifdef HAVE_ATTRIBUTE_CLEANUP
 #define CLEANUP_CMD_CLOSE
__attribute__((cleanup(guestfs_int_cleanup_cmd_close)))
-- 
2.5.0

Modify the copy-in/copy-out APIs to use the new guestfs_int_cmd_pipe
internal API.

This new API allows us to capture the stderr from the tar subprocess
if it fails, fixing RHBZ#1267032.  The user will now see errors like
this:

$ guestfish -N fs -m /dev/sda1 copy-in '/tmp/d/*' / : ll /
libguestfs: error: tar subprocess failed: tar: *: Cannot stat: No such file or
directory
tar: Exiting with failure status due to previous errors
---
 src/copy-in-out.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/src/copy-in-out.c b/src/copy-in-out.c
index 975c217..d5e7fb0 100644
--- a/src/copy-in-out.c
+++ b/src/copy-in-out.c
@@ -68,8 +68,10 @@ guestfs_impl_copy_in (guestfs_h *g,
   guestfs_int_cmd_add_arg (cmd, "-");
   guestfs_int_cmd_add_arg (cmd, basename);
 
-  r = guestfs_int_cmd_run_async (cmd, NULL, NULL, &fd, NULL);
-  if (r == -1)
+  guestfs_int_cmd_clear_capture_errors (cmd);
+
+  fd = guestfs_int_cmd_pipe_run (cmd, "r");
+  if (fd == -1)
     return -1;
 
   snprintf (fdbuf, sizeof fdbuf, "/dev/fd/%d", fd);
@@ -81,11 +83,16 @@ guestfs_impl_copy_in (guestfs_h *g,
     return -1;
   }
 
-  r = guestfs_int_cmd_wait (cmd);
+  r = guestfs_int_cmd_pipe_wait (cmd);
   if (r == -1)
     return -1;
-  if (!(WIFEXITED (r) && WEXITSTATUS (r) == 0))
+  if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
+    CLEANUP_FREE char *errors = guestfs_int_cmd_get_pipe_errors (cmd);
+    if (errors == NULL)
+      return -1;
+    error (g, "tar subprocess failed: %s", errors);
     return -1;
+  }
 
   return 0;
 }
@@ -190,8 +197,10 @@ guestfs_impl_copy_out (guestfs_h *g,
     guestfs_int_cmd_add_arg (cmd, "-xf");
     guestfs_int_cmd_add_arg (cmd, "-");
 
-    r = guestfs_int_cmd_run_async (cmd, NULL, &fd, NULL, NULL);
-    if (r == -1)
+    guestfs_int_cmd_clear_capture_errors (cmd);
+
+    fd = guestfs_int_cmd_pipe_run (cmd, "w");
+    if (fd == -1)
       return -1;
 
     snprintf (fdbuf, sizeof fdbuf, "/dev/fd/%d", fd);
@@ -203,11 +212,16 @@ guestfs_impl_copy_out (guestfs_h *g,
       return -1;
     }
 
-    r = guestfs_int_cmd_wait (cmd);
+    r = guestfs_int_cmd_pipe_wait (cmd);
     if (r == -1)
       return -1;
-    if (!(WIFEXITED (r) && WEXITSTATUS (r) == 0))
+    if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) {
+      CLEANUP_FREE char *errors = guestfs_int_cmd_get_pipe_errors (cmd);
+      if (errors == NULL)
+        return -1;
+      error (g, "tar subprocess failed: %s", errors);
       return -1;
+    }
   }
 
   return 0;
-- 
2.5.0

On Tuesday 29 September 2015 12:54:47 Richard W.M. Jones
wrote:
> Commits 3c27f3d91e1566854747bbe844186783fc84f3a8 and
> 1b6f0daa9ae7fcc94e389232d0c397816cda973d added an internal API for
> running commands asynchronously.  It is only used by the copy-in and
> copy-out APIs.
> 
> Unfortunately this made the command code very complex: it was almost
> impossible to redirect stderr to a file, and there were a lot of
> long-range dependencies through the file.  It was also buggy: it set
> up stderr of the child process pointing pipe, but never read from the
> pipe, so if the stderr output of the child process was sufficiently
> large then libguestfs would deadlock (possibly this can be triggered
> from a malicious filesystem too).
> 
> This patch series first reverts these commits, then adds a simpler way
> to run a child process without waiting (modelled on popen()), allows
> stderr errors to be captured, then reimplements copy-in/copy-out using
> these new internal APIs.
> 
> Note that the patch series breaks bisection, but I can't really think
> of a clearer way to reorganize it that would preserve bisection.
LGTM.

-- 
Pino Toscano