Richard W.M. Jones
2010-May-06 21:01 UTC
[Libguestfs] [PATCH 0/3] Fix resolving absolute symlinks (RHBZ#579608).
This patchset just fixes the 'hexdump' command as an example. The important part of the patch is #2 since that shows the approach I want to take to fix this. Rich. -- Richard Jones, Virtualization Group, Red Hat people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. et.redhat.com/~rjones/virt-top
Richard W.M. Jones
2010-May-06 21:03 UTC
[Libguestfs] [PATCH 1/3] daemon: Change command to abort() on resource problems.
-- Richard Jones, Virtualization Group, Red Hat people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. et.redhat.com/~rjones/virt-p2v -------------- next part -------------->From fd62c985a26bfd0cac27735a47ce7fd7b068fdb4 Mon Sep 17 00:00:00 2001From: Richard Jones <rjones at redhat.com> Date: Thu, 6 May 2010 20:32:09 +0100 Subject: [PATCH 1/3] daemon: Change command to abort() on resource problems. The comment in the code describes it thus: /* Note: abort is used in a few places along the error paths early * in this function. This is because (a) cleaning up correctly is * very complex at these places and (b) abort is used when a * resource problems is indicated which would be due to much more * serious issues - eg. memory or file descriptor leaks. We * wouldn't expect fork(2) or pipe(2) to fail in normal * circumstances. */ --- daemon/guestfsd.c | 19 ++++++++++++------- 1 files changed, 12 insertions(+), 7 deletions(-) diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index 03a975a..be79300 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -1,5 +1,5 @@ /* libguestfs - the guestfsd daemon - * Copyright (C) 2009 Red Hat Inc. + * Copyright (C) 2009-2010 Red Hat Inc. * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -738,19 +738,24 @@ commandrvf (char **stdoutput, char **stderror, int flags, printf ("\n"); } + /* Note: abort is used in a few places along the error paths early + * in this function. This is because (a) cleaning up correctly is + * very complex at these places and (b) abort is used when a + * resource problems is indicated which would be due to much more + * serious issues - eg. memory or file descriptor leaks. We + * wouldn't expect fork(2) or pipe(2) to fail in normal + * circumstances. + */ + if (pipe (so_fd) == -1 || pipe (se_fd) == -1) { perror ("pipe"); - return -1; + abort (); } pid = fork (); if (pid == -1) { perror ("fork"); - close (so_fd[0]); - close (so_fd[1]); - close (se_fd[0]); - close (se_fd[1]); - return -1; + abort (); } if (pid == 0) { /* Child process. */ -- 1.6.6.1
Richard W.M. Jones
2010-May-06 21:04 UTC
[Libguestfs] [PATCH 2/3] daemon: Fix for commands working on absolute symbolic links
-- Richard Jones, Virtualization Group, Red Hat people.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. et.redhat.com/~rjones/virt-p2v -------------- next part -------------->From 2b645e9aa7ac820e2b0477598b53bee5803b7292 Mon Sep 17 00:00:00 2001From: Richard Jones <rjones at redhat.com> Date: Thu, 6 May 2010 21:36:24 +0100 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 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"); + 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); close (so_fd[0]); close (se_fd[0]); if (!(flags & COMMAND_FLAG_FOLD_STDOUT_ON_STDERR)) @@ -773,7 +796,57 @@ commandrvf (char **stdoutput, char **stderror, int flags, execvp (argv[0], (void *) argv); perror (argv[0]); - _exit (1); + _exit (EXIT_FAILURE); + } + + if (flag_copy_stdin) { + int fd = flags & COMMAND_FLAG_FD_MASK; + + stdin_pid = fork (); + if (stdin_pid == -1) { + perror ("fork"); + abort (); + } + + if (stdin_pid == 0) { /* Child process copying stdin. */ + close (so_fd[0]); + close (so_fd[1]); + close (se_fd[0]); + close (se_fd[1]); + + close (1); + dup2 (stdin_fd[1], 1); + close (stdin_fd[0]); + close (stdin_fd[1]); + + if (chroot (sysroot) == -1) { + perror ("chroot"); + _exit (EXIT_FAILURE); + } + + ssize_t n; + char buffer[BUFSIZ]; + while ((n = read (fd, buffer, sizeof buffer)) > 0) { + if (xwrite (1, buffer, n) == -1) + _exit (EXIT_FAILURE); + } + + if (n == -1) { + perror ("read"); + _exit (EXIT_FAILURE); + } + + if (close (fd) == -1) { + perror ("close"); + _exit (EXIT_FAILURE); + } + + _exit (EXIT_SUCCESS); + } + + close (fd); + close (stdin_fd[0]); + close (stdin_fd[1]); } /* Parent process. */ @@ -796,6 +869,7 @@ commandrvf (char **stdoutput, char **stderror, int flags, close (so_fd[0]); close (se_fd[0]); waitpid (pid, NULL, 0); + if (stdin_pid >= 0) waitpid (stdin_pid, NULL, 0); return -1; } @@ -876,6 +950,23 @@ commandrvf (char **stdoutput, char **stderror, int flags, } } + if (flag_copy_stdin) { + /* Check copy process didn't fail. */ + if (waitpid (stdin_pid, &r, 0) != stdin_pid) { + perror ("waitpid"); + kill (pid, 9); + waitpid (pid, NULL, 0); + return -1; + } + + if (!WIFEXITED (r) || WEXITSTATUS (r) != 0) { + fprintf (stderr, "failed copying from input file, see earlier messages\n"); + kill (pid, 9); + waitpid (pid, NULL, 0); + return -1; + } + } + /* Get the exit status of the command. */ if (waitpid (pid, &r, 0) != pid) { perror ("waitpid"); diff --git a/images/Makefile.am b/images/Makefile.am index 7f35b75..771018f 100644 --- a/images/Makefile.am +++ b/images/Makefile.am @@ -73,6 +73,7 @@ images_files_build = \ $(builddir)/100kallspaces \ $(builddir)/100krandom \ $(builddir)/10klines \ + $(builddir)/abssymlink \ $(builddir)/hello.b64 \ $(builddir)/initrd \ $(builddir)/initrd-x86_64.img \ @@ -119,6 +120,9 @@ $(builddir)/10klines: done > $@-t mv $@-t $@ +$(builddir)/abssymlink: + ln -sf /10klines $@ + $(builddir)/hello.b64: echo "hello" | base64 > $@ -- 1.6.6.1
Richard W.M. Jones
2010-May-06 21:04 UTC
[Libguestfs] [PATCH 3/3] daemon: Fix hexdump to work on absolute symbolic links
-- Richard Jones, Virtualization Group, Red Hat people.redhat.com/~rjones libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. et.redhat.com/~rjones/libguestfs See what it can do: et.redhat.com/~rjones/libguestfs/recipes.html -------------- next part -------------->From f88f5e838fc884368fd56af7a66b1ed9a639fbdd Mon Sep 17 00:00:00 2001From: Richard Jones <rjones at redhat.com> Date: Thu, 6 May 2010 21:55:32 +0100 Subject: [PATCH 3/3] daemon: Fix hexdump to work on absolute symbolic links (RHBZ#579608). --- daemon/hexdump.c | 18 +++++++++++------- src/generator.ml | 5 ++++- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/daemon/hexdump.c b/daemon/hexdump.c index 1b33eeb..332af12 100644 --- a/daemon/hexdump.c +++ b/daemon/hexdump.c @@ -21,6 +21,8 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <unistd.h> +#include <fcntl.h> #include "daemon.h" #include "actions.h" @@ -28,18 +30,20 @@ char * do_hexdump (const char *path) { - char *buf; - int r; + int fd, flags, r; char *out, *err; - buf = sysroot_path (path); - if (!buf) { - reply_with_perror ("malloc"); + CHROOT_IN; + fd = open (path, O_RDONLY); + CHROOT_OUT; + + if (fd == -1) { + perror (path); return NULL; } - r = command (&out, &err, "hexdump", "-C", buf, NULL); - free (buf); + flags = COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN | fd; + r = commandf (&out, &err, flags, "hexdump", "-C", NULL); if (r == -1) { reply_with_error ("%s: %s", path, err); free (err); diff --git a/src/generator.ml b/src/generator.ml index cbed94b..5b479e7 100755 --- a/src/generator.ml +++ b/src/generator.ml @@ -2474,7 +2474,10 @@ The returned strings are transcoded to UTF-8."); * commands to segfault. *) InitISOFS, Always, TestRun ( - [["hexdump"; "/100krandom"]])], + [["hexdump"; "/100krandom"]]); + (* Test for RHBZ#579608, absolute symbolic links. *) + InitISOFS, Always, TestRun ( + [["hexdump"; "/abssymlink"]])], "dump a file in hexadecimal", "\ This runs C<hexdump -C> on the given C<path>. The result is -- 1.6.6.1
Maybe Matching Threads
- [PATCH] daemon: always provide stdin when running chroot commands (RHBZ#1280029)
- [PATCH] daemon: resolve paths for ll and llz
- RFC: copy-attributes command
- [PATCH libguestfs v2 0/3] daemon: Fix various commands which break on NTFS-3g compressed files.
- Re: [PATCH] daemon: scrub-file: resolve the path before calling scrub (RHBZ#1099490).