[PATCH] xl: close nullfd after dup2'ing it to stdin

[PATCH] xl: close nullfd after dup2'ing it to stdin

[Ian Campbell]

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(+)

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

Re: [PATCH] xl: close nullfd after dup2'ing it to stdin

[Wei Liu]

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
> 

Re: [PATCH] xl: close nullfd after dup2'ing it to stdin

[Ian Jackson]

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.

Re: [PATCH] xl: close nullfd after dup2'ing it to stdin

[Ian Campbell]

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

[PATCH v2] xl: close nullfd after dup2'ing it to stdin

[Ian Campbell]

We assert that nullfd if not std{in,out,err} since that would result
in closing one of the just dup2'd fds. For this to happen
std{in,out,err} would have needed to be closed, at which point all
sorts of other things could go wrong.

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
---
v2: Assert logfile and nullfd are not stdio fds
---
 tools/libxl/xl_cmdimpl.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 9958d8a..a377de1 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -499,12 +499,17 @@ static int do_daemonize(char *name, const char *pidfile)
 
     CHK_SYSCALL(logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND, 0644));
     free(fullname);
+    assert(logfile >= 3);
 
     CHK_SYSCALL(nullfd = open("/dev/null", O_RDONLY));
+    assert(nullfd >= 3);
+
     dup2(nullfd, 0);
     dup2(logfile, 1);
     dup2(logfile, 2);
 
+    close(nullfd);
+
     CHK_SYSCALL(daemon(0, 1));
 
     if (pidfile) {
-- 
2.1.4

Re: [PATCH v2] xl: close nullfd after dup2'ing it to stdin

[Ian Campbell]

On Wed, 2016-02-17 at 10:39 +0000, Ian Campbell wrote:
> We assert that nullfd if not std{in,out,err} since that would result
> in closing one of the just dup2'd fds. For this to happen
> std{in,out,err} would have needed to be closed, at which point all
> sorts of other things could go wrong.
> 
> 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

ping?

> ---
> v2: Assert logfile and nullfd are not stdio fds
> ---
>  tools/libxl/xl_cmdimpl.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 9958d8a..a377de1 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -499,12 +499,17 @@ static int do_daemonize(char *name, const char
> *pidfile)
>  
>      CHK_SYSCALL(logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND,
> 0644));
>      free(fullname);
> +    assert(logfile >= 3);
>  
>      CHK_SYSCALL(nullfd = open("/dev/null", O_RDONLY));
> +    assert(nullfd >= 3);
> +
>      dup2(nullfd, 0);
>      dup2(logfile, 1);
>      dup2(logfile, 2);
>  
> +    close(nullfd);
> +
>      CHK_SYSCALL(daemon(0, 1));
>  
>      if (pidfile) {

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2] xl: close nullfd after dup2'ing it to stdin

[Konrad Rzeszutek Wilk]

On Tue, Feb 23, 2016 at 10:30:31AM +0000, Ian Campbell wrote:
> On Wed, 2016-02-17 at 10:39 +0000, Ian Campbell wrote:
> > We assert that nullfd if not std{in,out,err} since that would result
> > in closing one of the just dup2'd fds. For this to happen
> > std{in,out,err} would have needed to be closed, at which point all
> > sorts of other things could go wrong.
> > 
> > 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
> 
> ping?

Ian, you wouldn't have a git branch with all your outstanding
patches you had posted somewhere?

Just in case we don't get to them done by feature freeze window and
somebody starts replaying these patches..

> 
> > ---
> > v2: Assert logfile and nullfd are not stdio fds
> > ---
> >  tools/libxl/xl_cmdimpl.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 9958d8a..a377de1 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -499,12 +499,17 @@ static int do_daemonize(char *name, const char
> > *pidfile)
> >  
> >      CHK_SYSCALL(logfile = open(fullname, O_WRONLY|O_CREAT|O_APPEND,
> > 0644));
> >      free(fullname);
> > +    assert(logfile >= 3);
> >  
> >      CHK_SYSCALL(nullfd = open("/dev/null", O_RDONLY));
> > +    assert(nullfd >= 3);
> > +
> >      dup2(nullfd, 0);
> >      dup2(logfile, 1);
> >      dup2(logfile, 2);
> >  
> > +    close(nullfd);
> > +
> >      CHK_SYSCALL(daemon(0, 1));
> >  
> >      if (pidfile) {
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2] xl: close nullfd after dup2'ing it to stdin

[Wei Liu]

On Mon, Feb 29, 2016 at 10:45:51AM -0500, Konrad Rzeszutek Wilk wrote:
> On Tue, Feb 23, 2016 at 10:30:31AM +0000, Ian Campbell wrote:
> > On Wed, 2016-02-17 at 10:39 +0000, Ian Campbell wrote:
> > > We assert that nullfd if not std{in,out,err} since that would result
> > > in closing one of the just dup2'd fds. For this to happen
> > > std{in,out,err} would have needed to be closed, at which point all
> > > sorts of other things could go wrong.
> > > 
> > > 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
> > 
> > ping?
> 
> Ian, you wouldn't have a git branch with all your outstanding
> patches you had posted somewhere?
> 
> Just in case we don't get to them done by feature freeze window and
> somebody starts replaying these patches..
> 

Bug fixes are allowed to go in even after the freeze.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v2] xl: close nullfd after dup2'ing it to stdin

[Ian Jackson]

Ian Campbell writes ("[PATCH v2] xl: close nullfd after dup2'ing it to stdin"):
> We assert that nullfd if not std{in,out,err} since that would result
> in closing one of the just dup2'd fds. For this to happen
> std{in,out,err} would have needed to be closed, at which point all
> sorts of other things could go wrong.
> 
> 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
> ---
> v2: Assert logfile and nullfd are not stdio fds

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Committed-by: Ian Jackson <ian.jackson@eu.citrix.com>

(The copy to ijc may bounce I guess...)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel