thr3ads.net - Libguestfs - [Libguestfs] [PATCH] daemon: improve internal commandrvf [Dec 2015]

If this information is useful, please help other people find it:
Share via:

Pino Toscano

2015-Dec-02 13:00 UTC

[Libguestfs] [PATCH] daemon: improve internal commandrvf

- add a flag to request chroot for the process, which is done only as
  very last (before chdir) operation before exec'ing the process in the
  child: this avoids using CHROOT_IN & CHROOT_OUT around command*
  invocations, and reduces the code spent in chroot mode
- add failure checks for dup2 and open done in child, not proceeding to
  executing the process if they fail
- open /dev/null without O_CLOEXEC, so it stays available for the
  exec'ed process, and thus we don't need to provide an own fd for stdin

Followup of commit fd2f175ee79d29df101d353e2f380db27b19553a, thanks also
to the notes and hints provided by Mateusz Guzik.
---
 daemon/command.c  | 17 ++---------------
 daemon/daemon.h   |  1 +
 daemon/guestfsd.c | 39 +++++++++++++++++++++++++++++++--------
 3 files changed, 34 insertions(+), 23 deletions(-)

diff --git a/daemon/command.c b/daemon/command.c
index 27a4d0c..c4efa5b 100644
--- a/daemon/command.c
+++ b/daemon/command.c
@@ -244,7 +244,7 @@ do_command (char *const *argv)
 {
   char *out;
   CLEANUP_FREE char *err = NULL;
-  int r, dev_null_fd, flags;
+  int r, flags;
   CLEANUP_BIND_STATE struct bind_state bind_state = { .mounted = false };
   CLEANUP_RESOLVER_STATE struct resolver_state resolver_state      { .mounted =
false };
@@ -261,17 +261,6 @@ do_command (char *const *argv)
     return NULL;
   }
 
-  /* Provide /dev/null as stdin for the command, since we want
-   * to make sure processes have an open stdin, and it is not
-   * possible to rely on the guest to provide it (Linux guests
-   * get /dev dynamically populated at runtime by udev).
-   */
-  dev_null_fd = open ("/dev/null", O_RDONLY|O_CLOEXEC);
-  if (dev_null_fd == -1) {
-    reply_with_perror ("/dev/null");
-    return NULL;
-  }
-
   if (bind_mount (&bind_state) == -1)
     return NULL;
   if (enable_network) {
@@ -279,11 +268,9 @@ do_command (char *const *argv)
       return NULL;
   }
 
-  flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | dev_null_fd;
+  flags = COMMAND_FLAG_DO_CHROOT;
 
-  CHROOT_IN;
   r = commandvf (&out, &err, flags, (const char * const *) argv);
-  CHROOT_OUT;
 
   free_bind_state (&bind_state);
   free_resolver_state (&resolver_state);
diff --git a/daemon/daemon.h b/daemon/daemon.h
index 7fbb2a2..af6f68c 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -128,6 +128,7 @@ extern char **empty_list (void);
 #define COMMAND_FLAG_FD_MASK                   (1024-1)
 #define COMMAND_FLAG_FOLD_STDOUT_ON_STDERR     1024
 #define COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN 2048
+#define COMMAND_FLAG_DO_CHROOT                 4096
 
 extern int commandf (char **stdoutput, char **stderror, int flags,
                      const char *name, ...) __attribute__((sentinel));
diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
index 0a29aa6..47245f7 100644
--- a/daemon/guestfsd.c
+++ b/daemon/guestfsd.c
@@ -932,21 +932,44 @@ commandrvf (char **stdoutput, char **stderror, int flags,
     signal (SIGPIPE, SIG_DFL);
     close (0);
     if (flag_copy_stdin) {
-      dup2 (flag_copy_fd, STDIN_FILENO);
+      if (dup2 (flag_copy_fd, STDIN_FILENO) == -1) {
+        perror ("dup2/flag_copy_fd");
+        _exit (EXIT_FAILURE);
+      }
     } else {
-      /* Set stdin to /dev/null (ignore failure) */
-      ignore_value (open ("/dev/null", O_RDONLY|O_CLOEXEC));
+      /* Set stdin to /dev/null. */
+      if (open ("/dev/null", O_RDONLY) == -1) {
+        perror ("open(/dev/null)");
+        _exit (EXIT_FAILURE);
+      }
     }
     close (so_fd[PIPE_READ]);
     close (se_fd[PIPE_READ]);
-    if (!(flags & COMMAND_FLAG_FOLD_STDOUT_ON_STDERR))
-      dup2 (so_fd[PIPE_WRITE], STDOUT_FILENO);
-    else
-      dup2 (se_fd[PIPE_WRITE], STDOUT_FILENO);
-    dup2 (se_fd[PIPE_WRITE], STDERR_FILENO);
+    if (!(flags & COMMAND_FLAG_FOLD_STDOUT_ON_STDERR)) {
+      if (dup2 (so_fd[PIPE_WRITE], STDOUT_FILENO) == -1) {
+        perror ("dup2/so_fd[PIPE_WRITE]");
+        _exit (EXIT_FAILURE);
+      }
+    } else {
+      if (dup2 (se_fd[PIPE_WRITE], STDOUT_FILENO) == -1) {
+        perror ("dup2/se_fd[PIPE_WRITE]");
+        _exit (EXIT_FAILURE);
+      }
+    }
+    if (dup2 (se_fd[PIPE_WRITE], STDERR_FILENO) == -1) {
+      perror ("dup2/se_fd[PIPE_WRITE]");
+      _exit (EXIT_FAILURE);
+    }
     close (so_fd[PIPE_WRITE]);
     close (se_fd[PIPE_WRITE]);
 
+    if (flags & COMMAND_FLAG_DO_CHROOT && sysroot_len > 0) {
+      if (chroot (sysroot) == -1) {
+        perror ("chroot in sysroot");
+        _exit (EXIT_FAILURE);
+      }
+    }
+
     ignore_value (chdir ("/"));
 
     execvp (argv[0], (void *) argv);
-- 
2.1.0

Mateusz Guzik

2015-Dec-02 16:44 UTC

head link

Re: [Libguestfs] [PATCH] daemon: improve internal commandrvf

On Wed, Dec 02, 2015 at 02:00:57PM +0100, Pino Toscano
wrote:> - add a flag to request chroot for the process, which is done only as
>   very last (before chdir) operation before exec'ing the process in the
>   child: this avoids using CHROOT_IN & CHROOT_OUT around command*
>   invocations, and reduces the code spent in chroot mode
> - add failure checks for dup2 and open done in child, not proceeding to
>   executing the process if they fail
> - open /dev/null without O_CLOEXEC, so it stays available for the
>   exec'ed process, and thus we don't need to provide an own fd for
stdin
> 
> Followup of commit fd2f175ee79d29df101d353e2f380db27b19553a, thanks also
> to the notes and hints provided by Mateusz Guzik.
Looks good, thanks. I only have optional nits.
> ---
>  daemon/command.c  | 17 ++---------------
>  daemon/daemon.h   |  1 +
>  daemon/guestfsd.c | 39 +++++++++++++++++++++++++++++++--------
>  3 files changed, 34 insertions(+), 23 deletions(-)
> 
> diff --git a/daemon/command.c b/daemon/command.c
> index 27a4d0c..c4efa5b 100644
> --- a/daemon/command.c
> +++ b/daemon/command.c
> @@ -244,7 +244,7 @@ do_command (char *const *argv)
>  {
>    char *out;
>    CLEANUP_FREE char *err = NULL;
> -  int r, dev_null_fd, flags;
> +  int r, flags;
>    CLEANUP_BIND_STATE struct bind_state bind_state = { .mounted = false };
>    CLEANUP_RESOLVER_STATE struct resolver_state resolver_state >      {
.mounted = false };
> @@ -261,17 +261,6 @@ do_command (char *const *argv)
>      return NULL;
>    }
>  
> -  /* Provide /dev/null as stdin for the command, since we want
> -   * to make sure processes have an open stdin, and it is not
> -   * possible to rely on the guest to provide it (Linux guests
> -   * get /dev dynamically populated at runtime by udev).
> -   */
> -  dev_null_fd = open ("/dev/null", O_RDONLY|O_CLOEXEC);
> -  if (dev_null_fd == -1) {
> -    reply_with_perror ("/dev/null");
> -    return NULL;
> -  }
> -
>    if (bind_mount (&bind_state) == -1)
>      return NULL;
>    if (enable_network) {
> @@ -279,11 +268,9 @@ do_command (char *const *argv)
>        return NULL;
>    }
>  
> -  flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | dev_null_fd;
> +  flags = COMMAND_FLAG_DO_CHROOT;
>  
> -  CHROOT_IN;
>    r = commandvf (&out, &err, flags, (const char * const *) argv);
> -  CHROOT_OUT;
>  
>    free_bind_state (&bind_state);
>    free_resolver_state (&resolver_state);
> diff --git a/daemon/daemon.h b/daemon/daemon.h
> index 7fbb2a2..af6f68c 100644
> --- a/daemon/daemon.h
> +++ b/daemon/daemon.h
> @@ -128,6 +128,7 @@ extern char **empty_list (void);
>  #define COMMAND_FLAG_FD_MASK                   (1024-1)
>  #define COMMAND_FLAG_FOLD_STDOUT_ON_STDERR     1024
>  #define COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN 2048
> +#define COMMAND_FLAG_DO_CHROOT                 4096
>  
Any eason for having this decimal? The standard thing is to use hex.
>  extern int commandf (char **stdoutput, char **stderror, int flags,
>                       const char *name, ...) __attribute__((sentinel));
> diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
> index 0a29aa6..47245f7 100644
> --- a/daemon/guestfsd.c
> +++ b/daemon/guestfsd.c
> @@ -932,21 +932,44 @@ commandrvf (char **stdoutput, char **stderror, int
flags,
>      signal (SIGPIPE, SIG_DFL);
>      close (0);
>      if (flag_copy_stdin) {
> -      dup2 (flag_copy_fd, STDIN_FILENO);
> +      if (dup2 (flag_copy_fd, STDIN_FILENO) == -1) {
> +        perror ("dup2/flag_copy_fd");
> +        _exit (EXIT_FAILURE);
> +      }
close(0) explicitly assumes that stdin is 0, which is fine, but dup2
uses STDIN_FILENO (which itself is also fine), but you should stick to
one scheme.
>      } else {
> -      /* Set stdin to /dev/null (ignore failure) */
> -      ignore_value (open ("/dev/null", O_RDONLY|O_CLOEXEC));
> +      /* Set stdin to /dev/null. */
> +      if (open ("/dev/null", O_RDONLY) == -1) {
> +        perror ("open(/dev/null)");
> +        _exit (EXIT_FAILURE);
> +      }
>      }
>      close (so_fd[PIPE_READ]);
>      close (se_fd[PIPE_READ]);
> -    if (!(flags & COMMAND_FLAG_FOLD_STDOUT_ON_STDERR))
> -      dup2 (so_fd[PIPE_WRITE], STDOUT_FILENO);
> -    else
> -      dup2 (se_fd[PIPE_WRITE], STDOUT_FILENO);
> -    dup2 (se_fd[PIPE_WRITE], STDERR_FILENO);
> +    if (!(flags & COMMAND_FLAG_FOLD_STDOUT_ON_STDERR)) {
> +      if (dup2 (so_fd[PIPE_WRITE], STDOUT_FILENO) == -1) {
> +        perror ("dup2/so_fd[PIPE_WRITE]");
> +        _exit (EXIT_FAILURE);
> +      }
> +    } else {
> +      if (dup2 (se_fd[PIPE_WRITE], STDOUT_FILENO) == -1) {
> +        perror ("dup2/se_fd[PIPE_WRITE]");
> +        _exit (EXIT_FAILURE);
> +      }
> +    }
> +    if (dup2 (se_fd[PIPE_WRITE], STDERR_FILENO) == -1) {
> +      perror ("dup2/se_fd[PIPE_WRITE]");
> +      _exit (EXIT_FAILURE);
> +    }
>      close (so_fd[PIPE_WRITE]);
>      close (se_fd[PIPE_WRITE]);
>  
> +    if (flags & COMMAND_FLAG_DO_CHROOT && sysroot_len > 0)
{
> +      if (chroot (sysroot) == -1) {
> +        perror ("chroot in sysroot");
> +        _exit (EXIT_FAILURE);
> +      }
> +    }
> +
>      ignore_value (chdir ("/"));
>  
This could also be error-checked.
>      execvp (argv[0], (void *) argv);
> -- 
> 2.1.0
> 
-- 
Mateusz Guzik

Pino Toscano

2015-Dec-02 18:35 UTC

head link

Re: [Libguestfs] [PATCH] daemon: improve internal commandrvf

On Wednesday 02 December 2015 17:44:13 Mateusz Guzik
wrote:> On Wed, Dec 02, 2015 at 02:00:57PM +0100, Pino Toscano wrote:
> > - add a flag to request chroot for the process, which is done only as
> >   very last (before chdir) operation before exec'ing the process
in the
> >   child: this avoids using CHROOT_IN & CHROOT_OUT around command*
> >   invocations, and reduces the code spent in chroot mode
> > - add failure checks for dup2 and open done in child, not proceeding
to
> >   executing the process if they fail
> > - open /dev/null without O_CLOEXEC, so it stays available for the
> >   exec'ed process, and thus we don't need to provide an own fd
for stdin
> > 
> > Followup of commit fd2f175ee79d29df101d353e2f380db27b19553a, thanks
also
> > to the notes and hints provided by Mateusz Guzik.
> 
> Looks good, thanks. I only have optional nits.
> 
> > ---
> >  daemon/command.c  | 17 ++---------------
> >  daemon/daemon.h   |  1 +
> >  daemon/guestfsd.c | 39 +++++++++++++++++++++++++++++++--------
> >  3 files changed, 34 insertions(+), 23 deletions(-)
> > 
> > diff --git a/daemon/command.c b/daemon/command.c
> > index 27a4d0c..c4efa5b 100644
> > --- a/daemon/command.c
> > +++ b/daemon/command.c
> > @@ -244,7 +244,7 @@ do_command (char *const *argv)
> >  {
> >    char *out;
> >    CLEANUP_FREE char *err = NULL;
> > -  int r, dev_null_fd, flags;
> > +  int r, flags;
> >    CLEANUP_BIND_STATE struct bind_state bind_state = { .mounted =
false };
> >    CLEANUP_RESOLVER_STATE struct resolver_state resolver_state >
>      { .mounted = false };
> > @@ -261,17 +261,6 @@ do_command (char *const *argv)
> >      return NULL;
> >    }
> >  
> > -  /* Provide /dev/null as stdin for the command, since we want
> > -   * to make sure processes have an open stdin, and it is not
> > -   * possible to rely on the guest to provide it (Linux guests
> > -   * get /dev dynamically populated at runtime by udev).
> > -   */
> > -  dev_null_fd = open ("/dev/null", O_RDONLY|O_CLOEXEC);
> > -  if (dev_null_fd == -1) {
> > -    reply_with_perror ("/dev/null");
> > -    return NULL;
> > -  }
> > -
> >    if (bind_mount (&bind_state) == -1)
> >      return NULL;
> >    if (enable_network) {
> > @@ -279,11 +268,9 @@ do_command (char *const *argv)
> >        return NULL;
> >    }
> >  
> > -  flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | dev_null_fd;
> > +  flags = COMMAND_FLAG_DO_CHROOT;
> >  
> > -  CHROOT_IN;
> >    r = commandvf (&out, &err, flags, (const char * const *)
argv);
> > -  CHROOT_OUT;
> >  
> >    free_bind_state (&bind_state);
> >    free_resolver_state (&resolver_state);
> > diff --git a/daemon/daemon.h b/daemon/daemon.h
> > index 7fbb2a2..af6f68c 100644
> > --- a/daemon/daemon.h
> > +++ b/daemon/daemon.h
> > @@ -128,6 +128,7 @@ extern char **empty_list (void);
> >  #define COMMAND_FLAG_FD_MASK                   (1024-1)
> >  #define COMMAND_FLAG_FOLD_STDOUT_ON_STDERR     1024
> >  #define COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN 2048
> > +#define COMMAND_FLAG_DO_CHROOT                 4096
> >  
> 
> Any eason for having this decimal? The standard thing is to use hex.
Mostly that the current code was using that style already.
> >  extern int commandf (char **stdoutput, char **stderror, int flags,
> >                       const char *name, ...)
__attribute__((sentinel));
> > diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
> > index 0a29aa6..47245f7 100644
> > --- a/daemon/guestfsd.c
> > +++ b/daemon/guestfsd.c
> > @@ -932,21 +932,44 @@ commandrvf (char **stdoutput, char **stderror,
int flags,
> >      signal (SIGPIPE, SIG_DFL);
> >      close (0);
> >      if (flag_copy_stdin) {
> > -      dup2 (flag_copy_fd, STDIN_FILENO);
> > +      if (dup2 (flag_copy_fd, STDIN_FILENO) == -1) {
> > +        perror ("dup2/flag_copy_fd");
> > +        _exit (EXIT_FAILURE);
> > +      }
> 
> close(0) explicitly assumes that stdin is 0, which is fine, but dup2
> uses STDIN_FILENO (which itself is also fine), but you should stick to
> one scheme.
Like in the case above, this is what the code before was doing.
I could change it, but in a later patch, as I want to avoid mixing style
and functional changes in the same patch.
> >      ignore_value (chdir ("/"));
> >  
> 
> This could also be error-checked.
Indeed, fixed locally:

+    if (chdir ("/") == -1) {
+      perror ("chdir");
+      _exit (EXIT_FAILURE);
+    }

Thanks for review,
-- 
Pino Toscano

Richard W.M. Jones

2015-Dec-15 12:43 UTC

head link

Re: [Libguestfs] [PATCH] daemon: improve internal commandrvf

On Wed, Dec 02, 2015 at 02:00:57PM +0100, Pino Toscano
wrote:> +        perror ("open(/dev/null)");
ACK (with the other reviewer's comments), but can you change this to
perror ("open: /dev/null") to make it consistent with other error
messages.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Reasonably Related Threads

Search for more apparently analagous threads

Libguestfs - Dec 2015 - [PATCH] daemon: improve internal commandrvf

[Libguestfs] [PATCH] daemon: improve internal commandrvf

Re: [Libguestfs] [PATCH] daemon: improve internal commandrvf

Re: [Libguestfs] [PATCH] daemon: improve internal commandrvf

Re: [Libguestfs] [PATCH] daemon: improve internal commandrvf

Reasonably Related Threads