[libvirt] [PATCH 3/6] rpc: Plug memory leak on virNetClientSendInternal() error path
Alex Jia
ajia at redhat.com
Thu Dec 1 03:01:39 UTC 2011
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 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.
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;
> }
>
>
> /*
>
More information about the libvir-list
mailing list