[libvirt] [PATCH 02/14] esx: Address resource leak found by Coverity

[John Ferlan]

On 01/10/2013 10:49 AM, Matthias Bolte wrote:
> 2013/1/10 Eric Blake <eblake at redhat.com>:
>> On 01/09/2013 07:54 AM, John Ferlan wrote:
>>> Because result was used to determine whether or not to free 'priv'
>>> resources Coverity tagged the code as having a resource leak. This
>>> change addresses that concern.
>>> ---
>>>  src/esx/esx_driver.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c
>>> index 1366c81..9befa38 100644
>>> --- a/src/esx/esx_driver.c
>>> +++ b/src/esx/esx_driver.c
>>> @@ -998,6 +998,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth,
>>>          virReportOOMError();
>>>          goto cleanup;
>>>      }
>>> +    conn->privateData = priv;
>>>
>>>      if (esxUtil_ParseUri(&priv->parsedUri, conn->uri) < 0) {
>>>          goto cleanup;
>>
>> I
>>
>>> @@ -1008,8 +1009,6 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth,
>>>      priv->supportsLongMode = esxVI_Boolean_Undefined;
>>>      priv->usedCpuTimeCounterId = -1;
>>>
>>> -    conn->privateData = priv;
>>> -
>>>      /*
>>>       * Set the port dependent on the transport protocol if no port is
>>>       * specified. This allows us to rely on the port parameter being
>>> @@ -1104,9 +1103,10 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth,
>>>      result = VIR_DRV_OPEN_SUCCESS;
>>>
>>>    cleanup:
>>> -    if (result == VIR_DRV_OPEN_ERROR) {
>>> +    if (result == VIR_DRV_OPEN_ERROR)
>>> +        conn->privateData = NULL;
>>> +    if (priv && !conn->privateData)
>>>          esxFreePrivate(&priv);
>>> -    }
>>
>> This feels a bit complex;
> 
> I agree.
> 

+1... for a reason though...

>> I had to go read esxFreePrivate() to make sure
>> it would behave if called on a partial object.  Would it be any easier
>> to delay the assignment to conn->privateData, and use transfer of
>> ownership semantics, so that the cleanup is unconditional?
>>
>>     conn->privateData = priv;
>>     priv = NULL;
>>     result = VIR_DRV_OPEN_SUCCESS;
>> cleanup:
>>     esxFreePrivate(&priv);
> 
> No! This cannot be done that easily, see commit
> b126715a48cd0cbe32ec6468c267cd8cf2961c55 "esx: Fix segfault in
> esxConnectToHost".
> 

And this is why...  It is a "false positive" and possibly could be dealt
with by adding a /* coverity[leaked_storage] */ in the right place as
well.  Although I've had some issues trying to do that for another set
of changes because the going out of scope happens on the return and
putting a coverity commit prior to a return hasn't done what I'd expect.

> We cannot move conn->privateData = priv; further down with the current
> code, because it is already used in between. But I like the simplicity
> of your suggestion.
> 
> I'll me come up with a patch for this later today.
>