4:03 p.m.
The following Coverity issues are flagged in Coverity 7.6.1 compared
to not being seen in 7.5.1 - this series just addresses those issues
before 7.6.1 is installed in our upstream Jenkins environment:
http://jenkins-virt.usersys.redhat.com/job/libvirt-coverity/
It seems the bulk are essentially false positives, but we can do something
in order to work around them.
For the first 3 patches, Coverity complains that virVasprintfInternal
can set "*strp = NULL;" on return (in the error path), but the callers
don't associate the error path (< 0) with the 'buffer' and thus believe
there could be some path where a return 0 would occur. This would result
in the callers of the virPCIFile, virPCIDriverFile, and virPCIDriverDir
having an error. Rather than try to add sa_assert()'s for each, adjust
the calls and callers to return the buffer
For the fourth patch, the strtok_r call expects that the first call will
have a non-NULL value for the first parameter and subsequent calls over
the same string would pass a NULL value in the first parameter and a
non-NULL parameter in the third parameter to be used as the "point" in
the string to continue looking for the token. However, Coverity doesn't
realize this and thus flags the strtok_r call with a possible forward
NULL dereference because we initialize the third parameter to NULL.
By adding an 'sa_assert' for what is the first argument lets Coverity
know we know what we're doing. Additionally, VIR_STRDUP could return a
NULL string and a zero return, the assertion in openvz_conf.c is on
the passed value to be scanned.
The fifth patch is perhaps the only one fixing a real issue; however,
it's been around since 1.2.11, so perhaps it's a less traveled path.
The sixth patch seems to be a false positive, but since Coverity doesn't
make the connection between nstack and stack, it's just as safe to
add the protection in the Free routine against a strange path being hit.
For the last patch, this is a false positive, but it also shows up in
the 7.5.1 coverity build:
http://jenkins-virt.usersys.redhat.com/job/libvirt-coverity/128/
Essentially the issue is that virResizeN can "return 0;" prior to
the virExpandN and that causes Coverity some angst since it would
then be possible to not have anything returned in "*ret" which would
cause a NULL dereference. Stepping through the logic has no path
in which that could happen. In order to handle this, rather than
use VIR_RESIZE_N for 'n' times through the loop, let's first count
the matches, use VIR_ALLOC_N, and then rerun the same loop filling
in each address as before. The difference being 'n' VIR_RESIZE_N
versus 2*'n' STREQ calls - perhaps a small wash performance wise.
John Ferlan (7):
util: Resolve Coverity FORWARD_NULL
util: Resolve Coverity FORWARD_NULL
util: Resolve Coverity FORWARD_NULL
Avoid Coverity FORWARD_NULL prior to strtok_r calls
phyp: Resolve Coverity FORWARD_NULL
util: Resolve Coverity FORWARD_NULL
util: Avoid Coverity FORWARD_NULL
src/esx/esx_vi.c | 1 +
src/libxl/libxl_conf.c | 1 +
src/openvz/openvz_conf.c | 2 ++
src/phyp/phyp_driver.c | 3 +--
src/util/virdbus.c | 4 +++
src/util/virpci.c | 67 ++++++++++++++++++++++++-----------------------
src/util/virtypedparam.c | 14 ++++++----
src/xenapi/xenapi_utils.c | 1 +
8 files changed, 53 insertions(+), 40 deletions(-)
--
2.1.0
4:03 p.m.
New subject: [libvirt] [PATCH 1/7] util: Resolve Coverity FORWARD_NULL
Convert virPCIFile to return the buffer allocated (or not) and make the
appropriate check in the caller.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virpci.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index cf2a253..ad08570 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -226,14 +226,13 @@ virPCIDriverFile(char **buffer, const char *driver, const char
*file)
}
-static int
-virPCIFile(char **buffer, const char *device, const char *file)
+static char *
+virPCIFile(const char *device, const char *file)
{
- VIR_FREE(*buffer);
+ char *buffer = NULL;
- if (virAsprintf(buffer, PCI_SYSFS "devices/%s/%s", device, file) < 0)
- return -1;
- return 0;
+ ignore_value(virAsprintf(&buffer, PCI_SYSFS "devices/%s/%s", device,
file));
+ return buffer;
}
@@ -252,7 +251,7 @@ virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path,
char **name)
*path = *name = NULL;
/* drvlink = "/sys/bus/pci/dddd:bb:ss.ff/driver" */
- if (virPCIFile(&drvlink, dev->name, "driver") < 0)
+ if (!(drvlink = virPCIFile(dev->name, "driver")))
goto cleanup;
if (!virFileExists(drvlink)) {
@@ -375,7 +374,7 @@ virPCIDeviceReadClass(virPCIDevicePtr dev, uint16_t *device_class)
int ret = -1;
unsigned int value;
- if (virPCIFile(&path, dev->name, "class") < 0)
+ if (!(path = virPCIFile(dev->name, "class")))
return ret;
/* class string is '0xNNNNNN\n' ... i.e. 9 bytes */
@@ -1055,7 +1054,7 @@ virPCIDeviceUnbind(virPCIDevicePtr dev, bool reprobe)
goto cleanup;
}
- if (virPCIFile(&path, dev->name, "driver/unbind") < 0)
+ if (!(path = virPCIFile(dev->name, "driver/unbind")))
goto cleanup;
if (virFileExists(path)) {
@@ -1190,7 +1189,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev,
virErrorPtr err = NULL;
if (virPCIDriverDir(&stubDriverPath, stubDriverName) < 0 ||
- virPCIFile(&driverLink, dev->name, "driver") < 0 ||
+ !(driverLink = virPCIFile(dev->name, "driver")) ||
VIR_STRDUP(newDriverName, stubDriverName) < 0)
goto cleanup;
@@ -1501,7 +1500,7 @@ virPCIDeviceReadID(virPCIDevicePtr dev, const char *id_name)
char *path = NULL;
char *id_str;
- if (virPCIFile(&path, dev->name, id_name) < 0)
+ if (!(path = virPCIFile(dev->name, id_name)))
return NULL;
/* ID string is '0xNNNN\n' ... i.e. 7 bytes */
@@ -2167,7 +2166,7 @@ virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr)
addr->bus, addr->slot, addr->function) < 0)
goto cleanup;
- if (virPCIFile(&devPath, devName, "iommu_group") < 0)
+ if (!(devPath = virPCIFile(devName, "iommu_group")))
goto cleanup;
if (virFileIsLink(devPath) != 1) {
ret = -2;
@@ -2209,7 +2208,7 @@ virPCIDeviceGetIOMMUGroupDev(virPCIDevicePtr dev)
char *groupPath = NULL;
char *groupDev = NULL;
- if (virPCIFile(&devPath, dev->name, "iommu_group") < 0)
+ if (!(devPath = virPCIFile(dev->name, "iommu_group")))
goto cleanup;
if (virFileIsLink(devPath) != 1) {
virReportError(VIR_ERR_INTERNAL_ERROR,
--
2.1.0
4:44 p.m.
New subject: [libvirt] [PATCH 1/7] util: Resolve Coverity FORWARD_NULL
On Wed, Jul 01, 2015 at 10:03:45 -0400, John Ferlan wrote:
Convert virPCIFile to return the buffer allocated (or not) and make
the
appropriate check in the caller.
ACK, safe for freeze. I like the return-the-buffer semantic more.
Peter
4:03 p.m.
New subject: [libvirt] [PATCH 2/7] util: Resolve Coverity FORWARD_NULL
Convert virPCIDriverFile to return the buffer allocated (or not) and make the
appropriate check in the caller.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virpci.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index ad08570..5d86c89 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -215,14 +215,13 @@ virPCIDriverDir(char **buffer, const char *driver)
}
-static int
-virPCIDriverFile(char **buffer, const char *driver, const char *file)
+static char *
+virPCIDriverFile(const char *driver, const char *file)
{
- VIR_FREE(*buffer);
+ char *buffer = NULL;
- if (virAsprintf(buffer, PCI_SYSFS "drivers/%s/%s", driver, file) < 0)
- return -1;
- return 0;
+ ignore_value(virAsprintf(&buffer, PCI_SYSFS "drivers/%s/%s", driver,
file));
+ return buffer;
}
@@ -1126,7 +1125,7 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
goto reprobe;
/* Xen's pciback.ko wants you to use remove_slot on the specific device */
- if (virPCIDriverFile(&path, driver, "remove_slot") < 0)
+ if (!(path = virPCIDriverFile(driver, "remove_slot")))
goto cleanup;
if (virFileExists(path) && virFileWriteStr(path, dev->name, 0) < 0) {
@@ -1148,7 +1147,8 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev)
* available, then re-probing would just cause the device to be
* re-bound to the stub.
*/
- if (driver && virPCIDriverFile(&path, driver, "remove_id") <
0)
+ VIR_FREE(path);
+ if (driver && !(path = virPCIDriverFile(driver, "remove_id")))
goto cleanup;
if (!driver || !virFileExists(drvdir) || virFileExists(path)) {
@@ -1212,7 +1212,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev,
* is triggered for such a device, it will also be immediately
* bound by the stub.
*/
- if (virPCIDriverFile(&path, stubDriverName, "new_id") < 0)
+ if (!(path = virPCIDriverFile(stubDriverName, "new_id")))
goto cleanup;
if (virFileWriteStr(path, dev->id, 0) < 0) {
@@ -1239,7 +1239,8 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev,
*/
if (!virFileLinkPointsTo(driverLink, stubDriverPath)) {
/* Xen's pciback.ko wants you to use new_slot first */
- if (virPCIDriverFile(&path, stubDriverName, "new_slot") < 0)
+ VIR_FREE(path);
+ if (!(path = virPCIDriverFile(stubDriverName, "new_slot")))
goto remove_id;
if (virFileExists(path) && virFileWriteStr(path, dev->name, 0) < 0)
{
@@ -1251,7 +1252,8 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev,
}
dev->remove_slot = true;
- if (virPCIDriverFile(&path, stubDriverName, "bind") < 0)
+ VIR_FREE(path);
+ if (!(path = virPCIDriverFile(stubDriverName, "bind")))
goto remove_id;
if (virFileWriteStr(path, dev->name, 0) < 0) {
@@ -1271,7 +1273,8 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev,
/* If 'remove_id' exists, remove the device id from pci-stub's dynamic
* ID table so that 'drivers_probe' works below.
*/
- if (virPCIDriverFile(&path, stubDriverName, "remove_id") < 0) {
+ VIR_FREE(path);
+ if (!(path = virPCIDriverFile(stubDriverName, "remove_id"))) {
/* We do not remove PCI ID from pci-stub, and we cannot reprobe it */
if (dev->reprobe) {
VIR_WARN("Could not remove PCI ID '%s' from %s, and the device
"
--
2.1.0
4:50 p.m.
New subject: [libvirt] [PATCH 2/7] util: Resolve Coverity FORWARD_NULL
On Wed, Jul 01, 2015 at 10:03:46 -0400, John Ferlan wrote:
Convert virPCIDriverFile to return the buffer allocated (or not) and
make the
appropriate check in the caller.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virpci.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index ad08570..5d86c89 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -215,14 +215,13 @@ virPCIDriverDir(char **buffer, const char *driver)
}
-static int
-virPCIDriverFile(char **buffer, const char *driver, const char *file)
+static char *
+virPCIDriverFile(const char *driver, const char *file)
{
- VIR_FREE(*buffer);
+ char *buffer = NULL;
Shouldn't be necessary to init this, sice virAsprintf sets it to NULL on
failure.
- if (virAsprintf(buffer, PCI_SYSFS "drivers/%s/%s", driver, file) < 0)
- return -1;
- return 0;
+ ignore_value(virAsprintf(&buffer, PCI_SYSFS "drivers/%s/%s", driver,
file));
+ return buffer;
}
ACK,
Peter
4:03 p.m.
New subject: [libvirt] [PATCH 3/7] util: Resolve Coverity FORWARD_NULL
Convert virPCIDriverDir to return the buffer allocated (or not) and make the
appropriate check in the caller.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virpci.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 5d86c89..5dabb61 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -204,14 +204,13 @@ static int virPCIOnceInit(void)
VIR_ONCE_GLOBAL_INIT(virPCI)
-static int
-virPCIDriverDir(char **buffer, const char *driver)
+static char *
+virPCIDriverDir(const char *driver)
{
- VIR_FREE(*buffer);
+ char *buffer = NULL;
- if (virAsprintf(buffer, PCI_SYSFS "drivers/%s", driver) < 0)
- return -1;
- return 0;
+ ignore_value(virAsprintf(&buffer, PCI_SYSFS "drivers/%s", driver));
+ return buffer;
}
@@ -998,7 +997,7 @@ virPCIProbeStubDriver(const char *driver)
bool probed = false;
recheck:
- if (virPCIDriverDir(&drvpath, driver) == 0 && virFileExists(drvpath)) {
+ if ((drvpath = virPCIDriverDir(driver)) && virFileExists(drvpath)) {
/* driver already loaded, return */
VIR_FREE(drvpath);
return 0;
@@ -1188,7 +1187,7 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev,
char *newDriverName = NULL;
virErrorPtr err = NULL;
- if (virPCIDriverDir(&stubDriverPath, stubDriverName) < 0 ||
+ if (!(stubDriverPath = virPCIDriverDir(stubDriverName)) ||
!(driverLink = virPCIFile(dev->name, "driver")) ||
VIR_STRDUP(newDriverName, stubDriverName) < 0)
goto cleanup;
--
2.1.0
5:11 p.m.
New subject: [libvirt] [PATCH 3/7] util: Resolve Coverity FORWARD_NULL
On Wed, Jul 01, 2015 at 10:03:47 -0400, John Ferlan wrote:
Convert virPCIDriverDir to return the buffer allocated (or not) and
make the
appropriate check in the caller.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virpci.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 5d86c89..5dabb61 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
...
@@ -998,7 +997,7 @@ virPCIProbeStubDriver(const char *driver)
bool probed = false;
recheck:
- if (virPCIDriverDir(&drvpath, driver) == 0 && virFileExists(drvpath)) {
+ if ((drvpath = virPCIDriverDir(driver)) && virFileExists(drvpath)) {
Extra space before &&.
/* driver already loaded, return */
VIR_FREE(drvpath);
return 0;
ACK with above comment fixed.
Peter
4:03 p.m.
New subject: [libvirt] [PATCH 4/7] Avoid Coverity FORWARD_NULL prior to strtok_r calls
The 'strtok_r' function requires passing a NULL as the first parameter
on subsequent calls in order to ensure the code picks up where it left
off on a previous call. However, Coverity doesn't quite realize this
and points out that if a NULL was passed in as the third argument it would
result in a possible NULL deref because the strtok_r function will assign
the third argument to the first in the call is NULL.
Adding an sa_assert() prior to each initial call quiets Coverity
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/esx/esx_vi.c | 1 +
src/libxl/libxl_conf.c | 1 +
src/openvz/openvz_conf.c | 2 ++
src/xenapi/xenapi_utils.c | 1 +
4 files changed, 5 insertions(+)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c
index af822b1..a57d753 100644
--- a/src/esx/esx_vi.c
+++ b/src/esx/esx_vi.c
@@ -1173,6 +1173,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const
char *path)
goto cleanup;
/* Lookup Datacenter */
+ sa_assert(tmp);
item = strtok_r(tmp, "/", &saveptr);
if (!item) {
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index e845759..7223ba2 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -353,6 +353,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps)
/* Split capabilities string into tokens. strtok_r is OK here because
* we "own" the buffer. Parse out the features from each token.
*/
+ sa_assert(ver_info->capabilities);
for (str = ver_info->capabilities, nr_guest_archs = 0;
nr_guest_archs < sizeof(guest_archs) / sizeof(guest_archs[0])
&& (token = strtok_r(str, " ", &saveptr)) !=
NULL;
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c
index 49d78c6..18cf6bd 100644
--- a/src/openvz/openvz_conf.c
+++ b/src/openvz/openvz_conf.c
@@ -138,6 +138,7 @@ openvzParseBarrierLimit(const char* value,
char *str;
int ret = -1;
+ sa_assert(value);
if (VIR_STRDUP(str, value) < 0)
goto error;
@@ -965,6 +966,7 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len)
}
}
+ sa_assert(line);
iden = strtok_r(line, " ", &saveptr);
uuidbuf = strtok_r(NULL, "\n", &saveptr);
diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c
index a80e084..6b710cd 100644
--- a/src/xenapi/xenapi_utils.c
+++ b/src/xenapi/xenapi_utils.c
@@ -301,6 +301,7 @@ getCpuBitMapfromString(char *mask, unsigned char *cpumap, int maplen)
int max_bits = maplen * 8;
char *num = NULL, *bp = NULL;
bzero(cpumap, maplen);
+ sa_assert(mask);
num = strtok_r(mask, ",", &bp);
while (num != NULL) {
if (virStrToLong_i(num, NULL, 10, &pos) < 0)
--
2.1.0
4:03 p.m.
New subject: [libvirt] [PATCH 5/7] phyp: Resolve Coverity FORWARD_NULL
Commit id 'cd490086' added a VIR_FORCE_CLOSE of the 'sock', but it
was after the VIR_FREE() of phyp_driver, resulting in a possible/likely
NULL dereference.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/phyp/phyp_driver.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index c558c48..ec0fde3 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -1249,9 +1249,8 @@ phypConnectClose(virConnectPtr conn)
virObjectUnref(phyp_driver->xmlopt);
phypUUIDTable_Free(phyp_driver->uuid_table);
VIR_FREE(phyp_driver->managed_system);
- VIR_FREE(phyp_driver);
-
VIR_FORCE_CLOSE(phyp_driver->sock);
+ VIR_FREE(phyp_driver);
return 0;
}
--
2.1.0
4:24 p.m.
New subject: [libvirt] [PATCH 5/7] phyp: Resolve Coverity FORWARD_NULL
On Wed, Jul 01, 2015 at 10:03:49 -0400, John Ferlan wrote:
Commit id 'cd490086' added a VIR_FORCE_CLOSE of the
'sock', but it
was after the VIR_FREE() of phyp_driver, resulting in a possible/likely
NULL dereference.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/phyp/phyp_driver.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
ACK, safe for freeze.
Peter
4:03 p.m.
New subject: [libvirt] [PATCH 6/7] util: Resolve Coverity FORWARD_NULL
Coverity "believes" that virDBusMessageIterDecode could find "nstack =
0"
in the first pass through the "for (;;)", thus break'ing out of the loop
prior to any virDBusTypeStackPush being called thus having 'stack == NULL'.
Rather than check for (!stack) prior to either the Encode or Decode path,
putting a (!*stack) in the StackFree ensures we don't have some sort of
NULL deref
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virdbus.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/util/virdbus.c b/src/util/virdbus.c
index 1cf1eef..78fb795 100644
--- a/src/util/virdbus.c
+++ b/src/util/virdbus.c
@@ -544,6 +544,10 @@ static void virDBusTypeStackFree(virDBusTypeStack **stack,
size_t *nstack)
{
size_t i;
+
+ if (!*stack)
+ return;
+
/* The iter in the first level of the stack is the
* root iter which must not be freed
*/
--
2.1.0
4:03 p.m.
New subject: [libvirt] [PATCH 7/7] util: Avoid Coverity FORWARD_NULL
Avoid a false positive since Coverity find a path in virResizeN which
could return 0 prior to the allocation of memory and thus flags a
possible NULL dereference. Instead use multiple loops to first count
the number of matches, perform one allocation, and then again search
for matches. It's a 'n' string comparisons and allocations versus
2*'n' string comparisons and one allocation.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virtypedparam.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c
index 106403c..79c7cab 100644
--- a/src/util/virtypedparam.c
+++ b/src/util/virtypedparam.c
@@ -501,7 +501,7 @@ virTypedParamsFilter(virTypedParameterPtr params,
const char *name,
virTypedParameterPtr **ret)
{
- size_t i, alloc = 0, n = 0;
+ size_t i, n = 0;
virCheckNonNullArgGoto(params, error);
virCheckNonNullArgGoto(name, error);
@@ -510,12 +510,16 @@ virTypedParamsFilter(virTypedParameterPtr params,
*ret = NULL;
for (i = 0; i < nparams; i++) {
- if (STREQ(params[i].field, name)) {
- if (VIR_RESIZE_N(*ret, alloc, n, 1) < 0)
- goto error;
+ if (STREQ(params[i].field, name))
+ n++;
+ }
- (*ret)[n] = ¶ms[i];
+ if (VIR_ALLOC_N(*ret, n) < 0)
+ goto error;
+ for (n = 0, i = 0; i < nparams; i++) {
+ if (STREQ(params[i].field, name)) {
+ (*ret)[n] = ¶ms[i];
n++;
}
}
--
2.1.0
4:30 p.m.
New subject: [libvirt] [PATCH 7/7] util: Avoid Coverity FORWARD_NULL
On Wed, Jul 01, 2015 at 10:03:51 -0400, John Ferlan wrote:
Avoid a false positive since Coverity find a path in virResizeN
which
could return 0 prior to the allocation of memory and thus flags a
possible NULL dereference. Instead use multiple loops to first count
the number of matches, perform one allocation, and then again search
for matches. It's a 'n' string comparisons and allocations versus
2*'n' string comparisons and one allocation.
Another option would be to alloc *ret to nparams and fill only the
matched ones leaving a part of the array unused. Since this will be
used mainly in cases where n will be a rather small number (I think
libvirt is able to support ~250 disks max) the memory overhead won't be
noticable.
Peter
4:20 p.m.
On Wed, Jul 01, 2015 at 10:03:44 -0400, John Ferlan wrote:
The following Coverity issues are flagged in Coverity 7.6.1 compared
to not being seen in 7.5.1 - this series just addresses those issues
before 7.6.1 is installed in our upstream Jenkins environment:
http://jenkins-virt.usersys.redhat.com/job/libvirt-coverity/
This is a Red Hat internal link where other users don't have access to
this definitely should not be published to a mailing list at least to
avoid frustration to non-redhatters.
It seems the bulk are essentially false positives, but we can do something
in order to work around them.
So why are we doing anything about that? Coverity is the software that
should be fixed here.
While coverity is a very useful tool in some cases where it reports
actual errors the noise it generates is sometimes quite unbearable and
as it looks the new version added just a few false positives, but no
actual fault.
Additionally if we continue to patch up the mistakes rather than
reporting it we might as well as end up by flagging something with the
sa_assert() macro that will change into an actual error in later
patches and then we won't be able to detect that.
As of such I think that libvirt should mostly fix just actual errors
found by coverity and people who run coverity on the libvirt code base
should rather report the errorrs to the coverity vendor to fix the false
positive notifications rather than working that around by silencing it.
</rant>
Peter
6:04 p.m.
On 07/01/2015 10:20 AM, Peter Krempa wrote:
On Wed, Jul 01, 2015 at 10:03:44 -0400, John Ferlan wrote:
> The following Coverity issues are flagged in Coverity 7.6.1 compared
> to not being seen in 7.5.1 - this series just addresses those issues
> before 7.6.1 is installed in our upstream Jenkins environment:
>
> http://jenkins-virt.usersys.redhat.com/job/libvirt-coverity/
This is a Red Hat internal link where other users don't have access to
this definitely should not be published to a mailing list at least to
avoid frustration to non-redhatters.
hmm.. for some reason I thought it was an external reference - I know
we've been running many tests thru Jenkins on an external CentOS server,
so my brain said this was too... But now you've reminded me that the
Coverity ones had to be internal because of the license... In any case -
it wasn't intentional... Sorry about that.
>
> It seems the bulk are essentially false positives, but we can do something
> in order to work around them.
So why are we doing anything about that? Coverity is the software that
should be fixed here.
While coverity is a very useful tool in some cases where it reports
actual errors the noise it generates is sometimes quite unbearable and
as it looks the new version added just a few false positives, but no
actual fault.
Additionally if we continue to patch up the mistakes rather than
reporting it we might as well as end up by flagging something with the
sa_assert() macro that will change into an actual error in later
patches and then we won't be able to detect that.
As of such I think that libvirt should mostly fix just actual errors
found by coverity and people who run coverity on the libvirt code base
should rather report the errorrs to the coverity vendor to fix the false
positive notifications rather than working that around by silencing it.
</rant>
Peter
Certainly understand your frustration with Coverity's false positive;
however, remember perhaps one person's "bug" is someone else's
"feature". The more complex the code and the deeper the calling stack,
the more difficult it is for artificial intelligence tools to model
every possible combination. There's a point in which even they have to
let the human intelligence (sic) take over.
A particular pain point are API's where a status or count is returned as
well as returning a pointer to something. We know from reading the man
pages or reading the code that the only way that pointer can be filled
in is if the status/count is a particular value (usually non negative -
so zero or more). However, that's more difficult for modeling code
which tries "all" paths when it finds a condition and then flags when
there's a "chance" for something to go wrong. For example, code such as
the following causes these types of issues:
char *retparam = NULL;
if ((rc = function(param, param, &retparam)) < 0)
goto error;
if (retparam->field)
Coverity will scan the code and "flag" that retparam could be a problem
here. It will further dive into 'function' and determine code paths from
there that could perhaps return >= 0 such that retparam wouldn't be
filled in. If 'function' is a macro for some other function which, then
calls yet another function, which is a macro for something else (ad
nauseum), it becomes more difficult to model, thus Coverity "flags" it
as a potential problem for a human to resolve/check. False positives are
a way of life - they get worse for stricter checking - we only do the
'basic' checks.
In the case of the 'virpci' changes (patches 1-3) - the "eventual" call
to virVasprintfInternal() can fail returning -1 and setting '*strp =
NULL;" - perfectly reasonable... The caller virAsprintfInternal doesn't
check ret, but passes it back to it's caller via macro in virstring.h:
# define virAsprintf(strp, ...) \
virAsprintfInternal(
Eventually virPCIFile (for example) returns -1 or 0 based on that
return, but it seems being able to correlate that return back in any of
the callers cannot happen. Coverity wants to mock both the true and
false path of the condition, thus the error. Whether it's macros or
variable call stacks that confuses things, I'm not quite sure. But, I'll
assume there was some corner case for someone else's code that caused
the modeling code to not flag it when someone felt it should. Who really
knows, I don't follow Coverity development that closely.
Sometimes, issues are even more opaque...
Consider the case from openvz.conf.c changes:
if (VIR_STRDUP(str, value) < 0)
goto error;
iden = strtok_r(line, " ", &saveptr);
Seems perfectly fine, right? However, if one digs into
VIR_STRDUP()/virStrdup() they find this:
*dest = NULL;
if (!src)
return 0;
So if by chance 'value' was NULL (we don't prevent or check it), then
str will be NULL and that's perfectly reasonable. The caller is
expected to check it, right? But in this case in order to get into the
function we have 'assumed' the 'value' is non-NULL based on the positive
return:
ret = openvzReadVPSConfigParam(veid, param, &temp);
if (ret > 0) {
if (openvzParseBarrierLimit(temp, &barrier, &limit)) {
So now Coverity cannot model the called functions, rather it has to
model the calling functions and "know" that ret > 0 means temp != NULL.
Since we can do this ourselves by reading the header of the ConfigParam
function, but the artificial intelligence just doesn't see that since it
doesn't know to correlate "ret" with "temp". Thus we just have to
teach
it and move on.
John
6:27 p.m.
On 07/01/2015 10:03 AM, John Ferlan wrote:
...
John Ferlan (7):
util: Resolve Coverity FORWARD_NULL
util: Resolve Coverity FORWARD_NULL
util: Resolve Coverity FORWARD_NULL
Avoid Coverity FORWARD_NULL prior to strtok_r calls
phyp: Resolve Coverity FORWARD_NULL
util: Resolve Coverity FORWARD_NULL
util: Avoid Coverity FORWARD_NULL
src/esx/esx_vi.c | 1 +
src/libxl/libxl_conf.c | 1 +
src/openvz/openvz_conf.c | 2 ++
src/phyp/phyp_driver.c | 3 +--
src/util/virdbus.c | 4 +++
src/util/virpci.c | 67 ++++++++++++++++++++++++-----------------------
src/util/virtypedparam.c | 14 ++++++----
src/xenapi/xenapi_utils.c | 1 +
8 files changed, 53 insertions(+), 40 deletions(-)
I have adjusted patches 1-3 to the point raised in the review of 2/7 in
order to just have "char *buffer;" and not "char *buffer = NULL;"
(overly cautious)
I have pushed patches 1-3 and 5 (phyp_driver) leaving 4, 6, and 7 for a
followup.
Tks -
John
6:41 p.m.
New subject: [libvirt] [PATCHv2 7/7 util: Avoid Coverity FORWARD_NULL
Avoid a false positive since Coverity find a path in virResizeN which
could return 0 prior to the allocation of memory and thus flags a
possible NULL dereference. Instead allocate the output buffer based
on 'nparams' and only fill it partially if need be - shouldn't be too
much a waste of space. Quicker than multiple VIR_RESIZE_N calls or
two loops of STREQ's sandwiched around a single VIR_ALLOC_N using
'n' matches from a first loop to generate the 'n' addresses to return
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
Difference to v1: Use single VIR_ALLOC_N of nparams and one loop as
suggested in review
src/util/virtypedparam.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c
index 106403c..f3ce157 100644
--- a/src/util/virtypedparam.c
+++ b/src/util/virtypedparam.c
@@ -501,21 +501,18 @@ virTypedParamsFilter(virTypedParameterPtr params,
const char *name,
virTypedParameterPtr **ret)
{
- size_t i, alloc = 0, n = 0;
+ size_t i, n = 0;
virCheckNonNullArgGoto(params, error);
virCheckNonNullArgGoto(name, error);
virCheckNonNullArgGoto(ret, error);
- *ret = NULL;
+ if (VIR_ALLOC_N(*ret, nparams) < 0)
+ goto error;
for (i = 0; i < nparams; i++) {
if (STREQ(params[i].field, name)) {
- if (VIR_RESIZE_N(*ret, alloc, n, 1) < 0)
- goto error;
-
(*ret)[n] = ¶ms[i];
-
n++;
}
}
--
2.1.0
7:34 a.m.
New subject: [libvirt] [PATCHv2 7/7 util: Avoid Coverity FORWARD_NULL
On Wed, Jul 01, 2015 at 12:41:23 -0400, John Ferlan wrote:
Avoid a false positive since Coverity find a path in virResizeN
which
could return 0 prior to the allocation of memory and thus flags a
possible NULL dereference. Instead allocate the output buffer based
on 'nparams' and only fill it partially if need be - shouldn't be too
much a waste of space. Quicker than multiple VIR_RESIZE_N calls or
two loops of STREQ's sandwiched around a single VIR_ALLOC_N using
'n' matches from a first loop to generate the 'n' addresses to return
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
Difference to v1: Use single VIR_ALLOC_N of nparams and one loop as
suggested in review
src/util/virtypedparam.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
ACK,
Peter
11:30 a.m.
On 01.07.2015 16:03, John Ferlan wrote:
I'm picking a random Coverity thread to express my latest thoughts on
this. I'm not trying to say the problems I'm raising are necessarily
introduced here in this patch set.
We've seen quite a lot of false positives lately. I'm not familiar with
coverity, but does it have a suppress file, something like valgrind has?
In valrgind, one can enumerate stacktraces that would be reported, but
has been investigated by a developer and therefore are known to be okay.
If coverity would have something like that I think should utilize it.
We've poisoned our code with sa_assert()-s, dummy coverity comments,
etc. If we can keep our code clean, that'd be nice. I guess I'm okay
with having the coverity suppress file in the repo - we have the
valgrind one already.
Oh, and for false positives maybe we should file a bug report against
coverity (if possible).
Michal
3:36 p.m.
On 07/02/2015 05:30 AM, Michal Privoznik wrote:
On 01.07.2015 16:03, John Ferlan wrote:
>
I'm picking a random Coverity thread to express my latest thoughts on
this. I'm not trying to say the problems I'm raising are necessarily
introduced here in this patch set.
We've seen quite a lot of false positives lately. I'm not familiar with
coverity, but does it have a suppress file, something like valgrind has?
In valrgind, one can enumerate stacktraces that would be reported, but
has been investigated by a developer and therefore are known to be okay.
If coverity would have something like that I think should utilize it.
We've poisoned our code with sa_assert()-s, dummy coverity comments,
etc. If we can keep our code clean, that'd be nice. I guess I'm okay
with having the coverity suppress file in the repo - we have the
valgrind one already.
Not sure how a Coverity suppress file works, but if I have some cycles
I'll look into it. However, if it's susceptible to code motion, then it
may not be worth it... The last time I looked (almost 2 years ago) at
the valgrind suppress file it had some stale chunks, but it also had a
decent way to "ignore" certain traces.
In any case, I'll look into removing existing sa_assert()'s... As a
test, I removed the definition and reran a build to find areas that
currently "require" it. I think of the 15 or so "issues", perhaps 1 or
2
could go away with some refactoring.
Oh, and for false positives maybe we should file a bug report against
coverity (if possible).
That's generally the more difficult part especially when it comes to
replicating the build environment (including .gnulib) in a "small
module" as any good developer would ask for when someone claims your
product has a defect. Also as I pointed out before - those aren't
necessarily Coverity defects - it's pointing out valid concerns which
take human intervention to determine whether they are a problem or not.
John
3363
days inactive
3365
days old
19 comments
3 participants
participants (3)
-
John Ferlan -
Michal Privoznik -
Peter Krempa