[libvirt] [PATCH 0/7] Various Coverity based concerns
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
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@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
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@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
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@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
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@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
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@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
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@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
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@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
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
participants (2)
-
John Ferlan -
Michal Privoznik