Re: [Xen-devel] [PATCH v2 qemu-traditional] qemu: Fix race condition when opening ports

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-12-09 at 13:27 +0000, Andrew Cooper wrote:
> Two Qemus can race to bind the same VNC port.  It is valid for multiple
> bind()s on the same socket to succeed, but only the first listen() will
> succeed.
> 
> In the case that two Qemus are starting at the same time, and both trying to
> grab the next free VNC port, the second one will fail with an EADDRINUSE, and
> bail with a fatal error which renders the domain functionally useless.
> 
> In the case that listen() fails with EADDRINUSE, rebind the socket again and
> try for the next port.

Does this do the right thing if vncunused=0? In that case the user has
given a specific port which we want to bind to, so going to the next
port is not desired.

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> Release-acked-by: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> 
> ---
> 
> Changes in v2:
>  * Fix commit message.
> 
> 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;

Does this code actually try for the next port? The try_next check is
after the goto here (you can see it in the context below). Won't it just
try to rebind the same port again and again?

> +                    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,



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel