If an offset of ports is specified to the inet_listen_saddr function(),
and two or more processes tries to bind from these ports at the same time,
occasionally more than one process may be able to bind to the same
port. The condition is detected by listen() but too late to avoid a failure.

This function is called by socket_listen() and used
by all socket listening code in QEMU, so all cases where any form of dynamic
port selection is used should be subject to this issue.

Add code to close and re-establish the socket when this
condition is observed, hiding the race condition from the user.

This has been developed and tested by means of the
test-listen unit test in the previous commit.
Enable the test for make check now that it passes.

Signed-off-by: Knut Omang <knut.omang@oracle.com>
Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com>
Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com>
---
 tests/Makefile.include |  2 +-
 util/qemu-sockets.c    | 68 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 53 insertions(+), 17 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 22bb97e..c38f94e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -127,7 +127,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF)
 gcov-files-check-bufferiszero-y = util/bufferiszero.c
 check-unit-y += tests/test-uuid$(EXESUF)
 check-unit-y += tests/ptimer-test$(EXESUF)
-#check-unit-y += tests/test-listen$(EXESUF)
+check-unit-y += tests/test-listen$(EXESUF)
 gcov-files-ptimer-test-y = hw/core/ptimer.c
 check-unit-y += tests/test-qapi-util$(EXESUF)
 gcov-files-test-qapi-util-y = qapi/qapi-util.c
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 48b9319..7b118b4 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -201,6 +201,42 @@ static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e)
 #endif
 }
 
+static int try_bind_listen(int *socket, InetSocketAddress *saddr,
+                           struct addrinfo *e, int port, Error **errp)
+{
+    int s = *socket;
+    int ret;
+
+    inet_setport(e, port);
+    ret = try_bind(s, saddr, e);
+    if (ret) {
+        if (errno != EADDRINUSE) {
+            error_setg_errno(errp, errno, "Failed to bind socket");
+        }
+        return errno;
+    }
+    if (listen(s, 1) == 0) {
+            return 0;
+    }
+    if (errno == EADDRINUSE) {
+        /* We got to bind the socket to a port but someone else managed
+         * to bind to the same port and beat us to listen on it!
+         * Recreate the socket and return EADDRINUSE to preserve the
+         * expected state by the caller:
+         */
+        closesocket(s);
+        s = create_fast_reuse_socket(e, errp);
+        if (s < 0) {
+            return errno;
+        }
+        *socket = s;
+        errno = EADDRINUSE;
+        return errno;
+    }
+    error_setg_errno(errp, errno, "Failed to listen on socket");
+    return errno;
+}
+
 static int inet_listen_saddr(InetSocketAddress *saddr,
                              int port_offset,
                              bool update_addr,
@@ -210,7 +246,9 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
     char port[33];
     char uaddr[INET6_ADDRSTRLEN+1];
     char uport[33];
-    int slisten, rc, port_min, port_max, p;
+    int rc, port_min, port_max, p;
+    int slisten = 0;
+    int saved_errno = 0;
     Error *err = NULL;
 
     memset(&ai,0, sizeof(ai));
@@ -276,28 +314,26 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
         port_min = inet_getport(e);
         port_max = saddr->has_to ? saddr->to + port_offset : port_min;
         for (p = port_min; p <= port_max; p++) {
-            inet_setport(e, p);
-            if (try_bind(slisten, saddr, e) >= 0) {
-                goto listen;
-            }
-            if (p == port_max) {
-                if (!e->ai_next) {
-                    error_setg_errno(errp, errno, "Failed to bind socket");
-                }
+            int eno = try_bind_listen(&slisten, saddr, e, p, &err);
+            if (!eno) {
+                goto listen_ok;
+            } else if (eno != EADDRINUSE) {
+                goto listen_failed;
             }
         }
+    }
+    error_setg_errno(errp, errno, "Failed to find available port");
+
+listen_failed:
+    saved_errno = errno;
+    if (slisten >= 0) {
         closesocket(slisten);
     }
     freeaddrinfo(res);
+    errno = saved_errno;
     return -1;
 
-listen:
-    if (listen(slisten,1) != 0) {
-        error_setg_errno(errp, errno, "Failed to listen on socket");
-        closesocket(slisten);
-        freeaddrinfo(res);
-        return -1;
-    }
+listen_ok:
     if (update_addr) {
         g_free(saddr->host);
         saddr->host = g_strdup(uaddr);
-- 
git-series 0.9.1

On Fri, Jun 23, 2017 at 12:31:08PM +0200, Knut Omang wrote:
> If an offset of ports is specified to the inet_listen_saddr function(),
> and two or more processes tries to bind from these ports at the same time,
> occasionally more than one process may be able to bind to the same
> port. The condition is detected by listen() but too late to avoid a failure.
> 
> This function is called by socket_listen() and used
> by all socket listening code in QEMU, so all cases where any form of dynamic
> port selection is used should be subject to this issue.
> 
> Add code to close and re-establish the socket when this
> condition is observed, hiding the race condition from the user.
> 
> This has been developed and tested by means of the
> test-listen unit test in the previous commit.
> Enable the test for make check now that it passes.
> 
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com>
> Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
> Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com>
> ---
>  tests/Makefile.include |  2 +-
>  util/qemu-sockets.c    | 68 ++++++++++++++++++++++++++++++++-----------
>  2 files changed, 53 insertions(+), 17 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 22bb97e..c38f94e 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -127,7 +127,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF)
>  gcov-files-check-bufferiszero-y = util/bufferiszero.c
>  check-unit-y += tests/test-uuid$(EXESUF)
>  check-unit-y += tests/ptimer-test$(EXESUF)
> -#check-unit-y += tests/test-listen$(EXESUF)
> +check-unit-y += tests/test-listen$(EXESUF)
>  gcov-files-ptimer-test-y = hw/core/ptimer.c
>  check-unit-y += tests/test-qapi-util$(EXESUF)
>  gcov-files-test-qapi-util-y = qapi/qapi-util.c
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 48b9319..7b118b4 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -201,6 +201,42 @@ static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e)
>  #endif
>  }
>  
> +static int try_bind_listen(int *socket, InetSocketAddress *saddr,
> +                           struct addrinfo *e, int port, Error **errp)
> +{
> +    int s = *socket;
> +    int ret;
> +
> +    inet_setport(e, port);
> +    ret = try_bind(s, saddr, e);
> +    if (ret) {
> +        if (errno != EADDRINUSE) {
> +            error_setg_errno(errp, errno, "Failed to bind socket");
> +        }
> +        return errno;
> +    }
> +    if (listen(s, 1) == 0) {
> +            return 0;
> +    }
> +    if (errno == EADDRINUSE) {
> +        /* We got to bind the socket to a port but someone else managed
> +         * to bind to the same port and beat us to listen on it!
> +         * Recreate the socket and return EADDRINUSE to preserve the
> +         * expected state by the caller:
> +         */
> +        closesocket(s);
> +        s = create_fast_reuse_socket(e, errp);
> +        if (s < 0) {
> +            return errno;
> +        }
> +        *socket = s;

I don't really like this at all - if we need to close + recreate the
socket, IMHO that should remain the job of the caller, since it owns
the socket FD ultimately.

> +        errno = EADDRINUSE;
> +        return errno;
> +    }
> +    error_setg_errno(errp, errno, "Failed to listen on socket");
> +    return errno;
> +}
> +
>  static int inet_listen_saddr(InetSocketAddress *saddr,
>                               int port_offset,
>                               bool update_addr,
> @@ -210,7 +246,9 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
>      char port[33];
>      char uaddr[INET6_ADDRSTRLEN+1];
>      char uport[33];
> -    int slisten, rc, port_min, port_max, p;
> +    int rc, port_min, port_max, p;
> +    int slisten = 0;
> +    int saved_errno = 0;
>      Error *err = NULL;
>  
>      memset(&ai,0, sizeof(ai));
> @@ -276,28 +314,26 @@ static int inet_listen_saddr(InetSocketAddress *saddr,

Just above this line is the original 'create_fast_reuse_socket' call.

I'd suggest that we push that call down into the body of the loop
below:

>          port_min = inet_getport(e);
>          port_max = saddr->has_to ? saddr->to + port_offset : port_min;
>          for (p = port_min; p <= port_max; p++) {
> -            inet_setport(e, p);
> -            if (try_bind(slisten, saddr, e) >= 0) {
> -                goto listen;
> -            }
> -            if (p == port_max) {
> -                if (!e->ai_next) {
> -                    error_setg_errno(errp, errno, "Failed to bind socket");
> -                }
> +            int eno = try_bind_listen(&slisten, saddr, e, p, &err);

Which would mean try_bind_listen no longer needs the magic to close +
recreate the socket.

The only cost of doing this is that you end up closing + recreating the
socket after bind hits EADDRINUSE, as well as after listen() hits it.

I think that's acceptable tradeoff for simpler code, since this is not
a performance critical operation.

> +            if (!eno) {
> +                goto listen_ok;
> +            } else if (eno != EADDRINUSE) {
> +                goto listen_failed;
>              }
>          }
> +    }
> +    error_setg_errno(errp, errno, "Failed to find available port");

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Fri, Jun 23, 2017 at 12:31:08PM +0200, Knut Omang wrote:
> If an offset of ports is specified to the inet_listen_saddr function(),
> and two or more processes tries to bind from these ports at the same time,
> occasionally more than one process may be able to bind to the same
> port. The condition is detected by listen() but too late to avoid a failure.
> 
> This function is called by socket_listen() and used
> by all socket listening code in QEMU, so all cases where any form of dynamic
> port selection is used should be subject to this issue.
> 
> Add code to close and re-establish the socket when this
> condition is observed, hiding the race condition from the user.
> 
> This has been developed and tested by means of the
> test-listen unit test in the previous commit.
> Enable the test for make check now that it passes.
> 
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com>
> Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
> Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com>
> ---
>  tests/Makefile.include |  2 +-
>  util/qemu-sockets.c    | 68 ++++++++++++++++++++++++++++++++-----------
>  2 files changed, 53 insertions(+), 17 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 22bb97e..c38f94e 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -127,7 +127,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF)
>  gcov-files-check-bufferiszero-y = util/bufferiszero.c
>  check-unit-y += tests/test-uuid$(EXESUF)
>  check-unit-y += tests/ptimer-test$(EXESUF)
> -#check-unit-y += tests/test-listen$(EXESUF)
> +check-unit-y += tests/test-listen$(EXESUF)
>  gcov-files-ptimer-test-y = hw/core/ptimer.c
>  check-unit-y += tests/test-qapi-util$(EXESUF)
>  gcov-files-test-qapi-util-y = qapi/qapi-util.c
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 48b9319..7b118b4 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -201,6 +201,42 @@ static int try_bind(int socket, InetSocketAddress *saddr, struct addrinfo *e)
>  #endif
>  }
>  
> +static int try_bind_listen(int *socket, InetSocketAddress *saddr,
> +                           struct addrinfo *e, int port, Error **errp)
> +{
> +    int s = *socket;
> +    int ret;
> +
> +    inet_setport(e, port);
> +    ret = try_bind(s, saddr, e);
> +    if (ret) {
> +        if (errno != EADDRINUSE) {
> +            error_setg_errno(errp, errno, "Failed to bind socket");
> +        }
> +        return errno;
> +    }
> +    if (listen(s, 1) == 0) {
> +            return 0;
> +    }
> +    if (errno == EADDRINUSE) {
> +        /* We got to bind the socket to a port but someone else managed
> +         * to bind to the same port and beat us to listen on it!
> +         * Recreate the socket and return EADDRINUSE to preserve the
> +         * expected state by the caller:
> +         */
> +        closesocket(s);
> +        s = create_fast_reuse_socket(e, errp);

This usage scenario for create_fast_reuse_socket() makes its error
reporting behaviour even more wrong. Recall that create_fast_reuse_socket
is reporting an error if e->ai_next is NULL, which is a way of determining
this is the last call to create_fast_reuse_socket in the loop. That
assumption is violated though now that we're calling the method from
inside the inner loop. Even when e->ai_next is NULL, we may be calling
create_fast_reuse_socket many many times due to the port  'to' range.

> +        if (s < 0) {
> +            return errno;
> +        }
> +        *socket = s;
> +        errno = EADDRINUSE;
> +        return errno;
> +    }
> +    error_setg_errno(errp, errno, "Failed to listen on socket");
> +    return errno;
> +}

This method is both preserving the global errno, and returning the
global errno. The caller expects global errno to be preserved, so
I think we can just return '-1' from this method.

> +
>  static int inet_listen_saddr(InetSocketAddress *saddr,
>                               int port_offset,
>                               bool update_addr,
> @@ -210,7 +246,9 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
>      char port[33];
>      char uaddr[INET6_ADDRSTRLEN+1];
>      char uport[33];
> -    int slisten, rc, port_min, port_max, p;
> +    int rc, port_min, port_max, p;
> +    int slisten = 0;
> +    int saved_errno = 0;
>      Error *err = NULL;
>  
>      memset(&ai,0, sizeof(ai));
> @@ -276,28 +314,26 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
>          port_min = inet_getport(e);
>          port_max = saddr->has_to ? saddr->to + port_offset : port_min;
>          for (p = port_min; p <= port_max; p++) {
> -            inet_setport(e, p);
> -            if (try_bind(slisten, saddr, e) >= 0) {
> -                goto listen;
> -            }
> -            if (p == port_max) {
> -                if (!e->ai_next) {
> -                    error_setg_errno(errp, errno, "Failed to bind socket");
> -                }
> +            int eno = try_bind_listen(&slisten, saddr, e, p, &err);
> +            if (!eno) {
> +                goto listen_ok;
> +            } else if (eno != EADDRINUSE) {
> +                goto listen_failed;
>              }
>          }
> +    }
> +    error_setg_errno(errp, errno, "Failed to find available port");
> +
> +listen_failed:
> +    saved_errno = errno;
> +    if (slisten >= 0) {
>          closesocket(slisten);
>      }
>      freeaddrinfo(res);
> +    errno = saved_errno;
>      return -1;
>  
> -listen:
> -    if (listen(slisten,1) != 0) {
> -        error_setg_errno(errp, errno, "Failed to listen on socket");
> -        closesocket(slisten);
> -        freeaddrinfo(res);
> -        return -1;
> -    }
> +listen_ok:
>      if (update_addr) {
>          g_free(saddr->host);
>          saddr->host = g_strdup(uaddr);
> -- 
> git-series 0.9.1

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, 2017-06-26 at 11:34 +0100, Daniel P. Berrange wrote:
> On Fri, Jun 23, 2017 at 12:31:08PM +0200, Knut Omang wrote:
> > 
> > If an offset of ports is specified to the inet_listen_saddr function(),
> > and two or more processes tries to bind from these ports at the same time,
> > occasionally more than one process may be able to bind to the same
> > port. The condition is detected by listen() but too late to avoid a failure.
> > 
> > This function is called by socket_listen() and used
> > by all socket listening code in QEMU, so all cases where any form of dynamic
> > port selection is used should be subject to this issue.
> > 
> > Add code to close and re-establish the socket when this
> > condition is observed, hiding the race condition from the user.
> > 
> > This has been developed and tested by means of the
> > test-listen unit test in the previous commit.
> > Enable the test for make check now that it passes.
> > 
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > Reviewed-by: Bhavesh Davda <bhavesh.davda@oracle.com>
> > Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
> > Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com>
> > ---
> >  tests/Makefile.include |  2 +-
> >  util/qemu-sockets.c    | 68 ++++++++++++++++++++++++++++++++-----------
> >  2 files changed, 53 insertions(+), 17 deletions(-)
> > 
> > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > index 22bb97e..c38f94e 100644
> > --- a/tests/Makefile.include
> > +++ b/tests/Makefile.include
> > @@ -127,7 +127,7 @@ check-unit-y += tests/test-bufferiszero$(EXESUF)
> >  gcov-files-check-bufferiszero-y = util/bufferiszero.c
> >  check-unit-y += tests/test-uuid$(EXESUF)
> >  check-unit-y += tests/ptimer-test$(EXESUF)
> > -#check-unit-y += tests/test-listen$(EXESUF)
> > +check-unit-y += tests/test-listen$(EXESUF)
> >  gcov-files-ptimer-test-y = hw/core/ptimer.c
> >  check-unit-y += tests/test-qapi-util$(EXESUF)
> >  gcov-files-test-qapi-util-y = qapi/qapi-util.c
> > diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> > index 48b9319..7b118b4 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -201,6 +201,42 @@ static int try_bind(int socket, InetSocketAddress
> > *saddr, struct addrinfo *e)
> >  #endif
> >  }
> >  
> > +static int try_bind_listen(int *socket, InetSocketAddress *saddr,
> > +                           struct addrinfo *e, int port, Error **errp)
> > +{
> > +    int s = *socket;
> > +    int ret;
> > +
> > +    inet_setport(e, port);
> > +    ret = try_bind(s, saddr, e);
> > +    if (ret) {
> > +        if (errno != EADDRINUSE) {
> > +            error_setg_errno(errp, errno, "Failed to bind socket");
> > +        }
> > +        return errno;
> > +    }
> > +    if (listen(s, 1) == 0) {
> > +            return 0;
> > +    }
> > +    if (errno == EADDRINUSE) {
> > +        /* We got to bind the socket to a port but someone else managed
> > +         * to bind to the same port and beat us to listen on it!
> > +         * Recreate the socket and return EADDRINUSE to preserve the
> > +         * expected state by the caller:
> > +         */
> > +        closesocket(s);
> > +        s = create_fast_reuse_socket(e, errp);
> 
> This usage scenario for create_fast_reuse_socket() makes its error
> reporting behaviour even more wrong. Recall that create_fast_reuse_socket
> is reporting an error if e->ai_next is NULL, which is a way of determining
> this is the last call to create_fast_reuse_socket in the loop. That
> assumption is violated though now that we're calling the method from
> inside the inner loop. Even when e->ai_next is NULL, we may be calling
> create_fast_reuse_socket many many times due to the port  'to' range.

I agree that the error reporting should go out of create_fast_reuse_socket().
Note however that this code will only be called when the race condition occurs,
which I think is very unlikely to happen more than once for each call to
inet_listen_saddr (except in my test of course..)

> 
> > 
> > +        if (s < 0) {
> > +            return errno;
> > +        }
> > +        *socket = s;
> > +        errno = EADDRINUSE;
> > +        return errno;
> > +    }
> > +    error_setg_errno(errp, errno, "Failed to listen on socket");
> > +    return errno;
> > +}
> 
> This method is both preserving the global errno, and returning the
> global errno. The caller expects global errno to be preserved, so
> I think we can just return '-1' from this method.

will do,

Thanks,
Knut

> 
> > 
> > +
> >  static int inet_listen_saddr(InetSocketAddress *saddr,
> >                               int port_offset,
> >                               bool update_addr,
> > @@ -210,7 +246,9 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
> >      char port[33];
> >      char uaddr[INET6_ADDRSTRLEN+1];
> >      char uport[33];
> > -    int slisten, rc, port_min, port_max, p;
> > +    int rc, port_min, port_max, p;
> > +    int slisten = 0;
> > +    int saved_errno = 0;
> >      Error *err = NULL;
> >  
> >      memset(&ai,0, sizeof(ai));
> > @@ -276,28 +314,26 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
> >          port_min = inet_getport(e);
> >          port_max = saddr->has_to ? saddr->to + port_offset : port_min;
> >          for (p = port_min; p <= port_max; p++) {
> > -            inet_setport(e, p);
> > -            if (try_bind(slisten, saddr, e) >= 0) {
> > -                goto listen;
> > -            }
> > -            if (p == port_max) {
> > -                if (!e->ai_next) {
> > -                    error_setg_errno(errp, errno, "Failed to bind socket");
> > -                }
> > +            int eno = try_bind_listen(&slisten, saddr, e, p, &err);
> > +            if (!eno) {
> > +                goto listen_ok;
> > +            } else if (eno != EADDRINUSE) {
> > +                goto listen_failed;
> >              }
> >          }
> > +    }
> > +    error_setg_errno(errp, errno, "Failed to find available port");
> > +
> > +listen_failed:
> > +    saved_errno = errno;
> > +    if (slisten >= 0) {
> >          closesocket(slisten);
> >      }
> >      freeaddrinfo(res);
> > +    errno = saved_errno;
> >      return -1;
> >  
> > -listen:
> > -    if (listen(slisten,1) != 0) {
> > -        error_setg_errno(errp, errno, "Failed to listen on socket");
> > -        closesocket(slisten);
> > -        freeaddrinfo(res);
> > -        return -1;
> > -    }
> > +listen_ok:
> >      if (update_addr) {
> >          g_free(saddr->host);
> >          saddr->host = g_strdup(uaddr);
> > -- 
> > git-series 0.9.1
> 
> Regards,
> Daniel