search for: flag_copy_stdin

..., this is the major and minor. * This is so we can ignore this device from the point of view of the * user, eg. in guestfs_list_devices and many other places. @@ -834,22 +838,22 @@ commandrvf (char **stdoutput, char **stderror, int flags, signal (SIGPIPE, SIG_DFL); close (0); if (flag_copy_stdin) { - dup2 (stdin_fd[0], 0); - close (stdin_fd[0]); - close (stdin_fd[1]); + dup2 (stdin_fd[PIPE_READ], STDIN_FILENO); + close (stdin_fd[PIPE_READ]); + close (stdin_fd[PIPE_WRITE]); } else { /* Set stdin to /dev/null (ignore failure) */ ignore_value (...

...//bugzilla.redhat.com/show_bug.cgi?id=1280029 the problem was that the target program was being executed in a chroot which did not have /dev/null populated, hence the open in commandrvf failed and the process was left without fd 0. commandrvf does the following in the child: close (0); if (flag_copy_stdin) { dup2 (flag_copy_fd, STDIN_FILENO); } else { /* Set stdin to /dev/null (ignore failure) */ ignore_value (open ("/dev/null", O_RDONLY|O_CLOEXEC)); } [..] execvp (argv[0], (void *) argv); First observation is that regardless of whether this open("/d...

...chrooted, it can open /dev/null. So you fork, open /dev/null and only then proceed to chroot. But as noted earlier, unpopulated /dev seems to be the real bug here. > > Current patch seems to work around shortcomings of the current API. > > > > Side content: > > if (flag_copy_stdin) close (flag_copy_fd); > > waitpid (pid, NULL, 0); > > return -1; > > This is done only in the main "while (quit < 2)" loop, and only on > error there, which will return with -1. > > > but some lines below there is: > > if (flag_copy_...

...id=1280029 the problem was that > the target program was being executed in a chroot which did not have > /dev/null populated, hence the open in commandrvf failed and the process > was left without fd 0. > > commandrvf does the following in the child: > close (0); > if (flag_copy_stdin) { > dup2 (flag_copy_fd, STDIN_FILENO); > } else { > /* Set stdin to /dev/null (ignore failure) */ > ignore_value (open ("/dev/null", O_RDONLY|O_CLOEXEC)); > } > [..] > execvp (argv[0], (void *) argv); > > First observation is...

When running commands in the mounted guest (using the "command" API, and APIs based on it), provide the /dev/null from the appliance as open fd for stdin. Commands usually assume stdin is open if they didn't close it explicitly, so this should avoid crashes or misbehavings due to that. --- daemon/command.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)

..., and that file + * descriptor is always closed by this function. See hexdump.c for an + * example of usage. + */ +int +commandrvf (char **stdoutput, char **stderror, unsigned flags, + char const* const *argv) +{ + size_t so_size = 0, se_size = 0; + int so_fd[2], se_fd[2]; + unsigned flag_copy_stdin = flags & COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN; + int flag_copy_fd = (int) (flags & COMMAND_FLAG_FD_MASK); + pid_t pid; + int r, quit, i; + fd_set rset, rset2; + char buf[256]; + char *p; + + if (stdoutput) *stdoutput = NULL; + if (stderror) *stderror = NULL; + + if (verbose) { +...

...-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/daemon/command.c b/daemon/command.c index 73fce56..2423a4e 100644 --- a/daemon/command.c +++ b/daemon/command.c @@ -185,6 +185,7 @@ commandrvf (char **stdoutput, char **stderror, unsigned flags, int so_fd[2], se_fd[2]; unsigned flag_copy_stdin = flags & COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN; int flag_copy_fd = (int) (flags & COMMAND_FLAG_FD_MASK); + unsigned flag_out_on_err = flags & COMMAND_FLAG_FOLD_STDOUT_ON_STDERR; pid_t pid; int r, quit, i; fd_set rset, rset2; @@ -196,7 +197,8 @@ commandrvf (char **stdoutpu...

..., ...) __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&q...

...iff --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_FILEN...

This is a more working version. Inspection (partially) succeeds on a real guest this time :-) You can test it out on a real guest (in this case, a CentOS disk image located at /tmp/centos-6.img) by doing: $ ./run guestfish -v -x -a /tmp/centos-6.img ><fs> run ><fs> debug sh "guestfs-inspection --verbose" which will print lots of debugging, and at the end the

For background on this change, see: https://rwmj.wordpress.com/2015/12/06/inspection-now-with-added-prolog/ v2 was previously posted here: https://www.redhat.com/archives/libguestfs/2015-December/msg00038.html To test this patch series on a real guest, you can do: $ ./run guestfish -v -x -a /var/tmp/centos-6.img ><fs> run ><fs> debug sh "guestfs-inspection

These two patches are probably not completely independent, but separating them is a lot of work. With *both* patches applied, all the tests and extra-tests pass. That's no guarantee however that there isn't a mistake, so I don't think this patch is a candidate for the 1.16 branch, until it's had a lot more testing in development. Rich.