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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Maydell (peter.maydell@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.maydell@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 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.

>  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 <dgilbert@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).

Dave

>  			return 0;
>  		}
> -- 
> 2.11.0
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK