[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 17:29:43 +0100 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
* 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 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 <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).
Dave
> return 0;
> }
> --
> 2.11.0
>
>
--
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