[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] slirp: fork_exec(): Don't close() a negative nu
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH] slirp: fork_exec(): Don't close() a negative number in fork_exec() |
Date: |
Tue, 11 Jul 2017 20:08:47 +0100 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
* 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
Re: [Qemu-devel] [PATCH] slirp: fork_exec(): Don't close() a negative number in fork_exec(), Samuel Thibault, 2017/07/11