Google
  Web www.spinics.net

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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On 12/01/2011 07:11 AM, Eric Blake wrote:
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@xxxxxxxxxx Write:
From: Alex Jia<ajia@xxxxxxxxxx>

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.
Yeah, I just saw codes again. Eric, thanks.
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;
  }


  /*


--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Virt Tools]     [Libvirt Users]     [Fedora Users]     [Fedora Legacy]     [Fedora Maintainers]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [Yosemite Photos]     [KDE Users]     [Fedora Tools]

Powered by Linux

Google
  Web www.spinics.net