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

[Robert Vogelgesang]

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 null_stdfds() is called independently of whether quiet is set or
not, and that would clearly be a change of behaviour.  This could lead
to lost error messages in cases when someone is actually looking
for such messages.

Thank you for your patience with a nitpicker. :-)

	Robert


>  		}
>  
>  		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..42dd38f 100644
> --- a/src/lxc/utils.c
> +++ b/src/lxc/utils.c
> @@ -1445,3 +1445,19 @@ domount:
>  	INFO("Mounted /proc in container for security transition");
>  	return 1;
>  }
> +
> +int null_stdfds(void)
> +{
> +	int fd;
> +
> +	fd = open("/dev/null", O_RDWR);
> +	if (fd < 0)
> +		return -1;
> +
> +	dup2(fd, 0);
> +	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