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

Robert Vogelgesang vogel at users.sourceforge.net
Tue Jun 9 14:15:07 UTC 2015


Hi Tycho,

On Tue, Jun 09, 2015 at 05:48:52AM -0600, Tycho Andersen wrote:
> On Tue, Jun 09, 2015 at 11:48:05AM +0200, Robert Vogelgesang wrote:
> > Hi,
> > 
> > On Mon, Jun 08, 2015 at 07:59:54PM -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.
> > > 
> > > Reported-by: Coverity
> > > Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
> > > ---
> > >  src/lxc/bdev.c         |  8 ++------
> > >  src/lxc/lxccontainer.c | 19 ++++++-------------
> > >  src/lxc/monitor.c      |  8 ++------
> > >  src/lxc/start.c        | 10 ++--------
> > >  src/lxc/utils.c        | 22 ++++++++++++++++++++++
> > >  src/lxc/utils.h        |  1 +
> > >  6 files changed, 35 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..18fceeb 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,8 @@ 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);
> > > +		if (quiet && null_stdfds() < 0) {
> > > +			exit(1);
> > 
> > Here, maybe a comment should be added to say that "quiet" means we
> > should close the fds by calling null_stdfds(), and not the other way
> > around, that we should exit if null_stdfds() fails.
> 
> Alternatively I can nest the ifs, but doesn't the order of the
> arguments in the && imply this?

Yes, the order does imply this, but a comment would make that explicit
for the next reviewer of the code.

> 
> > >  		}
> > >  
> > >  		src = c->lxc_conf->rootfs.path;
> > > diff --git a/src/lxc/monitor.c b/src/lxc/monitor.c
> > > index 0e56f75..144b16e 100644
> > > --- a/src/lxc/monitor.c
> > > +++ b/src/lxc/monitor.c
> > > @@ -329,12 +329,8 @@ int lxc_monitord_spawn(const char *lxcpath)
> > >  		exit(EXIT_FAILURE);
> > >  	}
> > >  	lxc_check_inherited(NULL, true, pipefd[1]);
> > > -	close(0);
> > > -	close(1);
> > > -	close(2);
> > > -	open("/dev/null", O_RDONLY);
> > > -	open("/dev/null", O_RDWR);
> > > -	open("/dev/null", O_RDWR);
> > > +	if (null_stdfds() < 0)
> > > +		exit(EXIT_FAILURE);
> > >  	close(pipefd[0]);
> > >  	sprintf(pipefd_str, "%d", pipefd[1]);
> > >  	execvp(args[0], args);
> > > diff --git a/src/lxc/start.c b/src/lxc/start.c
> > > index 71cd9ef..6eded61 100644
> > > --- a/src/lxc/start.c
> > > +++ b/src/lxc/start.c
> > > @@ -762,14 +762,8 @@ static int do_start(void *data)
> > >  
> > >  	close(handler->sigfd);
> > >  
> > > -	if (handler->backgrounded) {
> > > -		close(0);
> > > -		close(1);
> > > -		close(2);
> > > -		open("/dev/zero", O_RDONLY);
> > > -		open("/dev/null", O_RDWR);
> > > -		open("/dev/null", O_RDWR);
> > > -	}
> > > +	if (handler->backgrounded && null_stdfds() < 0)
> > > +		goto out_warn_father;
> > >  
> > >  	/* after this call, we are in error because this
> > >  	 * ops should not return as it execs */
> > > diff --git a/src/lxc/utils.c b/src/lxc/utils.c
> > > index 467bc1b..da9548f 100644
> > > --- a/src/lxc/utils.c
> > > +++ b/src/lxc/utils.c
> > > @@ -1445,3 +1445,25 @@ domount:
> > >  	INFO("Mounted /proc in container for security transition");
> > >  	return 1;
> > >  }
> > > +
> > > +int null_stdfds(void)
> > > +{
> > > +	int fd;
> > > +
> > > +	fd = open("/dev/zero", O_RDONLY);
> > > +	if (fd < 0)
> > > +		return -1;
> > > +
> > > +	dup2(fd, 0);
> > 
> > Well, I've seen the code above redirecting fd 0 to /dev/zero, but I think
> > this is potentially dangerous, and /dev/null should be used instead for
> > fd 0, too.
> > 
> > With /dev/zero, any reader process would get an endless stream of
> > zeros, which could result in unexpected hangs or crashes, depending on
> > what the reader process does.  Reading from /dev/null is safe, because
> > it returns immediately, with an empty buffer.
> 
> Sure, I can resend with /dev/null instead; I thought about that, but I
> was worried I was missing something r.e. /dev/zero instead.

Well, it's hard for me to think an example where you'd need a redirection
of stdin to /dev/zero instead of /dev/null.  Sure, /dev/zero is useful
in some cases, but not if you don't want any input.  So, yes, please
resend with /dev/null instead of /dev/zero.

	Robert

> 
> Tycho
> 
> > 	Robert
> > 
> > 
> > > +	close(fd);
> > > +
> > > +	fd = open("/dev/null", O_WRONLY);
> > > +	if (fd < 0)
> > > +		return -1;
> > > +
> > > +	dup2(fd, 1);
> > > +	dup2(fd, 2);
> > > +	close(fd);
> > > +
> > > +	return 0;
> > > +}
> > > diff --git a/src/lxc/utils.h b/src/lxc/utils.h
> > > index 6bd05e0..ee12dde 100644
> > > --- a/src/lxc/utils.h
> > > +++ b/src/lxc/utils.h
> > > @@ -280,4 +280,5 @@ int is_dir(const char *path);
> > >  char *get_template_path(const char *t);
> > >  int setproctitle(char *title);
> > >  int mount_proc_if_needed(const char *rootfs);
> > > +int null_stdfds(void);
> > >  #endif /* __LXC_UTILS_H */
> > > -- 
> > > 2.1.4
> > > 
> > > _______________________________________________
> > > lxc-devel mailing list
> > > lxc-devel at lists.linuxcontainers.org
> > > http://lists.linuxcontainers.org/listinfo/lxc-devel
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel


More information about the lxc-devel mailing list