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

[Serge Hallyn]

Quoting Tycho Andersen (tycho.andersen at canonical.com):
> 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
> v4: axe comment, check errors on dup2, s/quiet/need_null_stdfds
> 
> Reported-by: Coverity
> Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>

Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>

However, your patch drew my eye to something...  I'll send a patch
for that in a bit.  (So I'll apply this now so that we don't
conflict)

> ---
>  src/lxc/bdev.c         |  8 ++------
>  src/lxc/lxccontainer.c | 21 +++++++--------------
>  src/lxc/monitor.c      |  8 ++------
>  src/lxc/start.c        | 10 ++--------
>  src/lxc/utils.c        | 21 +++++++++++++++++++++
>  src/lxc/utils.h        |  1 +
>  6 files changed, 35 insertions(+), 34 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..7708a8c 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()) {
> @@ -956,7 +954,7 @@ static char *lxcbasename(char *path)
>  	return p;
>  }
>  
> -static bool create_run_template(struct lxc_container *c, char *tpath, bool quiet,
> +static bool create_run_template(struct lxc_container *c, char *tpath, bool need_null_stdfds,
>  				char *const argv[])
>  {
>  	pid_t pid;
> @@ -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 (need_null_stdfds && null_stdfds() < 0) {
> +			exit(1);
>  		}
>  
>  		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..7ced314 100644
> --- a/src/lxc/utils.c
> +++ b/src/lxc/utils.c
> @@ -1445,3 +1445,24 @@ domount:
>  	INFO("Mounted /proc in container for security transition");
>  	return 1;
>  }
> +
> +int null_stdfds(void)
> +{
> +	int fd, ret = -1;
> +
> +	fd = open("/dev/null", O_RDWR);
> +	if (fd < 0)
> +		return -1;
> +
> +	if (dup2(fd, 0) < 0)
> +		goto err;
> +	if (dup2(fd, 1) < 0)
> +		goto err;
> +	if (dup2(fd, 2) < 0)
> +		goto err;
> +
> +	ret = 0;
> +err:
> +	close(fd);
> +	return ret;
> +}
> 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