Re: [libvirt] [PATCH 02/14] esx: Address resource leak found by Coverity
On 01/10/2013 10:49 AM, Matthias Bolte wrote:
2013/1/10 Eric Blake <eblake(a)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.