* Peter Maydell (peter.mayd...@linaro.org) wrote: > [cc'd Eric as the sort of person > > On 11 July 2017 at 17:29, Dr. David Alan Gilbert <dgilb...@redhat.com> wrote: > > * Peter Maydell (peter.mayd...@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.mayd...@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 <dgilb...@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 / dgilb...@redhat.com / Manchester, UK