8:31 a.m.
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
8:31 a.m.
New subject: [libvirt] [PATCH 1/2] nwfilter: Clean up virNWFilterDetermineMissingVarsRec returns
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(a)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
5:50 a.m.
New subject: [libvirt] [PATCH 1/2] nwfilter: Clean up virNWFilterDetermineMissingVarsRec returns
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(a)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(a)redhat.com>
8:31 a.m.
New subject: [libvirt] [PATCH 2/2] nwfilter: Fix memory leak and error path
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(a)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
7:06 a.m.
New subject: [libvirt] [PATCH 2/2] nwfilter: Fix memory leak and error path
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(a)redhat.com>
Reviewed-by: Erik Skultety <eskultet(a)redhat.com>
2607
days inactive
2610
days old
4 comments
2 participants
participants (2)
-
Erik Skultety -
John Ferlan