[libvirt] [PATCH 0/7] Address some Coverity found issues
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
Convert virPCIFile to return the buffer allocated (or not) and make the appropriate check in the caller. Signed-off-by: John Ferlan <jferlan@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
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
Convert virPCIDriverFile to return the buffer allocated (or not) and make the appropriate check in the caller. Signed-off-by: John Ferlan <jferlan@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
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@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
Convert virPCIDriverDir to return the buffer allocated (or not) and make the appropriate check in the caller. Signed-off-by: John Ferlan <jferlan@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
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@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
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@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
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@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
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@redhat.com> --- src/phyp/phyp_driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
ACK, safe for freeze. Peter
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@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
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@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
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
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
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
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
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@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
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@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
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
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
participants (3)
-
John Ferlan -
Michal Privoznik -
Peter Krempa