[libvirt] [PATCH 3/6] rpc: Plug memory leak on virNetClientSendInternal() error path

[Eric Blake]

On 11/29/2011 11:40 PM, Alex Jia wrote:
> On 11/30/2011 02:26 PM, Wen Congyang wrote:
>> At 11/30/2011 01:57 PM, ajia at redhat.com Write:
>>> From: Alex Jia<ajia at redhat.com>
>>>
>>> Detected by Coverity. Leak introduced in commit 673adba.
>>>

>> So I think we should not free call if it is still on the queue.
> Yeah, it's a inappropriate place, in addition, in 'cleanup' label, if
> ret !=1, also will free 'all', then 'unlock' label still do this, the
> following should be a right place:
> 
> 1708     if (!client->sock || client->wantClose) {
> 1709         virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
> 1710                     _("client socket is closed"));
> *1711       VIR_FREE(call);*
> 1712         goto unlock;
> 1713     }

Closer, but still not quite right either.  You solved the failure if
!client->sock, but still left in the bug that a failed virCondInit did
goto cleanup, which then did a call to virCondDestroy on uninitialized
data, which is undefined by POSIX.

Rearranging some code and consolidating to one label will solve both
bugs (the leak if !client->sock, and the incorrect virCondDestroy call
on virCondInit failure), while still avoiding to free call on a partial
send.  Here's what I'm pushing under your name.

diff --git c/src/rpc/virnetclient.c w/src/rpc/virnetclient.c
index a738129..5165c8d 100644
--- c/src/rpc/virnetclient.c
+++ w/src/rpc/virnetclient.c
@@ -1705,13 +1705,13 @@ static int
virNetClientSendInternal(virNetClientPtr client,

     virNetClientLock(client);

     if (!client->sock || client->wantClose) {
         virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
                     _("client socket is closed"));
-        goto unlock;
+        goto cleanup;
     }

     if (virCondInit(&call->cond) < 0) {
         virNetError(VIR_ERR_INTERNAL_ERROR, "%s",
                     _("cannot initialize condition variable"));
         goto cleanup;
@@ -1726,22 +1726,22 @@ static int
virNetClientSendInternal(virNetClientPtr client,
     call->expectReply = expectReply;
     call->nonBlock = nonBlock;
     call->haveThread = true;

     ret = virNetClientIO(client, call);

-cleanup:
     /* If partially sent, then the call is still on the dispatch queue */
     if (ret == 1) {
         call->haveThread = false;
     } else {
         ignore_value(virCondDestroy(&call->cond));
-        VIR_FREE(call);
     }

-unlock:
+cleanup:
+    if (ret != 1)
+        VIR_FREE(call);
     virNetClientUnlock(client);
     return ret;
 }


 /*

-- 
Eric Blake   eblake at 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: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20111130/d2e4e881/attachment-0001.sig>