[libvirt] [PATCH 0/4] Remove need for STATIC_ANALYSIS in viralloc.h
Not sure which release exactly, but the coverity analysis bug I filed that was the reason for commit id 'c9a85af31' has been fixed, so it's time to remove that... In doing so uncovered another issue - it seems the fix for Coverity addressed the primary problem we've seen, but when there's functions returning allocated strings by reference that can also VIR_FREE(*string), whatever was added to address the main issue doesn't seem to recognize this other usage, resulinting in a false positive resource_leak. Luckily there's only two instances in our code for that. One is addressed by patch 3 and the other in virPCIDeviceGetDriverPathAndName which would require quite a few more changes to address (essentially split up the function - it'll be on my "to do" list). Patch 1: Addresses an issue seen once patches 2-4 were applied - that there's a real problem with the error path. It's a simple fix. Patches 2 & 3: Address a false positive resource leak even with patch 4 applied. Patch 4: Remove the rather ugly !STATIC_ANALYSIS within the VIR_FREE and VIR_DISPOSE* macros. As I found with a build after the fact, VIR_DISPOSE_STRING was missing an argument anyway (it should have been ": 0, 1, NULL" instead of ": 1, NULL"). NB: Patch 4 has been run through the private coverity server... John Ferlan (4): util: Fix error path for virPCIGetVirtualFunctions util: Remove need for ret in virPCIGetPhysicalFunction util: Adjust return for virPCIGetDeviceAddressFromSysfsLink util: Remove need for STATIC_ANALYSIS check src/util/viralloc.h | 34 ++++++---------------------------- src/util/virpci.c | 39 +++++++++++++++++++-------------------- 2 files changed, 25 insertions(+), 48 deletions(-) -- 2.5.5
If we get to the error: label and clear out the *virtual_functions[] pointers and then return w/ error to the caller - the caller has it's own cleanup of the same array in the out: label which is keyed off the value of num_virt_fns, which wasn't reset to 0 in the called function leading to a possible problem. Just clear the value (found by Coverity) Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virpci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virpci.c b/src/util/virpci.c index 3f1252d..be35017 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2573,6 +2573,7 @@ virPCIGetVirtualFunctions(const char *sysfs_path, for (i = 0; i < *num_virtual_functions; i++) VIR_FREE((*virtual_functions)[i]); VIR_FREE(*virtual_functions); + *num_virtual_functions = 0; goto cleanup; } -- 2.5.5
Since the callers only ever expect 0 or -1, let's just return that directly Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virpci.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index be35017..1119c2e 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2487,21 +2487,20 @@ int virPCIGetPhysicalFunction(const char *vf_sysfs_path, virPCIDeviceAddressPtr *pf) { - int ret = -1; char *device_link = NULL; if (virBuildPath(&device_link, vf_sysfs_path, "physfn") == -1) { virReportOOMError(); - return ret; + return -1; } - if ((ret = virPCIGetDeviceAddressFromSysfsLink(device_link, pf)) >= 0) { + if (virPCIGetDeviceAddressFromSysfsLink(device_link, pf) >= 0) { VIR_DEBUG("PF for VF device '%s': %.4x:%.2x:%.2x.%.1x", vf_sysfs_path, (*pf)->domain, (*pf)->bus, (*pf)->slot, (*pf)->function); } VIR_FREE(device_link); - return ret; + return 0; } -- 2.5.5
Rather than return 0/-1 and/or a pointer to some memory, adjust the helper to just return the allocated structure or NULL on failure. Adjust the callers in order to handle that Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virpci.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 1119c2e..3d18bb3 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2438,18 +2438,17 @@ virPCIDeviceAddressIsEqual(virPCIDeviceAddressPtr bdf1, (bdf1->function == bdf2->function)); } -static int -virPCIGetDeviceAddressFromSysfsLink(const char *device_link, - virPCIDeviceAddressPtr *bdf) +static virPCIDeviceAddressPtr +virPCIGetDeviceAddressFromSysfsLink(const char *device_link) { + virPCIDeviceAddressPtr bdf = NULL; char *config_address = NULL; char *device_path = NULL; char errbuf[64]; - int ret = -1; if (!virFileExists(device_link)) { VIR_DEBUG("'%s' does not exist", device_link); - return ret; + return NULL; } device_path = canonicalize_file_name(device_link); @@ -2458,26 +2457,25 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link, virReportSystemError(errno, _("Failed to resolve device link '%s'"), device_link); - return ret; + return NULL; } config_address = last_component(device_path); - if (VIR_ALLOC(*bdf) != 0) + if (VIR_ALLOC(bdf) < 0) goto out; - if (virPCIDeviceAddressParse(config_address, *bdf) != 0) { + if (virPCIDeviceAddressParse(config_address, bdf) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to parse PCI config address '%s'"), config_address); - VIR_FREE(*bdf); + VIR_FREE(bdf); goto out; } - ret = 0; out: VIR_FREE(device_path); - return ret; + return bdf; } /* @@ -2494,7 +2492,7 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, return -1; } - if (virPCIGetDeviceAddressFromSysfsLink(device_link, pf) >= 0) { + if ((*pf = virPCIGetDeviceAddressFromSysfsLink(device_link))) { VIR_DEBUG("PF for VF device '%s': %.4x:%.2x:%.2x.%.1x", vf_sysfs_path, (*pf)->domain, (*pf)->bus, (*pf)->slot, (*pf)->function); } @@ -2546,20 +2544,22 @@ virPCIGetVirtualFunctions(const char *sysfs_path, if (!virFileExists(device_link)) break; - if (virPCIGetDeviceAddressFromSysfsLink(device_link, &config_addr) < 0) { + if (!(config_addr = virPCIGetDeviceAddressFromSysfsLink(device_link))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to get SRIOV function from device link '%s'"), device_link); goto error; } - if (VIR_APPEND_ELEMENT(*virtual_functions, *num_virtual_functions, config_addr) < 0) + if (VIR_APPEND_ELEMENT(*virtual_functions, *num_virtual_functions, + config_addr) < 0) goto error; VIR_FREE(device_link); } while (1); - VIR_DEBUG("Found %zu virtual functions for %s", *num_virtual_functions, sysfs_path); + VIR_DEBUG("Found %zu virtual functions for %s", + *num_virtual_functions, sysfs_path); ret = 0; cleanup: VIR_FREE(device_link); @@ -2612,8 +2612,7 @@ virPCIGetVirtualFunctionIndex(const char *pf_sysfs_device_link, virPCIDeviceAddressPtr vf_bdf = NULL; virPCIDeviceAddressPtr *virt_fns = NULL; - if (virPCIGetDeviceAddressFromSysfsLink(vf_sysfs_device_link, - &vf_bdf) < 0) + if (!(vf_bdf = virPCIGetDeviceAddressFromSysfsLink(vf_sysfs_device_link))) return ret; if (virPCIGetVirtualFunctions(pf_sysfs_device_link, &virt_fns, -- 2.5.5
Seems recent versions of Coverity have (mostly) resolved the issue using ternary operations in VIR_FREE (and now VIR_DISPOSE*) macros. So let's just remove it and if necessary handle one off issues as the arise. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/viralloc.h | 34 ++++++---------------------------- 1 file changed, 6 insertions(+), 28 deletions(-) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 5f4e27b..008f58e 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -550,19 +550,11 @@ void virDispose(void *ptrptr, size_t count, size_t element_size, size_t *countpt * * This macro is safe to use on arguments with side effects. */ -# if !STATIC_ANALYSIS /* The ternary ensures that ptr is a non-const pointer and not an * integer type, all while evaluating ptr only once. This gives us * extra compiler safety when compiling under gcc. */ -# define VIR_FREE(ptr) virFree(1 ? (void *) &(ptr) : (ptr)) -# else -/* The Coverity static analyzer considers the else path of the "?:" and - * flags the VIR_FREE() of the address of the address of memory as a - * RESOURCE_LEAK resulting in numerous false positives (eg, VIR_FREE(&ptr)) - */ -# define VIR_FREE(ptr) virFree(&(ptr)) -# endif +# define VIR_FREE(ptr) virFree(1 ? (void *) &(ptr) : (ptr)) /** @@ -575,13 +567,8 @@ void virDispose(void *ptrptr, size_t count, size_t element_size, size_t *countpt * * This macro is safe to use on arguments with side effects. */ -# if !STATIC_ANALYSIS -/* See explanation in VIR_FREE */ -# define VIR_DISPOSE_N(ptr, count) virDispose(1 ? (void *) &(ptr) : (ptr), 0, \ +# define VIR_DISPOSE_N(ptr, count) virDispose(1 ? (void *) &(ptr) : (ptr), 0, \ sizeof(*(ptr)), &(count)) -# else -# define VIR_DISPOSE_N(ptr, count) virDispose(&(ptr), 0, sizeof(*(ptr)), &(count)) -# endif /** @@ -592,13 +579,8 @@ void virDispose(void *ptrptr, size_t count, size_t element_size, size_t *countpt * * This macro is not safe to be used on arguments with side effects. */ -# if !STATIC_ANALYSIS -/* See explanation in VIR_FREE */ -# define VIR_DISPOSE_STRING(ptr) virDispose(1 ? (void *) &(ptr) : (ptr), \ - (ptr) ? strlen((ptr)) : 0, 1, NULL) -# else -# define VIR_DISPOSE_STRING(ptr) virDispose(&(ptr), (ptr) ? strlen((ptr)) : 1, NULL) -# endif +# define VIR_DISPOSE_STRING(ptr) virDispose(1 ? (void *) &(ptr) : (ptr), \ + (ptr) ? strlen((ptr)) : 0, 1, NULL) /** @@ -609,12 +591,8 @@ void virDispose(void *ptrptr, size_t count, size_t element_size, size_t *countpt * * This macro is safe to be used on arguments with side effects. */ -# if !STATIC_ANALYSIS -/* See explanation in VIR_FREE */ -# define VIR_DISPOSE(ptr) virDispose(1 ? (void *) &(ptr) : (ptr), 1, sizeof(*(ptr)), NULL) -# else -# define VIR_DISPOSE(ptr) virDispose(&(ptr), 1, sizeof(*(ptr)), NULL) -# endif +# define VIR_DISPOSE(ptr) virDispose(1 ? (void *) &(ptr) : (ptr), 1, \ + sizeof(*(ptr)), NULL) void virAllocTestInit(void); -- 2.5.5
On Wed, May 18, 2016 at 08:07:52AM -0400, John Ferlan wrote:
Not sure which release exactly, but the coverity analysis bug I filed that was the reason for commit id 'c9a85af31' has been fixed, so it's time to remove that... In doing so uncovered another issue - it seems the fix for Coverity addressed the primary problem we've seen, but when there's functions returning allocated strings by reference that can also VIR_FREE(*string), whatever was added to address the main issue doesn't seem to recognize this other usage, resulinting in a false positive resource_leak. Luckily there's only two instances in our code for that. One is addressed by patch 3 and the other in virPCIDeviceGetDriverPathAndName which would require quite a few more changes to address (essentially split up the function - it'll be on my "to do" list).
Patch 1: Addresses an issue seen once patches 2-4 were applied - that there's a real problem with the error path. It's a simple fix.
Patches 2 & 3: Address a false positive resource leak even with patch 4 applied.
Patch 4: Remove the rather ugly !STATIC_ANALYSIS within the VIR_FREE and VIR_DISPOSE* macros. As I found with a build after the fact, VIR_DISPOSE_STRING was missing an argument anyway (it should have been ": 0, 1, NULL" instead of ": 1, NULL").
Nice to see it go.
NB: Patch 4 has been run through the private coverity server...
John Ferlan (4): util: Fix error path for virPCIGetVirtualFunctions util: Remove need for ret in virPCIGetPhysicalFunction util: Adjust return for virPCIGetDeviceAddressFromSysfsLink util: Remove need for STATIC_ANALYSIS check
src/util/viralloc.h | 34 ++++++---------------------------- src/util/virpci.c | 39 +++++++++++++++++++-------------------- 2 files changed, 25 insertions(+), 48 deletions(-)
ACK series Jan
participants (2)
-
John Ferlan -
Ján Tomko