| Message ID | 20171016201650.18399-4-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,v1,01/11] sockets: factor out a new try_bind() function | expand |
On 16 October 2017 at 21:16, Daniel P. Berrange <berrange@redhat.com> wrote: > From: Knut Omang <knut.omang@oracle.com> > > 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. > > 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> > Reviewed-by: Daniel P. Berrange <berrange@redhat.com> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> Hi. Coverity points out that this code could leak a socket fd (CID 1381805): > - /* create socket + bind */ > + /* create socket + bind/listen */ > for (e = res; e != NULL; e = e->ai_next) { > getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, > uaddr,INET6_ADDRSTRLEN,uport,32, > @@ -266,37 +269,58 @@ static int inet_listen_saddr(InetSocketAddress *saddr, > > slisten = create_fast_reuse_socket(e); Every time around this outer for() loop we create an fd by calling create_fast_reuse_socket(), which means that the previous fd we had in that variable is leaked. > if (slisten < 0) { > - if (!e->ai_next) { > - error_setg_errno(errp, errno, "Failed to create socket"); > - } > continue; > } > > + socket_created = true; > 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) { > + 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; > } > } > + if (!listen(slisten, 1)) { > + goto listen_ok; > + } > + if (errno != EADDRINUSE) { > + error_setg_errno(errp, errno, "Failed to listen on socket"); > + goto listen_failed; > + } > + /* 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); (Here we do close the old fd before opening a new one, so the inner loop doesn't leak, only the outer one.) > + if (slisten < 0) { > + error_setg_errno(errp, errno, > + "Failed to recreate failed listening socket"); > + goto listen_failed; > + } > } > + } thanks -- PMM
On Fri, Nov 03, 2017 at 06:54:44PM +0000, Peter Maydell wrote: > On 16 October 2017 at 21:16, Daniel P. Berrange <berrange@redhat.com> wrote: > > From: Knut Omang <knut.omang@oracle.com> > > > > 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. > > > > 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> > > Reviewed-by: Daniel P. Berrange <berrange@redhat.com> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > > Hi. Coverity points out that this code could leak a socket fd > (CID 1381805): Yeah, I have a patch posted a week or two back to fix this. I'll get a pull request in before release to fix it, along with test suite Regards, Daniel
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c index 65f6dcd5ef..b47fb45885 100644 --- a/util/qemu-sockets.c +++ b/util/qemu-sockets.c @@ -206,7 +206,10 @@ 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; + bool socket_created = false; Error *err = NULL; memset(&ai,0, sizeof(ai)); @@ -258,7 +261,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr, return -1; } - /* create socket + bind */ + /* create socket + bind/listen */ for (e = res; e != NULL; e = e->ai_next) { getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen, uaddr,INET6_ADDRSTRLEN,uport,32, @@ -266,37 +269,58 @@ static int inet_listen_saddr(InetSocketAddress *saddr, slisten = create_fast_reuse_socket(e); if (slisten < 0) { - if (!e->ai_next) { - error_setg_errno(errp, errno, "Failed to create socket"); - } continue; } + socket_created = true; 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) { + 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; } } + if (!listen(slisten, 1)) { + goto listen_ok; + } + if (errno != EADDRINUSE) { + error_setg_errno(errp, errno, "Failed to listen on socket"); + goto listen_failed; + } + /* 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); + if (slisten < 0) { + error_setg_errno(errp, errno, + "Failed to recreate failed listening socket"); + goto listen_failed; + } } + } + error_setg_errno(errp, errno, + socket_created ? + "Failed to find an available port" : + "Failed to create a socket"); +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);