[libvirt] [PATCH 0/2] Fix another possible memory leak in nwfilter
Oh lucky me - why am I stuck in nwfilter land.... The first patch just cleans up the code and the second one resolves the memory leak problem as well as a general error path problem. Coverity noted that the error path wasn't checked - this is because other recent changes in the module caused Coverity to decide to rethink about this one noticing the lack of an error check. Of course upon more visual inspection, the memory leak was obvious too. John Ferlan (2): nwfilter: Clean up virNWFilterDetermineMissingVarsRec returns nwfilter: Fix memory leak and error path src/nwfilter/nwfilter_gentech_driver.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) -- 2.13.5
Rather than using loop break;'s in order to force a return of rc = -1, let's just return -1 immediately on the various error paths and then return 0 on the success path. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 6758200b5..7a3d115ba 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -494,7 +494,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, virNWFilterDriverStatePtr driver) { virNWFilterObjPtr obj; - int rc = 0; + int rc; size_t i, j; virNWFilterDefPtr next_filter; virNWFilterDefPtr newNext_filter; @@ -515,15 +515,13 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, virNWFilterVarAccessPrint(rule->varAccess[j], &buf); if (virBufferError(&buf)) { virReportOOMError(); - rc = -1; - break; + return -1; } val = virNWFilterVarValueCreateSimpleCopyValue("1"); if (!val) { virBufferFreeAndReset(&buf); - rc = -1; - break; + return -1; } varAccess = virBufferContentAndReset(&buf); @@ -532,21 +530,16 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, VIR_FREE(varAccess); } } - if (rc) - break; } else if (inc) { VIR_DEBUG("Following filter %s", inc->filterref); if (!(obj = virNWFilterObjListFindInstantiateFilter(driver->nwfilters, - inc->filterref))) { - rc = -1; - break; - } + inc->filterref))) + return -1; /* create a temporary hashmap for depth-first tree traversal */ if (!(tmpvars = virNWFilterCreateVarsFrom(inc->params, vars))) { - rc = -1; virNWFilterObjUnlock(obj); - break; + return -1; } next_filter = virNWFilterObjGetDef(obj); @@ -571,10 +564,10 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, virNWFilterObjUnlock(obj); if (rc < 0) - break; + return -1; } } - return rc; + return 0; } -- 2.13.5
On Fri, Sep 29, 2017 at 09:31:08AM -0400, John Ferlan wrote:
Rather than using loop break;'s in order to force a return of rc = -1, let's just return -1 immediately on the various error paths and then return 0 on the success path.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 6758200b5..7a3d115ba 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -494,7 +494,7 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, virNWFilterDriverStatePtr driver) { virNWFilterObjPtr obj; - int rc = 0; + int rc;
I prefer the variables to be initialized on the stack, it prevents the unnecessary "may be uninitialized" warnings. It's a pity the other variables are uninitialized, but that's for a patch for another time. With this hunk dropped: Reviewed-by: Erik Skultety <eskultet@redhat.com>
Found by Coverity. If virNWFilterHashTablePut, then the 3rd arg @val must be free'd since it would be leaked. This also fixes potential problem on the error path where the caller could assume the virNWFilterHashTablePut was successful when in fact it failed leading to other issues. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/nwfilter/nwfilter_gentech_driver.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 7a3d115ba..500197bae 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -525,9 +525,12 @@ virNWFilterDetermineMissingVarsRec(virNWFilterDefPtr filter, } varAccess = virBufferContentAndReset(&buf); - virNWFilterHashTablePut(missing_vars, varAccess, - val); + rc = virNWFilterHashTablePut(missing_vars, varAccess, val); VIR_FREE(varAccess); + if (rc < 0) { + virNWFilterVarValueFree(val); + return -1; + } } } } else if (inc) { -- 2.13.5
On Fri, Sep 29, 2017 at 09:31:09AM -0400, John Ferlan wrote:
Found by Coverity. If virNWFilterHashTablePut, then the 3rd arg @val must be free'd since it would be leaked.
This also fixes potential problem on the error path where the caller could assume the virNWFilterHashTablePut was successful when in fact it failed leading to other issues.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
participants (2)
-
Erik Skultety -
John Ferlan