[Qemu-devel] [PATCH v2] sockets: avoid leak of listen file descriptor

[Qemu-devel] [PATCH v2] sockets: avoid leak of listen file descriptor

[Daniel P. Berrange]

If we iterate over the full port range without successfully binding+listening
on the socket, we'll try the next address, whereupon we overwrite the slisten
file descriptor variable without closing it.

Rather than having two places where we open + close socket FDs on different
iterations of nested for loops, re-arrange the code to always open+close
within the same loop iteration.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 util/qemu-sockets.c | 52 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index b47fb45885..4587829552 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -207,7 +207,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
     char uaddr[INET6_ADDRSTRLEN+1];
     char uport[33];
     int rc, port_min, port_max, p;
-    int slisten = 0;
+    int slisten = -1;
     int saved_errno = 0;
     bool socket_created = false;
     Error *err = NULL;
@@ -267,31 +267,42 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
 		        uaddr,INET6_ADDRSTRLEN,uport,32,
 		        NI_NUMERICHOST | NI_NUMERICSERV);
 
-        slisten = create_fast_reuse_socket(e);
-        if (slisten < 0) {
-            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);
-            rc = try_bind(slisten, saddr, e);
-            if (rc) {
-                if (errno == EADDRINUSE) {
+
+            slisten = create_fast_reuse_socket(e);
+            if (slisten < 0) {
+                /* First time we expect we might fail to create the socket
+                 * eg if 'e' has AF_INET6 but ipv6 kmod is not loaded.
+                 * Later iterations should always succeeed if first iteration
+                 * worked though, so treat that as fatal.
+                 */
+                if (p == port_min) {
                     continue;
                 } else {
-                    error_setg_errno(errp, errno, "Failed to bind socket");
+                    error_setg_errno(errp, errno,
+                                     "Failed to recreate failed listening 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;
+            socket_created = true;
+
+            rc = try_bind(slisten, saddr, e);
+            if (rc < 0) {
+                if (errno != EADDRINUSE) {
+                    error_setg_errno(errp, errno, "Failed to bind socket");
+                    goto listen_failed;
+                }
+            } else {
+                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
@@ -299,12 +310,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
              * 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;
-            }
+            slisten = -1;
         }
     }
     error_setg_errno(errp, errno,
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2] sockets: avoid leak of listen file descriptor

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

On 10/20/2017 09:12 AM, Daniel P. Berrange wrote:
> If we iterate over the full port range without successfully binding+listening
> on the socket, we'll try the next address, whereupon we overwrite the slisten
> file descriptor variable without closing it.
> 
> Rather than having two places where we open + close socket FDs on different
> iterations of nested for loops, re-arrange the code to always open+close
> within the same loop iteration.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  util/qemu-sockets.c | 52 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 23 deletions(-)
> 

> +            slisten = create_fast_reuse_socket(e);
> +            if (slisten < 0) {
> +                /* First time we expect we might fail to create the socket
> +                 * eg if 'e' has AF_INET6 but ipv6 kmod is not loaded.
> +                 * Later iterations should always succeeed if first iteration

s/succeeed/succeed/

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]