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

[Dr. David Alan Gilbert]

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

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 <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
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK