[lxc-devel] [PATCH v3 4/4] uniformly nullify std fds

[Serge Hallyn]

Quoting Robert Vogelgesang (vogel at users.sourceforge.net):
> Hi Tycho,
> 
> thank you for the updated patch, but you missed my intention, see below.
> 
> On Tue, Jun 09, 2015 at 10:09:28AM -0600, Tycho Andersen wrote:
> > In various places throughout the code, we want to "nullify" the std fds,
> > opening them to /dev/null or zero or so. Instead, let's unify this code and
> > do it in such a way that Coverity (probably) won't complain.
> > 
> > v2: use /dev/null for stdin as well
> > v3: add a comment about use of C's short circuiting
> > 
> > Reported-by: Coverity
> > Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
> > ---
> >  src/lxc/bdev.c         |  8 ++------
> >  src/lxc/lxccontainer.c | 24 +++++++++++-------------
> >  src/lxc/monitor.c      |  8 ++------
> >  src/lxc/start.c        | 10 ++--------
> >  src/lxc/utils.c        | 16 ++++++++++++++++
> >  src/lxc/utils.h        |  1 +
> >  6 files changed, 34 insertions(+), 33 deletions(-)
> > 
> > diff --git a/src/lxc/bdev.c b/src/lxc/bdev.c
> > index 53465b1..520652c 100644
> > --- a/src/lxc/bdev.c
> > +++ b/src/lxc/bdev.c
> > @@ -224,12 +224,8 @@ static int do_mkfs(const char *path, const char *fstype)
> >  
> >  	// If the file is not a block device, we don't want mkfs to ask
> >  	// us about whether to proceed.
> > -	close(0);
> > -	close(1);
> > -	close(2);
> > -	open("/dev/zero", O_RDONLY);
> > -	open("/dev/null", O_RDWR);
> > -	open("/dev/null", O_RDWR);
> > +	if (null_stdfds() < 0)
> > +		exit(1);
> >  	execlp("mkfs", "mkfs", "-t", fstype, path, NULL);
> >  	exit(1);
> >  }
> > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > index 445cc22..a0dd2a2 100644
> > --- a/src/lxc/lxccontainer.c
> > +++ b/src/lxc/lxccontainer.c
> > @@ -722,12 +722,10 @@ static bool do_lxcapi_start(struct lxc_container *c, int useinit, char * const a
> >  			return false;
> >  		}
> >  		lxc_check_inherited(conf, true, -1);
> > -		close(0);
> > -		close(1);
> > -		close(2);
> > -		open("/dev/zero", O_RDONLY);
> > -		open("/dev/null", O_RDWR);
> > -		open("/dev/null", O_RDWR);
> > +		if (null_stdfds() < 0) {
> > +			ERROR("failed to close fds");
> > +			return false;
> > +		}
> >  		setsid();
> >  	} else {
> >  		if (!am_single_threaded()) {
> > @@ -978,13 +976,13 @@ static bool create_run_template(struct lxc_container *c, char *tpath, bool quiet
> >  		char **newargv;
> >  		struct lxc_conf *conf = c->lxc_conf;
> >  
> > -		if (quiet) {
> > -			close(0);
> > -			close(1);
> > -			close(2);
> > -			open("/dev/zero", O_RDONLY);
> > -			open("/dev/null", O_RDWR);
> > -			open("/dev/null", O_RDWR);
> > +		/*
> > +		 * Here, we're taking advantage of C's short circuiting of
> > +		 * conditions: we should only fail if quiet is set and
> > +		 * null_stdfds fails.
> > +		 */
> > +		if (quiet && null_stdfds() < 0) {
> > +			exit(1);
> 
> My concern is not about someone not understanding short circuiting.
> There should be a warning against reversing the order of the two parts.
> 
> Short circuiting is rather common, but that "quiet" means to close
> fds, is unusual and not obvious.
> 
> If someone would change that to
> 
> > +		if (null_stdfds() < 0 && quiet) {

Then we would hopefully reject the patch.

thanks,
-serge