[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.
> 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 <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
