[libvirt] [PATCH] esx: Avoid Coverity warning about resource leak in esxOpen
Commit 4445e16bfa8056980ac643fabf17186f9e685925 changed the signature of esxConnectToHost and esxConnectToVCenter by replacing the esxPrivate pointer with virConnectPtr. The esxPrivate pointer was then retrieved again from virConnectPtr's privateData. This resulted in a NULL pointer dereference, because the privateData pointer was not yet initialized at the point where esxConnectToHost and esxConnectToVCenter are called. This was fixed in commit b126715a48cd0cbe32ec6468c267cd8cf2961c55 that moved the initialization of privateData before the problematic calls. Coverity tagged the esxPrivate pointer a potentially leaked because of the conditional free call. But this is a false positive, there is not actual leak. Avoid this warning from Coverity by making the call to esxFreePrivate unconditional and changing esxConnectToHost and esxConnectToVCenter back to take a esxPrivate pointer directly. This allows to assign esxPrivate to the virConnectPtr's privateData pointer as one of the last steps in esxOpen making it more obvious that it is not initialized during the earlier steps of esxOpen. --- This patch is meant as a replacement this patch: https://www.redhat.com/archives/libvir-list/2013-January/msg00530.html src/esx/esx_driver.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1366c81..dad10a1 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -650,7 +650,8 @@ esxCapsInit(esxPrivate *priv) static int -esxConnectToHost(virConnectPtr conn, +esxConnectToHost(esxPrivate *priv, + virConnectPtr conn, virConnectAuthPtr auth, char **vCenterIpAddress) { @@ -663,7 +664,6 @@ esxConnectToHost(virConnectPtr conn, esxVI_String *propertyNameList = NULL; esxVI_ObjectContent *hostSystem = NULL; esxVI_Boolean inMaintenanceMode = esxVI_Boolean_Undefined; - esxPrivate *priv = conn->privateData; esxVI_ProductVersion expectedProductVersion = STRCASEEQ(conn->uri->scheme, "esx") ? esxVI_ProductVersion_ESX : esxVI_ProductVersion_GSX; @@ -785,7 +785,8 @@ esxConnectToHost(virConnectPtr conn, static int -esxConnectToVCenter(virConnectPtr conn, +esxConnectToVCenter(esxPrivate *priv, + virConnectPtr conn, virConnectAuthPtr auth, const char *hostname, const char *hostSystemIpAddress) @@ -796,7 +797,6 @@ esxConnectToVCenter(virConnectPtr conn, char *unescapedPassword = NULL; char *password = NULL; char *url = NULL; - esxPrivate *priv = conn->privateData; if (hostSystemIpAddress == NULL && (priv->parsedUri->path == NULL || STREQ(priv->parsedUri->path, "/"))) { @@ -1008,8 +1008,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 @@ -1036,7 +1034,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, if (STRCASEEQ(conn->uri->scheme, "esx") || STRCASEEQ(conn->uri->scheme, "gsx")) { /* Connect to host */ - if (esxConnectToHost(conn, auth, + if (esxConnectToHost(priv, conn, auth, &potentialVCenterIpAddress) < 0) { goto cleanup; } @@ -1075,7 +1073,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, } } - if (esxConnectToVCenter(conn, auth, + if (esxConnectToVCenter(priv, conn, auth, vCenterIpAddress, priv->host->ipAddress) < 0) { goto cleanup; @@ -1085,7 +1083,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, priv->primary = priv->host; } else { /* VPX */ /* Connect to vCenter */ - if (esxConnectToVCenter(conn, auth, + if (esxConnectToVCenter(priv, conn, auth, conn->uri->server, NULL) < 0) { goto cleanup; @@ -1101,13 +1099,12 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, goto cleanup; } + conn->privateData = priv; + priv = NULL; result = VIR_DRV_OPEN_SUCCESS; cleanup: - if (result == VIR_DRV_OPEN_ERROR) { - esxFreePrivate(&priv); - } - + esxFreePrivate(&priv); VIR_FREE(potentialVCenterIpAddress); return result; -- 1.7.9.5
On 01/10/2013 02:47 PM, Matthias Bolte wrote:
Commit 4445e16bfa8056980ac643fabf17186f9e685925 changed the signature of esxConnectToHost and esxConnectToVCenter by replacing the esxPrivate pointer with virConnectPtr. The esxPrivate pointer was then retrieved again from virConnectPtr's privateData. This resulted in a NULL pointer dereference, because the privateData pointer was not yet initialized at the point where esxConnectToHost and esxConnectToVCenter are called.
This was fixed in commit b126715a48cd0cbe32ec6468c267cd8cf2961c55 that moved the initialization of privateData before the problematic calls.
Coverity tagged the esxPrivate pointer a potentially leaked because of the conditional free call. But this is a false positive, there is not actual leak.
Avoid this warning from Coverity by making the call to esxFreePrivate unconditional and changing esxConnectToHost and esxConnectToVCenter back to take a esxPrivate pointer directly. This allows to assign esxPrivate to the virConnectPtr's privateData pointer as one of the last steps in esxOpen making it more obvious that it is not initialized during the earlier steps of esxOpen. ---
ACK.
This patch is meant as a replacement this patch:
https://www.redhat.com/archives/libvir-list/2013-January/msg00530.html
John, thanks again for pointing out the Coverity warning, and doing a first pass analysis, even if we didn't end up using your patch. One of the nice things about open source is that by making the problems public, we can often come up with even better patches. And Matthias, thanks for jumping in to improve the code you originally wrote. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/10/2013 05:27 PM, Eric Blake wrote:
On 01/10/2013 02:47 PM, Matthias Bolte wrote:
Commit 4445e16bfa8056980ac643fabf17186f9e685925 changed the signature of esxConnectToHost and esxConnectToVCenter by replacing the esxPrivate pointer with virConnectPtr. The esxPrivate pointer was then retrieved again from virConnectPtr's privateData. This resulted in a NULL pointer dereference, because the privateData pointer was not yet initialized at the point where esxConnectToHost and esxConnectToVCenter are called.
This was fixed in commit b126715a48cd0cbe32ec6468c267cd8cf2961c55 that moved the initialization of privateData before the problematic calls.
Coverity tagged the esxPrivate pointer a potentially leaked because of the conditional free call. But this is a false positive, there is not actual leak.
Avoid this warning from Coverity by making the call to esxFreePrivate unconditional and changing esxConnectToHost and esxConnectToVCenter back to take a esxPrivate pointer directly. This allows to assign esxPrivate to the virConnectPtr's privateData pointer as one of the last steps in esxOpen making it more obvious that it is not initialized during the earlier steps of esxOpen. ---
ACK.
Still get a Coverity error, but I think it has something to do with the VIR_FREE(*priv); done in the esxFreePrivate(). I'm beginning to believe it's a Coverity "issue". I have no idea why what I had done previously was able to bypass the error. I'm trying to figure out the right mechanism to ask in the Coverity world. I've been chasing issues in the 'esx_vi.c' and 'esx_vi_types.c' code today which is why I note this. I was able to put together some sample code that removes all the macros and it ended up being very similar to this code ironically.
This patch is meant as a replacement this patch:
https://www.redhat.com/archives/libvir-list/2013-January/msg00530.html
John, thanks again for pointing out the Coverity warning, and doing a first pass analysis, even if we didn't end up using your patch. One of the nice things about open source is that by making the problems public, we can often come up with even better patches. And Matthias, thanks for jumping in to improve the code you originally wrote.
I'm perfectly fine with someone else making changes.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/10/2013 05:14 PM, John Ferlan wrote:
Still get a Coverity error, but I think it has something to do with the VIR_FREE(*priv); done in the esxFreePrivate(). I'm beginning to believe it's a Coverity "issue". I have no idea why what I had done previously was able to bypass the error. I'm trying to figure out the right mechanism to ask in the Coverity world.
Can you paste the actual Coverity analysis on this code? Knowing what call stack it is tracing through will help others look at this setup to see why it is still complaining. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/10/2013 07:28 PM, Eric Blake wrote:
On 01/10/2013 05:14 PM, John Ferlan wrote:
Still get a Coverity error, but I think it has something to do with the VIR_FREE(*priv); done in the esxFreePrivate(). I'm beginning to believe it's a Coverity "issue". I have no idea why what I had done previously was able to bypass the error. I'm trying to figure out the right mechanism to ask in the Coverity world.
Can you paste the actual Coverity analysis on this code? Knowing what call stack it is tracing through will help others look at this setup to see why it is still complaining.
The full analysis is a bit large so I cut it down a bit. The attached esx.html has the trace in question - the links within won't work, but it hopefully gives you a picture of the trace. The key is that the esxFreePrivate(&priv); frees both the data in the structure and the structure itself which is what I believe is confusing coverity. If I add a VIR_FREE(priv) after this call - then the error goes away even though it's redundant with the VIR_FREE(*priv) in esxFreePrivate().
This issue (and the same for hyperv) was tracked down to a Coverity problem. For context see: https://www.redhat.com/archives/libvir-list/2013-January/msg01504.html Essentially the esxFreePrivate(&priv); and subsequent VIR_FREE(*priv) caused the issue. Also see: https://communities.coverity.com/thread/2655 John On 01/10/2013 04:47 PM, Matthias Bolte wrote:
Commit 4445e16bfa8056980ac643fabf17186f9e685925 changed the signature of esxConnectToHost and esxConnectToVCenter by replacing the esxPrivate pointer with virConnectPtr. The esxPrivate pointer was then retrieved again from virConnectPtr's privateData. This resulted in a NULL pointer dereference, because the privateData pointer was not yet initialized at the point where esxConnectToHost and esxConnectToVCenter are called.
This was fixed in commit b126715a48cd0cbe32ec6468c267cd8cf2961c55 that moved the initialization of privateData before the problematic calls.
Coverity tagged the esxPrivate pointer a potentially leaked because of the conditional free call. But this is a false positive, there is not actual leak.
Avoid this warning from Coverity by making the call to esxFreePrivate unconditional and changing esxConnectToHost and esxConnectToVCenter back to take a esxPrivate pointer directly. This allows to assign esxPrivate to the virConnectPtr's privateData pointer as one of the last steps in esxOpen making it more obvious that it is not initialized during the earlier steps of esxOpen. ---
This patch is meant as a replacement this patch:
https://www.redhat.com/archives/libvir-list/2013-January/msg00530.html
src/esx/esx_driver.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1366c81..dad10a1 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -650,7 +650,8 @@ esxCapsInit(esxPrivate *priv)
static int -esxConnectToHost(virConnectPtr conn, +esxConnectToHost(esxPrivate *priv, + virConnectPtr conn, virConnectAuthPtr auth, char **vCenterIpAddress) { @@ -663,7 +664,6 @@ esxConnectToHost(virConnectPtr conn, esxVI_String *propertyNameList = NULL; esxVI_ObjectContent *hostSystem = NULL; esxVI_Boolean inMaintenanceMode = esxVI_Boolean_Undefined; - esxPrivate *priv = conn->privateData; esxVI_ProductVersion expectedProductVersion = STRCASEEQ(conn->uri->scheme, "esx") ? esxVI_ProductVersion_ESX : esxVI_ProductVersion_GSX; @@ -785,7 +785,8 @@ esxConnectToHost(virConnectPtr conn,
static int -esxConnectToVCenter(virConnectPtr conn, +esxConnectToVCenter(esxPrivate *priv, + virConnectPtr conn, virConnectAuthPtr auth, const char *hostname, const char *hostSystemIpAddress) @@ -796,7 +797,6 @@ esxConnectToVCenter(virConnectPtr conn, char *unescapedPassword = NULL; char *password = NULL; char *url = NULL; - esxPrivate *priv = conn->privateData;
if (hostSystemIpAddress == NULL && (priv->parsedUri->path == NULL || STREQ(priv->parsedUri->path, "/"))) { @@ -1008,8 +1008,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 @@ -1036,7 +1034,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, if (STRCASEEQ(conn->uri->scheme, "esx") || STRCASEEQ(conn->uri->scheme, "gsx")) { /* Connect to host */ - if (esxConnectToHost(conn, auth, + if (esxConnectToHost(priv, conn, auth, &potentialVCenterIpAddress) < 0) { goto cleanup; } @@ -1075,7 +1073,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, } }
- if (esxConnectToVCenter(conn, auth, + if (esxConnectToVCenter(priv, conn, auth, vCenterIpAddress, priv->host->ipAddress) < 0) { goto cleanup; @@ -1085,7 +1083,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, priv->primary = priv->host; } else { /* VPX */ /* Connect to vCenter */ - if (esxConnectToVCenter(conn, auth, + if (esxConnectToVCenter(priv, conn, auth, conn->uri->server, NULL) < 0) { goto cleanup; @@ -1101,13 +1099,12 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, goto cleanup; }
+ conn->privateData = priv; + priv = NULL; result = VIR_DRV_OPEN_SUCCESS;
cleanup: - if (result == VIR_DRV_OPEN_ERROR) { - esxFreePrivate(&priv); - } - + esxFreePrivate(&priv); VIR_FREE(potentialVCenterIpAddress);
return result;
2013/1/22 John Ferlan <jferlan@redhat.com>:
This issue (and the same for hyperv) was tracked down to a Coverity problem. For context see:
https://www.redhat.com/archives/libvir-list/2013-January/msg01504.html
Essentially the esxFreePrivate(&priv); and subsequent VIR_FREE(*priv) caused the issue.
Also see: https://communities.coverity.com/thread/2655
John
Thanks for figuring out the real cause. But I still like the code simplification in my patch. So I changed the commit message and pushed it. -- Matthias Bolte http://photron.blogspot.com
participants (3)
-
Eric Blake -
John Ferlan -
Matthias Bolte