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

2013/1/10 John Ferlan <jferlan(a)redhat.com&gt;: > On 01/10/2013 10:49 AM, Matthias Bolte wrote: >> 2013/1/10 Eric Blake <eblake(a)redhat.com&gt;: >>> 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