10:28 a.m.
I'm sure it'll be felt one or two could just be false positives,
but I have 35-40 of true false positives and it seems at least
these go above just noise on the channel.
Perhaps the most difficult one to immediately see was the libxl
refcnt patch. That involves a little bit of theory and has been
in my noise pile for a while until I noted that the @args is
being added in a loop to a callback function that just Unref's
it when done. So if there was more than 1 IP Address, then all
sorts of fun things could happen. Without any change, the Alloc
is matched by the Unref, but with the change we add a Ref to
match each Unref in the I/O loop and we just ensure the Unref
is done for the path that put @args into the I/O callback.
I also think the nwfilter patch was "interesting" insomuch as
it has my "favorite" 'if (int-value) {' condition. IOW, if
not zero, then do something. What became "interesting" is that
the virNWFilterIPAddrMapDelIPAddr could return -1 if the
virHashLookup on @req->binding->portdevname returned NULL,
so when "shrinking" the code to only call the instantiation
for/when there was an IP Address found resolves a couple of
issues in the code.
John Ferlan (7):
lxc: Only check @nparams in lxcDomainBlockStatsFlags
libxl: Fix possible object refcnt issue
tests: Inline a sysconf call for linuxCPUStatsToBuf
util: Data overrun may lead to divide by zero
tests: Alter logic in testCompareXMLToDomConfig
tests: Use STRNEQ_NULLABLE
nwfilter: Alter virNWFilterSnoopReqLeaseDel logic
src/libxl/libxl_migration.c | 4 ++--
src/lxc/lxc_driver.c | 2 +-
src/nwfilter/nwfilter_dhcpsnoop.c | 9 ++++-----
src/util/virutil.c | 11 +++++------
tests/commandtest.c | 4 ++--
tests/libxlxml2domconfigtest.c | 11 +++++------
tests/virhostcputest.c | 12 ++++++++++--
7 files changed, 29 insertions(+), 24 deletions(-)
--
2.17.1
10:28 a.m.
New subject: [libvirt] [PATCH 1/7] lxc: Only check @nparams in lxcDomainBlockStatsFlags
Remove the "!params" check from the condition since it's possible
someone could pass a non NULL value there, but a 0 for the nparams
and thus continue on. The external API only checks if @nparams is
non-zero, then check for NULL @params.
Found by Coverity
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/lxc/lxc_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index d95ed63c18..f732305649 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -2388,7 +2388,7 @@ lxcDomainBlockStatsFlags(virDomainPtr dom,
/* We don't return strings, and thus trivially support this flag. */
flags &= ~VIR_TYPED_PARAM_STRING_OKAY;
- if (!params && !*nparams) {
+ if (!*nparams) {
*nparams = LXC_NB_DOMAIN_BLOCK_STAT_PARAM;
return 0;
}
--
2.17.1
10:28 a.m.
New subject: [libvirt] [PATCH 2/7] libxl: Fix possible object refcnt issue
When libxlDomainMigrationDstPrepare adds the @args to an
virNetSocketAddIOCallback using libxlMigrateDstReceive as
the target of the virNetSocketIOFunc @func with the knowledge
that the libxlMigrateDstReceive will virObjectUnref @args
at the end thus not needing to Unref during normal processing
for libxlDomainMigrationDstPrepare.
However, Coverity believes there's an issue with this. The
problem is there can be @nsocks virNetSocketAddIOCallback's
added, but only one virObjectUnref. That means the first
one done will Unref and the subsequent callers may not get
the @args (or @opaque) as they expected. If there's only
one socket returned from virNetSocketNewListenTCP, then sure
that works. However, if it returned more than one there's
going to be a problem.
To resolve this, since we start with 1 reference from the
virObjectNew for @args, we will add 1 reference for each
time @args is used for virNetSocketAddIOCallback. Then
since libxlDomainMigrationDstPrepare would be done with
@args, move it's virObjectUnref from the error: label to
the done: label (since error: falls through). That way
once the last IOCallback is done, then @args will be freed.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libxl/libxl_migration.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index fc7ccb53d0..88d7ecf2ce 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -793,7 +793,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
if (virNetSocketAddIOCallback(socks[i],
VIR_EVENT_HANDLE_READABLE,
libxlMigrateDstReceive,
- args,
+ virObjectRef(args),
NULL) < 0)
continue;
@@ -815,7 +815,6 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
virObjectUnref(socks[i]);
}
VIR_FREE(socks);
- virObjectUnref(args);
if (priv) {
virPortAllocatorRelease(priv->migrationPort);
priv->migrationPort = 0;
@@ -831,6 +830,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
VIR_FREE(hostname);
else
virURIFree(uri);
+ virObjectUnref(args);
virDomainObjEndAPI(&vm);
virObjectUnref(cfg);
return ret;
--
2.17.1
10:28 a.m.
New subject: [libvirt] [PATCH 3/7] tests: Inline a sysconf call for linuxCPUStatsToBuf
While unlikely, sysconf(_SC_CLK_TCK) could fail leading to
indeterminate results for the subsequent division. So let's
just remove the # define and inline the same change.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tests/virhostcputest.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/tests/virhostcputest.c b/tests/virhostcputest.c
index 8cbc302cae..1df4bb8ee6 100644
--- a/tests/virhostcputest.c
+++ b/tests/virhostcputest.c
@@ -72,7 +72,6 @@ linuxTestCompareFiles(const char *cpuinfofile,
return ret;
}
-# define TICK_TO_NSEC (1000ull * 1000ull * 1000ull / sysconf(_SC_CLK_TCK))
static int
linuxCPUStatsToBuf(virBufferPtr buf,
@@ -81,6 +80,15 @@ linuxCPUStatsToBuf(virBufferPtr buf,
size_t nparams)
{
size_t i = 0;
+ unsigned long long tick_to_nsec;
+ long long sc_clk_tck;
+
+ if ((sc_clk_tck = sysconf(_SC_CLK_TCK)) < 0) {
+ fprintf(stderr, "sysconf(_SC_CLK_TCK) fails : %s\n",
+ strerror(errno));
+ return -1;
+ }
+ tick_to_nsec = (1000ull * 1000ull * 1000ull) / sc_clk_tck;
if (cpu < 0)
virBufferAddLit(buf, "cpu:\n");
@@ -89,7 +97,7 @@ linuxCPUStatsToBuf(virBufferPtr buf,
for (i = 0; i < nparams; i++)
virBufferAsprintf(buf, "%s: %llu\n", param[i].field,
- param[i].value / TICK_TO_NSEC);
+ param[i].value / tick_to_nsec);
virBufferAddChar(buf, '\n');
return 0;
--
2.17.1
10:28 a.m.
New subject: [libvirt] [PATCH 4/7] util: Data overrun may lead to divide by zero
Commit 87a8a30d6 added the function based on the virsh function,
but used an unsigned long long instead of a double and thus that
limits the maximum result.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virutil.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index b5b949ab22..c0783ecb28 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -491,6 +491,10 @@ virFormatIntDecimal(char *buf, size_t buflen, int val)
*
* Similar to vshPrettyCapacity, but operates on integers and not doubles
*
+ * NB: Since using unsigned long long, we are limited to at most a "PiB"
+ * to make pretty. This is because a PiB is 1152921504606846976 bytes,
+ * but that value * 1024 > ULLONG_MAX value 18446744073709551615 bytes.
+ *
* Returns shortened value that can be used with @unit.
*/
unsigned long long
@@ -524,12 +528,7 @@ virFormatIntPretty(unsigned long long val,
return val / (limit / 1024);
}
limit *= 1024;
- if (val % limit) {
- *unit = "PiB";
- return val / (limit / 1024);
- }
- limit *= 1024;
- *unit = "EiB";
+ *unit = "PiB";
return val / (limit / 1024);
}
--
2.17.1
10:28 a.m.
New subject: [libvirt] [PATCH 5/7] tests: Alter logic in testCompareXMLToDomConfig
Rather than initialize actualconfig and expectconfig before
having the possibility that libxlDriverConfigNew could fail
and thus land in cleanup, let's just move them and return
immediately upon failure.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tests/libxlxml2domconfigtest.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/tests/libxlxml2domconfigtest.c b/tests/libxlxml2domconfigtest.c
index 22f9c2c871..2f0bfd5eaf 100644
--- a/tests/libxlxml2domconfigtest.c
+++ b/tests/libxlxml2domconfigtest.c
@@ -62,14 +62,14 @@ testCompareXMLToDomConfig(const char *xmlfile,
char *tempjson = NULL;
char *expectjson = NULL;
- libxl_domain_config_init(&actualconfig);
- libxl_domain_config_init(&expectconfig);
-
if (!(cfg = libxlDriverConfigNew()))
- goto cleanup;
+ return -1;
cfg->caps = caps;
+ libxl_domain_config_init(&actualconfig);
+ libxl_domain_config_init(&expectconfig);
+
if (!(log = (xentoollog_logger *)xtl_createlogger_stdiostream(stderr, XTL_DEBUG,
0)))
goto cleanup;
@@ -135,8 +135,7 @@ testCompareXMLToDomConfig(const char *xmlfile,
libxl_domain_config_dispose(&actualconfig);
libxl_domain_config_dispose(&expectconfig);
xtl_logger_destroy(log);
- if (cfg)
- cfg->caps = NULL;
+ cfg->caps = NULL;
virObjectUnref(cfg);
return ret;
}
--
2.17.1
10:28 a.m.
New subject: [libvirt] [PATCH 6/7] tests: Use STRNEQ_NULLABLE
It's possible that the @outbuf and/or @errbuf could be NULL
and thus we need to use the right comparison macro.
Found by Coverity
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
tests/commandtest.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/commandtest.c b/tests/commandtest.c
index 8aa1fdc84e..4d9a468db2 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -875,12 +875,12 @@ static int test21(const void *unused ATTRIBUTE_UNUSED)
if (virTestGetVerbose())
printf("STDOUT:%s\nSTDERR:%s\n", NULLSTR(outbuf), NULLSTR(errbuf));
- if (STRNEQ(outbuf, outbufExpected)) {
+ if (STRNEQ_NULLABLE(outbuf, outbufExpected)) {
virTestDifference(stderr, outbufExpected, outbuf);
goto cleanup;
}
- if (STRNEQ(errbuf, errbufExpected)) {
+ if (STRNEQ_NULLABLE(errbuf, errbufExpected)) {
virTestDifference(stderr, errbufExpected, errbuf);
goto cleanup;
}
--
2.17.1
10:28 a.m.
New subject: [libvirt] [PATCH 7/7] nwfilter: Alter virNWFilterSnoopReqLeaseDel logic
Move the fetch of @ipAddrLeft to after the goto skip_instantiate
and remove the (req->binding) guard since we know that as long
as req->binding is created, then req->threadkey is filled in.
Found by Coverity
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/nwfilter/nwfilter_dhcpsnoop.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c
index 6d114557c7..e40f649ed5 100644
--- a/src/nwfilter/nwfilter_dhcpsnoop.c
+++ b/src/nwfilter/nwfilter_dhcpsnoop.c
@@ -846,7 +846,6 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req,
int ret = 0;
virNWFilterSnoopIPLeasePtr ipl;
char *ipstr = NULL;
- int ipAddrLeft = 0;
/* protect req->start, req->ifname and the lease */
virNWFilterSnoopReqLock(req);
@@ -867,13 +866,13 @@ virNWFilterSnoopReqLeaseDel(virNWFilterSnoopReqPtr req,
if (update_leasefile)
virNWFilterSnoopLeaseFileSave(ipl);
- if (req->binding)
- ipAddrLeft = virNWFilterIPAddrMapDelIPAddr(req->binding->portdevname,
ipstr);
-
if (!req->threadkey || !instantiate)
goto skip_instantiate;
- if (ipAddrLeft) {
+ /* Assumes that req->binding is valid since req->threadkey
+ * is only generated after req->binding is filled in during
+ * virNWFilterDHCPSnoopReq processing */
+ if ((virNWFilterIPAddrMapDelIPAddr(req->binding->portdevname, ipstr)) > 0)
{
ret = virNWFilterInstantiateFilterLate(req->driver,
req->binding,
req->ifindex);
--
2.17.1
4:31 a.m.
On 09/28/2018 05:28 PM, John Ferlan wrote:
I'm sure it'll be felt one or two could just be false
positives,
but I have 35-40 of true false positives and it seems at least
these go above just noise on the channel.
Perhaps the most difficult one to immediately see was the libxl
refcnt patch. That involves a little bit of theory and has been
in my noise pile for a while until I noted that the @args is
being added in a loop to a callback function that just Unref's
it when done. So if there was more than 1 IP Address, then all
sorts of fun things could happen. Without any change, the Alloc
is matched by the Unref, but with the change we add a Ref to
match each Unref in the I/O loop and we just ensure the Unref
is done for the path that put @args into the I/O callback.
I also think the nwfilter patch was "interesting" insomuch as
it has my "favorite" 'if (int-value) {' condition. IOW, if
not zero, then do something. What became "interesting" is that
the virNWFilterIPAddrMapDelIPAddr could return -1 if the
virHashLookup on @req->binding->portdevname returned NULL,
so when "shrinking" the code to only call the instantiation
for/when there was an IP Address found resolves a couple of
issues in the code.
John Ferlan (7):
lxc: Only check @nparams in lxcDomainBlockStatsFlags
libxl: Fix possible object refcnt issue
tests: Inline a sysconf call for linuxCPUStatsToBuf
util: Data overrun may lead to divide by zero
tests: Alter logic in testCompareXMLToDomConfig
tests: Use STRNEQ_NULLABLE
nwfilter: Alter virNWFilterSnoopReqLeaseDel logic
src/libxl/libxl_migration.c | 4 ++--
src/lxc/lxc_driver.c | 2 +-
src/nwfilter/nwfilter_dhcpsnoop.c | 9 ++++-----
src/util/virutil.c | 11 +++++------
tests/commandtest.c | 4 ++--
tests/libxlxml2domconfigtest.c | 11 +++++------
tests/virhostcputest.c | 12 ++++++++++--
7 files changed, 29 insertions(+), 24 deletions(-)
ACK
Michal
2244
days inactive
2247
days old
8 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik