Re: [PATCH qemu-traditional] qemu: Fix race condition when binding ports

[George Dunlap <george.dunlap@eu.citrix.com>]

On 12/09/2013 01:10 PM, Andrew Cooper wrote:
> Two Qemus can race to bind the same VNC port.  It is valid for multiple
> listen()s on the same socket to succeed, but only the first bind() will
> succeed.
>
> In the case that two Qemus are racing, the second one will fail with an
> EADDRINUSE, and bail with a fatal error which renders the domain functionally
> useless.
>
> In the case that bind() fails with EADDRINUSE, rebind the socket again and try
> for the next port.

Sorry if I'm being a bit dense here, but it looks like you have those 
mixed up: if listen() fails with EADDRINUSE, you're going back to call 
bind() again; which would suggest that you can have multible bind()s but 
only a single listen()?  Or am I confused?

In any case, this is clearly a bug fix, and a very well tested one at 
that, so:

Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
>
> ---
>
> This fix has been in XenServer for two and a half years
> ---
>   qemu-sockets.c |   23 ++++++++++++++++-------
>   1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/qemu-sockets.c b/qemu-sockets.c
> index 9b9fa77..e54f6ad 100644
> --- a/qemu-sockets.c
> +++ b/qemu-sockets.c
> @@ -163,6 +163,7 @@ int inet_listen(const char *str, char *ostr, int olen,
>   
>       /* create socket + bind */
>       for (e = res; e != NULL; e = e->ai_next) {
> +    rebind:
>   	getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
>   		    uaddr,INET6_ADDRSTRLEN,uport,32,
>   		    NI_NUMERICHOST | NI_NUMERICSERV);
> @@ -186,7 +187,20 @@ int inet_listen(const char *str, char *ostr, int olen,
>                   if (sockets_debug)
>                       fprintf(stderr,"%s: bind(%s,%s,%d): OK\n", __FUNCTION__,
>                               inet_strfamily(e->ai_family), uaddr, inet_getport(e));
> -                goto listen;
> +                if (listen(slisten,1) == 0) {
> +                    goto listened;
> +                } else {
> +                    int err = errno;
> +
> +                    perror("listen");
> +                    closesocket(slisten);
> +
> +                    if (err == EADDRINUSE)
> +                        goto rebind;
> +                    freeaddrinfo(res);
> +                    return -1;
> +                }
> +
>               }
>               try_next = to && (inet_getport(e) <= to + port_offset);
>               if (!try_next || sockets_debug)
> @@ -205,12 +219,7 @@ int inet_listen(const char *str, char *ostr, int olen,
>       freeaddrinfo(res);
>       return -1;
>   
> -listen:
> -    if (listen(slisten,1) != 0) {
> -        perror("listen");
> -        closesocket(slisten);
> -        return -1;
> -    }
> +listened:
>       if (ostr) {
>           if (e->ai_family == PF_INET6) {
>               snprintf(ostr, olen, "[%s]:%d%s", uaddr,