7:07 a.m.
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
7:07 a.m.
New subject: [libvirt] [PATCH 1/4] util: Fix error path for virPCIGetVirtualFunctions
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(a)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
7:07 a.m.
New subject: [libvirt] [PATCH 2/4] util: Remove need for ret in virPCIGetPhysicalFunction
Since the callers only ever expect 0 or -1, let's just return that directly
Signed-off-by: John Ferlan <jferlan(a)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
7:07 a.m.
New subject: [libvirt] [PATCH 3/4] util: Adjust return for virPCIGetDeviceAddressFromSysfsLink
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(a)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
7:07 a.m.
New subject: [libvirt] [PATCH 4/4] util: Remove need for STATIC_ANALYSIS check
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(a)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
11:08 a.m.
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
3147
days inactive
3148
days old
5 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko