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

[John Ferlan]

On 01/10/2013 04:55 PM, Matthias Bolte wrote:
> 2013/1/10 John Ferlan <jferlan at redhat.com>:
>> 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.
> 
> Well, you're commit message didn't state this as a false positive, so
> I looked carefully at esxOpen whether or not Coverity was right here,
> but couldn't find anything.
> 

Sorry- I had put that in the cover letter

> Anyway, I'd like to simplify the logic in esxOpen a bit the way Eric
> suggested. Here's a patch that does this:
> 
> https://www.redhat.com/archives/libvir-list/2013-January/msg00657.html
>