[libvirt] [PATCH 01/14] phyp: Resolve some file descriptor leaks

[Eric Blake]

On 01/09/2013 07:54 AM, John Ferlan wrote:
> The phypUUIDTable_Push and phypUUIDTable_Pull leaked their fd's on normal
> return.  I also noted that the Read function had a cut-n-paste error from
> the write function on a couple of VIR_WARN's
> 
> The openSSHSession leaked the sock on the failure path.  Additionally that
> turns into the internal_socket in the phypOpen code.  That was neither saved
> nor closed on any path. So I used the connnection_data->sock field to save
> the socket for eventual close. Of interest here is that phypExec used the
> connection_data->sock field even though it had never been initialized.
> ---
>  src/phyp/phyp_driver.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
> index f89871f..b74cab8 100644
> --- a/src/phyp/phyp_driver.c
> +++ b/src/phyp/phyp_driver.c
> @@ -577,6 +577,7 @@ phypUUIDTable_Push(virConnectPtr conn)
>          libssh2_channel_free(channel);
>          channel = NULL;
>      }
> +    VIR_FORCE_FCLOSE(fd);

Who named their FILE* 'fd' anyways?  But not your fault that you are
trying to clean up some notoriously hairy code.

>      virBufferFreeAndReset(&username);
>      return 0;

Hmm, the code feels rather repetitive:

    if (channel) {
        libssh2_channel_send_eof(channel);
        libssh2_channel_wait_eof(channel);
        libssh2_channel_wait_closed(channel);
        libssh2_channel_free(channel);
        channel = NULL;
    }
    VIR_FORCE_FCLOSE(fd);
    virBufferFreeAndReset(&username);
    return 0;

err:
    if (channel) {
        libssh2_channel_send_eof(channel);
        libssh2_channel_wait_eof(channel);
        libssh2_channel_wait_closed(channel);
        libssh2_channel_free(channel);
        channel = NULL;
    }
    VIR_FORCE_FCLOSE(fd);
    return -1;
}

Also, the virBufferFreeAndReset(&username) on the success path is
useless (the only way to get there is if username was already freed
earlier), and the rest of the code is identical except for the return
value.  Oh, and the entire 'username' variable is pointless - it is
either empty or precisely conn->uri->user; no need to malloc a buffer to
copy a string when we can just use the string directly.

How about we do this instead:

diff --git i/src/phyp/phyp_driver.c w/src/phyp/phyp_driver.c
index f89871f..87a94c0 100644
--- i/src/phyp/phyp_driver.c
+++ w/src/phyp/phyp_driver.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2010-2012 Red Hat, Inc.
+ * Copyright (C) 2010-2013 Red Hat, Inc.
  * Copyright IBM Corp. 2009
  *
  * phyp_driver.c: ssh layer to access Power Hypervisors
@@ -493,42 +493,30 @@ phypUUIDTable_Push(virConnectPtr conn)
     ConnectionData *connection_data = conn->networkPrivateData;
     LIBSSH2_SESSION *session = connection_data->session;
     LIBSSH2_CHANNEL *channel = NULL;
-    virBuffer username = VIR_BUFFER_INITIALIZER;
     struct stat local_fileinfo;
     char buffer[1024];
     int rc = 0;
-    FILE *fd = NULL;
+    FILE *f = NULL;
     size_t nread, sent;
     char *ptr;
     char local_file[] = "./uuid_table";
     char *remote_file = NULL;
+    int ret = -1;

-    if (conn->uri->user != NULL) {
-        virBufferAdd(&username, conn->uri->user, -1);
-
-        if (virBufferError(&username)) {
-            virBufferFreeAndReset(&username);
-            virReportOOMError();
-            goto err;
-        }
-    }
-
-    if (virAsprintf
-        (&remote_file, "/home/%s/libvirt_uuid_table",
-         virBufferContentAndReset(&username))
-        < 0) {
+    if (virAsprintf(&remote_file, "/home/%s/libvirt_uuid_table",
+                    NULLSTR(conn->uri->user)) < 0) {
         virReportOOMError();
-        goto err;
+        goto cleanup;
     }

     if (stat(local_file, &local_fileinfo) == -1) {
         VIR_WARN("Unable to stat local file.");
-        goto err;
+        goto cleanup;
     }

-    if (!(fd = fopen(local_file, "rb"))) {
+    if (!(f = fopen(local_file, "rb"))) {
         VIR_WARN("Unable to open local file.");
-        goto err;
+        goto cleanup;
     }

     do {
@@ -539,18 +527,18 @@ phypUUIDTable_Push(virConnectPtr conn)

         if ((!channel) && (libssh2_session_last_errno(session) !=
                            LIBSSH2_ERROR_EAGAIN))
-            goto err;
+            goto cleanup;
     } while (!channel);

     do {
-        nread = fread(buffer, 1, sizeof(buffer), fd);
+        nread = fread(buffer, 1, sizeof(buffer), f);
         if (nread <= 0) {
-            if (feof(fd)) {
+            if (feof(f)) {
                 /* end of file */
                 break;
             } else {
                 VIR_ERROR(_("Failed to read from %s"), local_file);
-                goto err;
+                goto cleanup;
             }
         }
         ptr = buffer;
@@ -570,17 +558,9 @@ phypUUIDTable_Push(virConnectPtr conn)
         } while (rc > 0 && sent < nread);
     } while (1);

-    if (channel) {
-        libssh2_channel_send_eof(channel);
-        libssh2_channel_wait_eof(channel);
-        libssh2_channel_wait_closed(channel);
-        libssh2_channel_free(channel);
-        channel = NULL;
-    }
-    virBufferFreeAndReset(&username);
-    return 0;
+    ret = 0;

-err:
+cleanup:
     if (channel) {
         libssh2_channel_send_eof(channel);
         libssh2_channel_wait_eof(channel);
@@ -588,8 +568,8 @@ err:
         libssh2_channel_free(channel);
         channel = NULL;
     }
-    VIR_FORCE_FCLOSE(fd);
-    return -1;
+    VIR_FORCE_FCLOSE(f);
+    return ret;
 }

 static int


[I didn't look at the rest of the patch, I was so distracted by the poor
quality of this one function]

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130109/535b70e8/attachment-0001.sig>