[v5,4/4] sockets: Handle race condition between binds to the same port

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.

Also clean up some issues with error handling to allow more
accurate reporting of the cause of an error.

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>
Signed-off-by: Knut Omang <knut.omang@oracle.com>
---
 tests/Makefile.include |  2 +-
 util/qemu-sockets.c    | 51 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 39 insertions(+), 14 deletions(-)

On Sat, Jul 22, 2017 at 09:49:33AM +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.
> 
> Also clean up some issues with error handling to allow more
> accurate reporting of the cause of an error.
> 
> 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>
> Signed-off-by: Knut Omang <knut.omang@oracle.com>
> ---
>  tests/Makefile.include |  2 +-
>  util/qemu-sockets.c    | 51 ++++++++++++++++++++++++++++++++-----------
>  2 files changed, 39 insertions(+), 14 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index b37c0c8..9d2131d 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -128,7 +128,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 39f207c..8ca3ba6 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -210,7 +210,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));
> @@ -277,27 +279,50 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
>          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) {
> +            rc = try_bind(slisten, saddr, e);
> +            if (rc) {
> +                if (errno == EADDRINUSE) {

Add in  '&& e->ai_next' here, so we trigger the error handling
on the else branch if this was the last address we had.

> +                    continue;
> +                } else {
>                      error_setg_errno(errp, errno, "Failed to bind socket");
> +                    goto listen_failed;
> +                }
> +            }
> +            rc = listen(slisten, 1);
> +            if (!rc) {

Assigning to rc seems redundant here.

> +                goto listen_ok;
> +            } else if (errno == EADDRINUSE) {
> +                /* Someone else managed to bind to the same port and beat us
> +                 * to listen on it! Socket semantics does not allow us to
> +                 * recover from this situation, so we need to recreate the
> +                 * socket to allow bind attempts for subsequent ports:
> +                 */
> +                closesocket(slisten);
> +                slisten = create_fast_reuse_socket(e, errp);

create_fast_reuse_socket may report an error here

> +                if (slisten >= 0) {
> +                    continue;
>                  }
>              }
> +            error_setg_errno(errp, errno, "Failed to listen on socket");

at which point you report another error on top of it with a stale
errno value here. This bug would be more obvious if error reporting
was pulled out of create_fast_reuse_socket() as described in the
earlier patch.

I think it'd be clearer if we reduced the nesting here. eg

     if (listen() == 0) {
        goto listen_ok;
     }
     if (errno != EADDRINUSE) {
         error_setg_error(errp, errno, "....");
	 goto listen_failed;
     }
     closesocket(listen);
     slisten = create_fast_reuse_socket(e);
     if (slisten < 0 && !e->ai_next) {
         error_setg_error(errp, errno, "....");
	 goto listen_failed;
     }

> +            goto listen_failed;
>          }
> +    }
> +    if (err) {
> +        error_propagate(errp, err);
> +    } else {
> +        error_setg_errno(errp, errno, "Failed to find an available port");
> +    }

if we tidied up the first call to create_fast_reuse_socket() too, it would
remove the need for this.

> +
> +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);

Regards,
Daniel

On Tue, 2017-07-25 at 10:54 +0100, Daniel P. Berrange wrote:
> On Sat, Jul 22, 2017 at 09:49:33AM +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.
> > 
> > Also clean up some issues with error handling to allow more
> > accurate reporting of the cause of an error.
> > 
> > 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>
> > Signed-off-by: Knut Omang <knut.omang@oracle.com>
> > ---
> >  tests/Makefile.include |  2 +-
> >  util/qemu-sockets.c    | 51 ++++++++++++++++++++++++++++++++-----------
> >  2 files changed, 39 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > index b37c0c8..9d2131d 100644
> > --- a/tests/Makefile.include
> > +++ b/tests/Makefile.include
> > @@ -128,7 +128,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 39f207c..8ca3ba6 100644
> > --- a/util/qemu-sockets.c
> > +++ b/util/qemu-sockets.c
> > @@ -210,7 +210,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));
> > @@ -277,27 +279,50 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
> >          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) {
> > +            rc = try_bind(slisten, saddr, e);
> > +            if (rc) {
> > +                if (errno == EADDRINUSE) {
> 
> Add in  '&& e->ai_next' here, so we trigger the error handling
> on the else branch if this was the last address we had.

See my comment on patch 2 - with that reporting semantics, inability to
create a socket will only be reported if none of the addrinfo structs
allowed socket creation so this reporting case goes away.

> > +                    continue;
> > +                } else {
> >                      error_setg_errno(errp, errno, "Failed to bind socket");
> > +                    goto listen_failed;
> > +                }
> > +            }
> > +            rc = listen(slisten, 1);
> > +            if (!rc) {
> 
> Assigning to rc seems redundant here.

removed.

> > +                goto listen_ok;
> > +            } else if (errno == EADDRINUSE) {
> > +                /* Someone else managed to bind to the same port and beat us
> > +                 * to listen on it! Socket semantics does not allow us to
> > +                 * recover from this situation, so we need to recreate the
> > +                 * socket to allow bind attempts for subsequent ports:
> > +                 */
> > +                closesocket(slisten);
> > +                slisten = create_fast_reuse_socket(e, errp);
> 
> create_fast_reuse_socket may report an error here
> 
> > +                if (slisten >= 0) {
> > +                    continue;
> >                  }
> >              }
> > +            error_setg_errno(errp, errno, "Failed to listen on socket");
> 
> at which point you report another error on top of it with a stale
> errno value here. This bug would be more obvious if error reporting
> was pulled out of create_fast_reuse_socket() as described in the
> earlier patch.
> 
> I think it'd be clearer if we reduced the nesting here. eg
> 
>      if (listen() == 0) {
>         goto listen_ok;
>      }
>      if (errno != EADDRINUSE) {
>          error_setg_error(errp, errno, "....");
> 	 goto listen_failed;
>      }
>      closesocket(listen);
>      slisten = create_fast_reuse_socket(e);
>      if (slisten < 0 && !e->ai_next) {
>          error_setg_error(errp, errno, "....");
> 	 goto listen_failed;
>      }

Good idea to push the != case up, done!

> > +            goto listen_failed;
> >          }
> > +    }
> > +    if (err) {
> > +        error_propagate(errp, err);
> > +    } else {
> > +        error_setg_errno(errp, errno, "Failed to find an available port");
> > +    }
> 
> if we tidied up the first call to create_fast_reuse_socket() too, it would
> remove the need for this.

I think v6 now solves this in a better way by separating between 
whether one or more ports have actually been tried and the case where no 
valid/working addrinfo records have been supplied.

Thanks,
Knut
> 
> > +
> > +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);
> 
> Regards,
> Daniel