[lxc-devel] [PATCH v3 4/4] uniformly nullify std fds
Tycho Andersen
tycho.andersen at canonical.com
Tue Jun 9 18:53:58 UTC 2015
On Tue, Jun 09, 2015 at 07:51:07PM +0200, Robert Vogelgesang wrote:
> 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. :-)
Can you write exactly what you'd like the comment to say? I think this
is a fairly common C idiom, so I doubt someone would switch the order.
Tycho
> 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
> _______________________________________________
> 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