similar to: [PATCH] daemon: always provide stdin when running chroot commands (RHBZ#1280029)

- 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

On Thu, Nov 19, 2015 at 05:38:25PM +0100, Pino Toscano wrote: > 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. This does not

On Tue, Dec 01, 2015 at 04:16:57PM +0100, Pino Toscano wrote: > On Tuesday 01 December 2015 15:59:56 Mateusz Guzik wrote: > > I would argue that /dev has to be at least partially populated for anything > > that gets executed in the chroot. At the very least special nodes like null, > > zero and {u,}random are needed. > > We do not assume anything about guests, which

On Tuesday 01 December 2015 15:59:56 Mateusz Guzik wrote: > On Thu, Nov 19, 2015 at 05:38:25PM +0100, Pino Toscano wrote: > > 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

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

On Thu, Nov 19, 2015 at 05:38:25PM +0100, Pino Toscano wrote: > 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. > --- >

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

Improve readability of commandrvf() by replacing bare int values for file descriptors with their symbolic names STD{IN,OUT,ERR}_FILENO. Also add PIPE_READ and PIPE_WRITE for referencing relevant ends of a pipe. --- daemon/guestfsd.c | 79 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 42 insertions(+), 37 deletions(-) diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c

Simply a file rename, no other change. --- daemon/Makefile.am | 2 +- daemon/command.c | 319 ----------------------------------------------------- daemon/sh.c | 319 +++++++++++++++++++++++++++++++++++++++++++++++++++++ po/POTFILES | 2 +- 4 files changed, 321 insertions(+), 321 deletions(-) delete mode 100644 daemon/command.c create mode 100644 daemon/sh.c diff --git

When the COMMAND_FLAG_FOLD_STDOUT_ON_STDERR flag is passed to command*(), indicate that as stdout=e in debugging message. --- daemon/command.c | 6 ++++-- 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,

On Tue, Dec 01, 2015 at 03:59:56PM +0100, Mateusz Guzik wrote: > CHROOT_IN/OUT around commandvf are definitely problematic. chroot should be > done in the child, which also removes the need to chroot out in the > parent. The CHROOT_IN/OUT business does need to be rewritten. Every instance where we currently do something like: CHROOT_IN; r = stat (fd, &statbuf); CHROOT_OUT

Commit 72afcf450a78b7e58f65b4a7aaf94d71cd25fca5 was partially incorrect. If the guest userspace is expecting /selinux to exist, then we should bind-mount /sys/fs/selinux from the appliance kernel there. --- daemon/command.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/daemon/command.c b/daemon/command.c index 1aa1a52..939bf87 100644 --- a/daemon/command.c

Do SELinux relabelling properly.

Allows more sharing between the daemon and the inspection program. --- daemon/Makefile.am | 2 + daemon/cleanups.c | 80 ++++++++++ daemon/cleanups.h | 47 ++++++ daemon/command.c | 436 +++++++++++++++++++++++++++++++++++++++++++++++++++++ daemon/command.h | 41 +++++ daemon/daemon.h | 47 +----- daemon/guestfsd.c | 392 ----------------------------------------------- po/POTFILES

Causes a compiler warning to be emitted if you omit the trailing NULL argument. --- daemon/daemon.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/daemon/daemon.h b/daemon/daemon.h index 8f932d2..df1ba3a 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -100,9 +100,9 @@ extern char **split_lines (char *str); #define COMMAND_FLAG_CHROOT_COPY_FILE_TO_STDIN 2048

On Tue, Dec 01, 2015 at 04:58:11PM +0000, Richard W.M. Jones wrote: > On Tue, Dec 01, 2015 at 03:59:56PM +0100, Mateusz Guzik wrote: > > CHROOT_IN/OUT around commandvf are definitely problematic. chroot should be > > done in the child, which also removes the need to chroot out in the > > parent. > > The CHROOT_IN/OUT business does need to be rewritten. Every >

daemon.c does just a chroot, without chdir. The result is that pwd does not work correctly (it causes fs/dcache.c:prepend_unreachable() to add the unreachable string). A workaround is to add "cd /" before each sh command. ><fs> mount /dev/sda2 / ><fs> sh "cd / ; chroot / ; /bin/pwd" / ><fs> sh "/bin/pwd" (unreachable)/ ><fs> sh

On Tuesday 28 January 2014 16:21:09 Richard W.M. Jones wrote: > Commit 72afcf450a78b7e58f65b4a7aaf94d71cd25fca5 was partially > incorrect. If the guest userspace is expecting /selinux to exist, > then we should bind-mount /sys/fs/selinux from the appliance kernel > there. > --- > daemon/command.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) >

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.