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&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.