Re: [Qemu-devel] [PATCH v5 4/4] sockets: Handle race condition between binds to the same port
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> > Reviewed-by: Bhavesh Davda > > Reviewed-by: Yuval Shaia > > Reviewed-by: Girish Moodalbail > > Signed-off-by: Knut Omang > > --- > > 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(,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
Re: [Qemu-devel] [PATCH v5 4/4] sockets: Handle race condition between binds to the same port
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> Reviewed-by: Bhavesh Davda > Reviewed-by: Yuval Shaia > Reviewed-by: Girish Moodalbail > Signed-off-by: Knut Omang > --- > 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(,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) { > +
[Qemu-devel] [PATCH 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 OmangReviewed-by: Bhavesh Davda Reviewed-by: Yuval Shaia Reviewed-by: Girish Moodalbail Signed-off-by: Knut Omang --- 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(,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) { +continue; +} else { error_setg_errno(errp, errno, "Failed to bind socket"); +goto listen_failed; +} +} +rc = listen(slisten, 1); +if (!rc) { +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); +if (slisten >= 0) { +continue; } } +error_setg_errno(errp, errno, "Failed to listen on socket"); +goto listen_failed; } +} +if (err) { +error_propagate(errp, err); +} else { +error_setg_errno(errp, errno, "Failed to find an 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