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

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(a)redhat.com Write: >>> From: Alex Jia<ajia(a)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; } /*