| Message ID | 1455622545-22269-1-git-send-email-ian.campbell@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 16, 2016 at 11:35:45AM +0000, Ian Campbell wrote: > Taking care not to do so if nullfd happens (somehow) to have the same > fd number as stdin/out/err. > > CID: 1130519 > > It was previously hypothesised[0] that fixing 1130516 would solve this > too, but that appears to not have been the case. > > Compile tested only. > > [0] http://lists.xenproject.org/archives/html/xen-devel/2013-11/msg02931.html > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Cc: andrew.cooper3@citrix.com Acked-by: Wei Liu <wei.liu2@citrix.com> > --- > tools/libxl/xl_cmdimpl.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index d07ccb2..f38e3dd 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -505,6 +505,16 @@ static int do_daemonize(char *name, const char *pidfile) > dup2(logfile, 1); > dup2(logfile, 2); > > + /* Close nullfd unless it happens to == std{in,out,err} */ > + switch (nullfd) { > + case 0: > + case 1: > + case 2: > + break; > + default: > + close(nullfd); > + } > + > CHK_SYSCALL(daemon(0, 1)); > > if (pidfile) { > -- > 2.1.4 >
Ian Campbell writes ("[PATCH] xl: close nullfd after dup2'ing it to stdin"):
> Taking care not to do so if nullfd happens (somehow) to have the same
> fd number as stdin/out/err.
I think that can only happen if the program (the process) has a
serious problem: ie, fd 0 1 or 2 would have to be closed. If that
happens many other things can go badly wrong.
If this is causing Coverity to complain I would suggest adding
assert(nullfd >= 3);
assert(logfile >= 3);
instead.
Ian.
On Tue, 2016-02-16 at 17:45 +0000, Ian Jackson wrote: > Ian Campbell writes ("[PATCH] xl: close nullfd after dup2'ing it to > stdin"): > > Taking care not to do so if nullfd happens (somehow) to have the > same > > fd number as stdin/out/err. > > I think that can only happen if the program (the process) has a > serious problem: ie, fd 0 1 or 2 would have to be closed. Yes, that was my thought and what I wanted to guard against. > If that happens many other things can go badly wrong. Indeed. I've seen this happen in other scenarios with non-C programs forking and execing stuff with stdio fds closed. > If this is causing Coverity to complain I would suggest adding > assert(nullfd >= 3); > assert(logfile >= 3); > instead. Coverity wasn't complaining about this particular aspect, it was only complaining about the leak of nullfd, avoiding stdin/out/err was just me being belt and braces about the possibility of nullfd being one of the stdio fds. I'm happy with the assert approach too. Ian
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index d07ccb2..f38e3dd 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -505,6 +505,16 @@ static int do_daemonize(char *name, const char *pidfile) dup2(logfile, 1); dup2(logfile, 2); + /* Close nullfd unless it happens to == std{in,out,err} */ + switch (nullfd) { + case 0: + case 1: + case 2: + break; + default: + close(nullfd); + } + CHK_SYSCALL(daemon(0, 1)); if (pidfile) {
Taking care not to do so if nullfd happens (somehow) to have the same fd number as stdin/out/err. CID: 1130519 It was previously hypothesised[0] that fixing 1130516 would solve this too, but that appears to not have been the case. Compile tested only. [0] http://lists.xenproject.org/archives/html/xen-devel/2013-11/msg02931.html Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: andrew.cooper3@citrix.com --- tools/libxl/xl_cmdimpl.c | 10 ++++++++++ 1 file changed, 10 insertions(+)