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 <address@hidden> wrote:
> * Peter Maydell (address@hidden) 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 <address@hidden>
>> ---
>> 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 <address@hidden>
>
> (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