[Libguestfs] [PATCH 2/3] daemon: Fix for commands working on absolute symbolic links

[Jim Meyering]

Richard W.M. Jones wrote:
> Subject: [PATCH 2/3] daemon: Fix for commands working on absolute symbolic links (RHBZ#579608).
>
> The original idea (suggested by Al Viro) was to fork and chroot
> into the sysroot and read the file from there.  Because of the
> separate process being chrooted, absolute links would be resolved
> correctly.  The slightly modified idea is to open the file in the
> daemon process (but temporarily chrooted, so symlinks resolve
> correctly), fork, and have the subprocess just be responsible for
> copying the file.  (Strictly speaking we don't need to fork, but
> this implementation is simpler).
>
> This commit just includes the changes needed to the command*()
> functions in daemon/guestfsd.c and adds an absolute symlink to
> the test ISO for testing it.  Later commits will fix the broken
> daemon commands themselves.
> ---
>  .gitignore         |    1 +
>  daemon/daemon.h    |    4 ++-
>  daemon/guestfsd.c  |   99 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  images/Makefile.am |    4 ++
>  4 files changed, 103 insertions(+), 5 deletions(-)
>
> diff --git a/.gitignore b/.gitignore
> index 382cbcf..a79edca 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -103,6 +103,7 @@ images/100kallspaces
>  images/100kallzeroes
>  images/100krandom
>  images/10klines
> +images/abssymlink
>  images/hello.b64
>  images/initrd
>  images/initrd-x86_64.img
> diff --git a/daemon/daemon.h b/daemon/daemon.h
> index 81a9f36..977dfdc 100644
> --- a/daemon/daemon.h
> +++ b/daemon/daemon.h
> @@ -53,7 +53,9 @@ extern void free_stringslen (char **argv, int len);
>  #define commandv(out,err,argv) commandvf((out),(err),0,(argv))
>  #define commandrv(out,err,argv) commandrvf((out),(err),0,(argv))
>
> -#define COMMAND_FLAG_FOLD_STDOUT_ON_STDERR 1
> +#define COMMAND_FLAG_FD_MASK                   (1024-1)
> +#define COMMAND_FLAG_FOLD_STDOUT_ON_STDERR     1024
> +#define COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN 2048

This looks fine.  A couple of stylistic nits below.

The 1024 seems like it's related to the setting of _POSIX_OPEN_MAX
(1024) on Linux-based systems these days.  It might be good
to add something like this, just in case it's ever increased:

  verify (_POSIX_OPEN_MAX <= COMMAND_FLAG_FOLD_STDOUT_ON_STDERR);


>  extern int commandf (char **stdoutput, char **stderror, int flags,
>                       const char *name, ...);
> diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c
> index be79300..4832842 100644
> --- a/daemon/guestfsd.c
> +++ b/daemon/guestfsd.c
> @@ -715,6 +715,14 @@ commandvf (char **stdoutput, char **stderror, int flags,
>   * error messages in the *stderror buffer.  If using this flag,
>   * you should pass stdoutput as NULL because nothing could ever be
>   * captured in that buffer.
> + *
> + * COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN: For running external
> + * commands on chrooted files correctly (see RHBZ#579608) specifying
> + * this flag causes another process to be forked which chroots into
> + * sysroot and just copies the input file to stdin of the specified
> + * command.  The file descriptor is ORed with the flags, and that file
> + * descriptor is always closed by this function.  See hexdump.c for an
> + * example of usage.
>   */
>  int
>  commandrvf (char **stdoutput, char **stderror, int flags,
> @@ -722,7 +730,9 @@ commandrvf (char **stdoutput, char **stderror, int flags,
>  {
>    int so_size = 0, se_size = 0;
>    int so_fd[2], se_fd[2];
> -  pid_t pid;
> +  int flag_copy_stdin = flags & COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN;
> +  int stdin_fd[2] = { -1, -1 };
> +  pid_t pid, stdin_pid = -1;
>    int r, quit, i;
>    fd_set rset, rset2;
>    char buf[256];
> @@ -752,15 +762,28 @@ commandrvf (char **stdoutput, char **stderror, int flags,
>      abort ();
>    }
>
> +  if (flag_copy_stdin) {
> +    if (pipe (stdin_fd) == -1) {
> +      perror ("pipe");

No big deal, but something to keep in mind...
It's slightly better to use a function like glibc's error here,
since it flushes stderr.  Especially when the next thing that
happens is an abort or kill, it can happen that the desired output
is never produced.

> +      abort ();
> +    }
> +  }
> +
>    pid = fork ();
>    if (pid == -1) {
>      perror ("fork");
>      abort ();
>    }
>
> -  if (pid == 0) {		/* Child process. */
> +  if (pid == 0) {		/* Child process running the command. */
>      close (0);
> -    open ("/dev/null", O_RDONLY); /* Set stdin to /dev/null (ignore failure) */
> +    if (flag_copy_stdin) {
> +      dup2 (stdin_fd[0], 0);
> +      close (stdin_fd[0]);
> +      close (stdin_fd[1]);
> +    } else
> +      /* Set stdin to /dev/null (ignore failure) */
> +      open ("/dev/null", O_RDONLY);

IMHO, it's safer to put curly braces around multi-line blocks
like this "else" one, even though it's obviously a single stmt.
Here's some justification:
http://libvirt.org/git/?p=libvirt.git;a=blob;f=HACKING#l108

I'm a little sensitive about this, since I introduced a bug
into libvirt recently due to a similar situation.
My penance was to write that addition to HACKING ;-)