[libvirt] [PATCH 00/14] Resolve more Coverity issues
I got answer from someone on the Coverity boards regarding a false positive related to how VIR_FREE() was operating when passed an address of an address to some memory, see my note from last week: https://www.redhat.com/archives/libvir-list/2013-January/msg01353.html Essentially the issue is the "?:" construct in the VIR_FREE() macro and Coverity taking the "else" condition as a possible path even though it technically couldn't happen ((1) ? xxx : yyy). The suggestion made was to remove the "?:", but since this only happens in the static analysis case I used the existing STATIC_ANALYSIS build directive. This change resolved about 100 false positives. In doing this, Coverity uncovered a few more uninitialized variables prior to VIR_FREE calls and a couple of instances where VIR_FREE was being called on already free'd memory plus one instance where a pointer to free'd memory was being returned (in qemumonitortestutils.c). John Ferlan (14): viralloc: Adjust definition of VIR_FREE() for Coverity conf: Need to initialize variables before VIR_FREE virnetserver: Need to initialize 'sigdata' virnetsockettest: Need to initialize 'path' virnetdev: Need to initialize 'pciConfigAddr' commandtest: Need to initialize 'errbuf' virfile: Need to initialize 'looppath' lxc: Need to initialize 'dst' virsh: Need to intialize 'str' storage: Need to initialize 'zerobuf' interface: Need to initialize 'ifaces_list' security: Need to initialize 'sens' virkeepalive: Remove erroneous VIR_FREE(msg) tests: Need to initialize 'test' properly on error path src/conf/domain_audit.c | 4 ++-- src/interface/interface_backend_udev.c | 2 +- src/lxc/lxc_driver.c | 4 ++-- src/rpc/virkeepalive.c | 1 - src/rpc/virnetserver.c | 2 +- src/security/security_selinux.c | 2 +- src/storage/storage_backend.c | 2 +- src/util/viralloc.h | 11 ++++++++++- src/util/virfile.c | 2 +- src/util/virnetdev.c | 2 +- tests/commandtest.c | 2 +- tests/qemumonitortestutils.c | 1 + tests/virnetsockettest.c | 4 ++-- tools/virsh.c | 2 +- 14 files changed, 25 insertions(+), 16 deletions(-) -- 1.7.11.7
The Coverity static analyzer was generating many false positives for the unary operation inside the VIR_FREE() definition as it was trying to evaluate the else portion of the "?:" even though the if portion was (1). --- src/util/viralloc.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 37ec5ee..0c9b177 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -365,7 +365,16 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * while evaluating ptr only once. For now, we intentionally cast * away const, since a number of callers safely pass const char *. */ -# define VIR_FREE(ptr) virFree((void *) (1 ? (const void *) &(ptr) : (ptr))) +# if !STATIC_ANALYSIS +# define VIR_FREE(ptr) virFree((void *) (1 ? (const 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((void *) ((const void *) &(ptr))) +# endif + # if TEST_OOM -- 1.7.11.7
On 01/22/13 15:15, John Ferlan wrote:
The Coverity static analyzer was generating many false positives for the unary operation inside the VIR_FREE() definition as it was trying to evaluate the else portion of the "?:" even though the if portion was (1). --- src/util/viralloc.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 37ec5ee..0c9b177 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -365,7 +365,16 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * while evaluating ptr only once. For now, we intentionally cast * away const, since a number of callers safely pass const char *. */ -# define VIR_FREE(ptr) virFree((void *) (1 ? (const void *) &(ptr) : (ptr))) +# if !STATIC_ANALYSIS +# define VIR_FREE(ptr) virFree((void *) (1 ? (const 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((void *) ((const void *) &(ptr))) +# endif +
Uh, this is black magic to me. I leave this one on Eric who added the ternary magic. Peter
# if TEST_OOM
On 01/22/2013 07:15 AM, John Ferlan wrote:
The Coverity static analyzer was generating many false positives for the unary operation inside the VIR_FREE() definition as it was trying to evaluate the else portion of the "?:" even though the if portion was (1).
Simplifying VIR_FREE for Coverity is fine by me - we get the best of both worlds: gcc compilation points out type mismatches, and Coverity doesn't get confused with false positives.
-# define VIR_FREE(ptr) virFree((void *) (1 ? (const void *) &(ptr) : (ptr))) +# if !STATIC_ANALYSIS +# define VIR_FREE(ptr) virFree((void *) (1 ? (const 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((void *) ((const void *) &(ptr)))
However, this is too complex. It is sufficient to do: # define VIR_FREE(ptr) virFree((void *) &(ptr)) ACK with that change, so I'll go ahead and push it in your name. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Resolve a couple of instances where variables were not initialized prior to potential VIR_FREE call in cleanup path. --- src/conf/domain_audit.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_audit.c b/src/conf/domain_audit.c index 7082804..c00bd11 100644 --- a/src/conf/domain_audit.c +++ b/src/conf/domain_audit.c @@ -201,7 +201,7 @@ virDomainAuditNetDevice(virDomainDefPtr vmDef, virDomainNetDefPtr netDef, char uuidstr[VIR_UUID_STRING_BUFLEN]; char macstr[VIR_MAC_STRING_BUFLEN]; char *vmname; - char *dev_name; + char *dev_name = NULL; char *rdev; const char *virt; @@ -504,7 +504,7 @@ virDomainAuditCgroupPath(virDomainObjPtr vm, virCgroupPtr cgroup, { char *detail; char *rdev; - char *extra; + char *extra = NULL; /* Nothing to audit for regular files. */ if (rc > 0) -- 1.7.11.7
On 01/22/13 15:15, John Ferlan wrote:
Resolve a couple of instances where variables were not initialized prior to potential VIR_FREE call in cleanup path. --- src/conf/domain_audit.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK. Peter
It was possible to call VIR_FREE in error prior to initialization --- src/rpc/virnetserver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index cbbc986..95333d0 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -926,7 +926,7 @@ int virNetServerAddSignalHandler(virNetServerPtr srv, virNetServerSignalFunc func, void *opaque) { - virNetServerSignalPtr sigdata; + virNetServerSignalPtr sigdata = NULL; struct sigaction sig_action; virObjectLock(srv); -- 1.7.11.7
It was possible to call VIR_FREE in cleanup prior to initialization --- tests/virnetsockettest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index b968973..aaa6acb 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -207,7 +207,7 @@ static int testSocketUNIXAccept(const void *data ATTRIBUTE_UNUSED) virNetSocketPtr csock = NULL; /* Client socket */ int ret = -1; - char *path; + char *path = NULL; char *tmpdir; char template[] = "/tmp/libvirt_XXXXXX"; @@ -257,7 +257,7 @@ static int testSocketUNIXAddrs(const void *data ATTRIBUTE_UNUSED) virNetSocketPtr csock = NULL; /* Client socket */ int ret = -1; - char *path; + char *path = NULL; char *tmpdir; char template[] = "/tmp/libvirt_XXXXXX"; -- 1.7.11.7
On 01/22/13 15:15, John Ferlan wrote:
It was possible to call VIR_FREE in cleanup prior to initialization --- tests/virnetsockettest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK. Peter
It was possible to call VIR_FREE in cleanup prior to initialization --- src/util/virnetdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 7ffd3c2..295884f 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -986,7 +986,7 @@ virNetDevGetVirtualFunctions(const char *pfname, int ret = -1, i; char *pf_sysfs_device_link = NULL; char *pci_sysfs_device_link = NULL; - char *pciConfigAddr; + char *pciConfigAddr = NULL; if (virNetDevSysfsFile(&pf_sysfs_device_link, pfname, "device") < 0) return ret; -- 1.7.11.7
It was possible to call VIR_FREE in cleanup prior to initialization --- tests/commandtest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/commandtest.c b/tests/commandtest.c index fd2b8c0..b5c5882 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -656,7 +656,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED) virCommandPtr cmd = virCommandNew("true"); int ret = -1; char *outbuf; - char *errbuf; + char *errbuf = NULL; virCommandSetOutputBuffer(cmd, &outbuf); if (outbuf != NULL) { -- 1.7.11.7
It was possible to call VIR_FREE in cleanup prior to initialization. --- src/util/virfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 50999aa..5cca54d 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -602,7 +602,7 @@ static int virFileLoopDeviceOpen(char **dev_name) int fd = -1; DIR *dh = NULL; struct dirent *de; - char *looppath; + char *looppath = NULL; struct loop_info64 lo; VIR_DEBUG("Looking for loop devices in /dev"); -- 1.7.11.7
It was possible to call VIR_FREE in cleanup prior to initialization --- src/lxc/lxc_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 3268e22..175ea27 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -3821,7 +3821,7 @@ lxcDomainDetachDeviceDiskLive(virLXCDriverPtr driver, virDomainDiskDefPtr def = NULL; virCgroupPtr group = NULL; int i, ret = -1; - char *dst; + char *dst = NULL; if (!priv->initpid) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -3958,7 +3958,7 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriverPtr driver, virDomainHostdevDefPtr def = NULL; virCgroupPtr group = NULL; int idx, ret = -1; - char *dst; + char *dst = NULL; char *vroot; usbDevice *usb = NULL; -- 1.7.11.7
It was possible to call VIR_FREE in error prior to initialization. --- tools/virsh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh.c b/tools/virsh.c index 908c6a1..27ed511 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2268,7 +2268,7 @@ vshOutputLogFile(vshControl *ctl, int log_level, const char *msg_format, va_list ap) { virBuffer buf = VIR_BUFFER_INITIALIZER; - char *str; + char *str = NULL; size_t len; const char *lvl = ""; time_t stTime; -- 1.7.11.7
It was possible to call VIR_FREE in cleanup prior to initialization. --- src/storage/storage_backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index c2c4c51..cab72c6 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -141,7 +141,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol, size_t rbytes = READ_BLOCK_SIZE_DEFAULT; size_t wbytes = 0; int interval; - char *zerobuf; + char *zerobuf = NULL; char *buf = NULL; struct stat st; -- 1.7.11.7
It was possible to call VIR_FREE in cleanup prior to initialization --- src/interface/interface_backend_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index d259043..92c35d9 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -290,7 +290,7 @@ udevIfaceListAllInterfaces(virConnectPtr conn, struct udev_enumerate *enumerate = NULL; struct udev_list_entry *devices; struct udev_list_entry *dev_entry; - virInterfacePtr *ifaces_list; + virInterfacePtr *ifaces_list = NULL; virInterfacePtr iface_obj; int tmp_count; int count = 0; -- 1.7.11.7
On 01/22/13 15:15, John Ferlan wrote:
It was possible to call VIR_FREE in cleanup prior to initialization --- src/interface/interface_backend_udev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK. Peter
It was possible to call VIR_FREE in cleanup prior to initialization --- src/security/security_selinux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index b5e1a9a..99e3f9e 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -110,7 +110,7 @@ virSecuritySELinuxMCSFind(virSecurityManagerPtr mgr) char *mcs = NULL; security_context_t ourSecContext = NULL; context_t ourContext = NULL; - char *sens, *cat, *tmp; + char *sens = NULL, *cat, *tmp; int catMin, catMax, catRange; if (getcon_raw(&ourSecContext) < 0) { -- 1.7.11.7
The 'msg' free is handled via virNetMessageFree() already. --- src/rpc/virkeepalive.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/rpc/virkeepalive.c b/src/rpc/virkeepalive.c index 5fe9e8b..f57fed4 100644 --- a/src/rpc/virkeepalive.c +++ b/src/rpc/virkeepalive.c @@ -108,7 +108,6 @@ virKeepAliveMessage(virKeepAlivePtr ka, int proc) error: VIR_WARN("Failed to generate keepalive %s", procstr); - VIR_FREE(msg); return NULL; } -- 1.7.11.7
In the error path, the test buffer is free'd, but due to how the free routine is written the 'test' buffer pointer does not return to the caller as NULL and then the free'd buffer address is returned to the caller. --- tests/qemumonitortestutils.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index eb9174d..d1b2ab5 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -536,6 +536,7 @@ no_memory: error: VIR_FREE(tmpdir_template); qemuMonitorTestFree(test); + test = NULL; goto cleanup; } -- 1.7.11.7
On 01/22/13 15:15, John Ferlan wrote:
In the error path, the test buffer is free'd, but due to how the free routine is written the 'test' buffer pointer does not return to the caller as NULL and then the free'd buffer address is returned to the caller. --- tests/qemumonitortestutils.c | 1 + 1 file changed, 1 insertion(+)
ACK. Peter
I've pushed patches 2 to 14 of this series. I'd like to see Eric's opinion on 1/14. Peter
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa