[Qemu-devel] [PATCH] slirp: fork_exec(): Don't close() a negative number in fork_exec()

[Qemu-devel] [PATCH] slirp: fork_exec(): Don't close() a negative number in fork_exec()

[Peter Maydell]

In a fork_exec() error path we try to closesocket(s) when s might
be a negative number because the thing that failed was the
qemu_socket() call. Add a guard so we don't do this.

(Spotted by Coverity: CID 1005727 issue 1 of 2.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Issue 2 of 2 in CID 1005727 is trickier -- we need to move as
much as possible of the client-end connect/accept out of the
child process and into the parent as possible. I'm not sure
if it's safe to do it all in the parent without deadlocking...

 slirp/misc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/slirp/misc.c b/slirp/misc.c
index 88e9d94197..260187b6b6 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -112,7 +112,9 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
 		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
 		    listen(s, 1) < 0) {
 			error_report("Error: inet socket: %s", strerror(errno));
-			closesocket(s);
+			if (s >= 0) {
+			    closesocket(s);
+			}
 
 			return 0;
 		}
-- 
2.11.0

Re: [Qemu-devel] [PATCH] slirp: fork_exec(): Don't close() a negative number in fork_exec()

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> In a fork_exec() error path we try to closesocket(s) when s might
> be a negative number because the thing that failed was the
> qemu_socket() call. Add a guard so we don't do this.
> 
> (Spotted by Coverity: CID 1005727 issue 1 of 2.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Issue 2 of 2 in CID 1005727 is trickier -- we need to move as
> much as possible of the client-end connect/accept out of the
> child process and into the parent as possible. I'm not sure
> if it's safe to do it all in the parent without deadlocking...

or just bail earlier?   The bit that worries me there
is the dup2(s, [012]); which is called unchecked, if that fails
then your telnetd or whatever probably ends up connected to whatever
your 0..2 were originally.

>  slirp/misc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/slirp/misc.c b/slirp/misc.c
> index 88e9d94197..260187b6b6 100644
> --- a/slirp/misc.c
> +++ b/slirp/misc.c
> @@ -112,7 +112,9 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
>  		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
>  		    listen(s, 1) < 0) {
>  			error_report("Error: inet socket: %s", strerror(errno));
> -			closesocket(s);
> +			if (s >= 0) {
> +			    closesocket(s);
> +			}


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

(I'm not convinced this would ever do anything bad, at least on a *nix
system, the -ve value is always going to be an invalid fd so the close
will just fail).

Dave

>  			return 0;
>  		}
> -- 
> 2.11.0
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] slirp: fork_exec(): Don't close() a negative number in fork_exec()

[Peter Maydell]

[cc'd Eric as the sort of person

On 11 July 2017 at 17:29, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> In a fork_exec() error path we try to closesocket(s) when s might
>> be a negative number because the thing that failed was the
>> qemu_socket() call. Add a guard so we don't do this.
>>
>> (Spotted by Coverity: CID 1005727 issue 1 of 2.)
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Issue 2 of 2 in CID 1005727 is trickier -- we need to move as
>> much as possible of the client-end connect/accept out of the
>> child process and into the parent as possible. I'm not sure
>> if it's safe to do it all in the parent without deadlocking...
>
> or just bail earlier?

The problem is you can only bail while you're in the parent
before forking. Once you've started the child there's no
mechanism for dealing with failure.

>   The bit that worries me there
> is the dup2(s, [012]); which is called unchecked, if that fails
> then your telnetd or whatever probably ends up connected to whatever
> your 0..2 were originally.

dup2() in a child is actually pretty safe -- the only ways
it can fail are:
 * fd2 isn't actually an open file descriptor (can't happen)
 * fd1 is negative or bigger than OPEN_MAX (can't happen)
 * EINTR (just retry, I guess)

The awkward part is POSIX says that dup2() may fail with EIO if
the close() of newfd failed, in which case I dunno what the
child is supposed to do about it -- do a manual close(), ignore
the error from close() and then dup2() again??
Linux specifically says it doesn't do this, and BSD/OSX don't
document EIO as possible so I assume they have sane behaviour.

In any case, ignoring the possibility that dup2(s, [012]) in a child
process could fail is AFAIK very very widespread standard
behaviour for unix daemons. (We have another example in
os_setup_post() in os-posix.c, for instance.)

Random extra: Linux dup2() manpage has a mysterious remark about
EBUSY -- does anybody know what that's all about? It's not
sanctioned by POSIX...

What I would like to do and think should be safe is:

    s = qemu_socket(...);
    bind(s);
    listen(s, 1);
    cs = qemu_socket(...);
    connect(cs, ...);
    switch (fork ()) {
        child:
           dup2
           close fds
           execvp(...);
        parent:
           break;
    }
    close(cs);
    ss = accept(s, ...);
    close(s);
    etc;

ie push the bind/listen/create client socket/connect up into
before the fork(), to give the behaviour of "like socketpair()
but for AF_INET".

(I believe this will work and not deadlock because connect()
doesn't block until accept(), it only needs the tcp handshake.)

>>  slirp/misc.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/slirp/misc.c b/slirp/misc.c
>> index 88e9d94197..260187b6b6 100644
>> --- a/slirp/misc.c
>> +++ b/slirp/misc.c
>> @@ -112,7 +112,9 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
>>                   bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
>>                   listen(s, 1) < 0) {
>>                       error_report("Error: inet socket: %s", strerror(errno));
>> -                     closesocket(s);
>> +                     if (s >= 0) {
>> +                         closesocket(s);
>> +                     }
>
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> (I'm not convinced this would ever do anything bad, at least on a *nix
> system, the -ve value is always going to be an invalid fd so the close
> will just fail).

Indeed. But it keeps Coverity happy.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] slirp: fork_exec(): Don't close() a negative number in fork_exec()

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> [cc'd Eric as the sort of person
> 
> On 11 July 2017 at 17:29, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> >> In a fork_exec() error path we try to closesocket(s) when s might
> >> be a negative number because the thing that failed was the
> >> qemu_socket() call. Add a guard so we don't do this.
> >>
> >> (Spotted by Coverity: CID 1005727 issue 1 of 2.)
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >> ---
> >> Issue 2 of 2 in CID 1005727 is trickier -- we need to move as
> >> much as possible of the client-end connect/accept out of the
> >> child process and into the parent as possible. I'm not sure
> >> if it's safe to do it all in the parent without deadlocking...
> >
> > or just bail earlier?
> 
> The problem is you can only bail while you're in the parent
> before forking. Once you've started the child there's no
> mechanism for dealing with failure.

Well, you can always exit the child before anything worse can happen.

> >   The bit that worries me there
> > is the dup2(s, [012]); which is called unchecked, if that fails
> > then your telnetd or whatever probably ends up connected to whatever
> > your 0..2 were originally.
> 
> dup2() in a child is actually pretty safe -- the only ways
> it can fail are:
>  * fd2 isn't actually an open file descriptor (can't happen)
>  * fd1 is negative or bigger than OPEN_MAX (can't happen)
>  * EINTR (just retry, I guess)

True, I'd missed that fd1 was probably always a valid fd;
so probably the rest of this is pretty academic.

> The awkward part is POSIX says that dup2() may fail with EIO if
> the close() of newfd failed, in which case I dunno what the
> child is supposed to do about it -- do a manual close(), ignore
> the error from close() and then dup2() again??

I wouldn't like to bet on it being legal to call close() on an
error, what state is the fd you wanted to close in?

> Linux specifically says it doesn't do this, and BSD/OSX don't
> document EIO as possible so I assume they have sane behaviour.
> 
> In any case, ignoring the possibility that dup2(s, [012]) in a child
> process could fail is AFAIK very very widespread standard
> behaviour for unix daemons. (We have another example in
> os_setup_post() in os-posix.c, for instance.)
> 
> Random extra: Linux dup2() manpage has a mysterious remark about
> EBUSY -- does anybody know what that's all about? It's not
> sanctioned by POSIX...
> 
> What I would like to do and think should be safe is:
> 
>     s = qemu_socket(...);
>     bind(s);
>     listen(s, 1);
>     cs = qemu_socket(...);
>     connect(cs, ...);
>     switch (fork ()) {
>         child:
>            dup2
>            close fds
>            execvp(...);
>         parent:
>            break;
>     }
>     close(cs);
>     ss = accept(s, ...);
>     close(s);
>     etc;
> 
> ie push the bind/listen/create client socket/connect up into
> before the fork(), to give the behaviour of "like socketpair()
> but for AF_INET".
> 
> (I believe this will work and not deadlock because connect()
> doesn't block until accept(), it only needs the tcp handshake.)

OK, I don't know the details of the blocking htere.

Dave

> >>  slirp/misc.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/slirp/misc.c b/slirp/misc.c
> >> index 88e9d94197..260187b6b6 100644
> >> --- a/slirp/misc.c
> >> +++ b/slirp/misc.c
> >> @@ -112,7 +112,9 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
> >>                   bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
> >>                   listen(s, 1) < 0) {
> >>                       error_report("Error: inet socket: %s", strerror(errno));
> >> -                     closesocket(s);
> >> +                     if (s >= 0) {
> >> +                         closesocket(s);
> >> +                     }
> >
> >
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> > (I'm not convinced this would ever do anything bad, at least on a *nix
> > system, the -ve value is always going to be an invalid fd so the close
> > will just fail).
> 
> Indeed. But it keeps Coverity happy.
> 
> thanks
> -- PMM
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] slirp: fork_exec(): Don't close() a negative number in fork_exec()

[Peter Maydell]

On 11 July 2017 at 20:08, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> [cc'd Eric as the sort of person
>>
>> On 11 July 2017 at 17:29, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
>> > * Peter Maydell (peter.maydell@linaro.org) wrote:
>> >> In a fork_exec() error path we try to closesocket(s) when s might
>> >> be a negative number because the thing that failed was the
>> >> qemu_socket() call. Add a guard so we don't do this.
>> >>
>> >> (Spotted by Coverity: CID 1005727 issue 1 of 2.)
>> >>
>> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> >> ---
>> >> Issue 2 of 2 in CID 1005727 is trickier -- we need to move as
>> >> much as possible of the client-end connect/accept out of the
>> >> child process and into the parent as possible. I'm not sure
>> >> if it's safe to do it all in the parent without deadlocking...
>> >
>> > or just bail earlier?
>>
>> The problem is you can only bail while you're in the parent
>> before forking. Once you've started the child there's no
>> mechanism for dealing with failure.
>
> Well, you can always exit the child before anything worse can happen.

You need a mechanism then for causing the parent to notice.
The current code would leave the parent in a blocking
accept() call forever (this is what all the XXX comments
in the current code are about).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] slirp: fork_exec(): Don't close() a negative number in fork_exec()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]

On 07/11/2017 03:40 PM, Peter Maydell wrote:
>>>
>>> The problem is you can only bail while you're in the parent
>>> before forking. Once you've started the child there's no
>>> mechanism for dealing with failure.
>>
>> Well, you can always exit the child before anything worse can happen.
> 
> You need a mechanism then for causing the parent to notice.
> The current code would leave the parent in a blocking
> accept() call forever (this is what all the XXX comments
> in the current code are about).

You can have the parent create a pipe before forking and does a blocking
read, the child then manages fds and writes success (or failure) to its
end of the pipe (or an early exit causes the parent to read EOF); when
the parent finally gets word on the pipe, then it knows whether the
child is dead-on-arrival due to early setup failures, or whether it is
now safe to block on accept().  But it can easily get more and more
complex the more you want to handle; and if the parent is
multi-threaded, you have to be careful that you only call
async-signal-safe functions in the child between the fork and exec.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] slirp: fork_exec(): Don't close() a negative number in fork_exec()

[Samuel Thibault]

Peter Maydell, on dim. 09 juil. 2017 18:54:22 +0100, wrote:
> In a fork_exec() error path we try to closesocket(s) when s might
> be a negative number because the thing that failed was the
> qemu_socket() call. Add a guard so we don't do this.
> 
> (Spotted by Coverity: CID 1005727 issue 1 of 2.)

Applied to my tree, thanks!

Samuel