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

[Matthias Bolte]

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.

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

-- 
Matthias Bolte
http://photron.blogspot.com