Quoting Tycho Andersen (tycho.ander...@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
>
> Reported-by: Coverity
> Signed-off-by: Tycho Andersen <tycho.ander...@canonical.com>
Acked-by: Serge E. Hallyn <serge.hal...@ubuntu.com>
Two comments below, please resend with my ack.
> ---
> 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.
> + */
No, please drop that comment ^.
> + if (quiet && 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..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);
Just to make sure, have you tried a package or ppa build to make
sure that the lack of error checking doesn't upset the build
system?
> + 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@lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
