[libvirt] [PATCH] Yet more coverity fixes
Addressing the following reports: Error: RESOURCE_LEAK: /libvirt/src/nodeinfo.c:631: alloc_fn: Calling allocation function "fopen". /libvirt/src/nodeinfo.c:631: var_assign: Assigning: "cpuinfo" = storage returned from "fopen("/proc/cpuinfo", "r")". /libvirt/src/nodeinfo.c:640: leaked_storage: Variable "cpuinfo" going out of scope leaks the storage it points to. Error: RESOURCE_LEAK: /libvirt/src/vmx/vmx.c:2431: alloc_fn: Calling allocation function "calloc". /libvirt/src/vmx/vmx.c:2431: var_assign: Assigning: "networkName" = storage returned from "calloc(1UL, 1UL)". /libvirt/src/vmx/vmx.c:2495: leaked_storage: Variable "networkName" going out of scope leaks the storage it points to. Error: RESOURCE_LEAK: /libvirt/src/util/virnetlink.c:338: alloc_arg: Calling allocation function "virAlloc" on "srv". /libvirt/src/util/memory.c:101: alloc_fn: Storage is returned from allocation function "calloc". /libvirt/src/util/memory.c:101: var_assign: Assigning: "*((void **)ptrptr)" = "calloc(1UL, size)". /libvirt/src/util/virnetlink.c:343: noescape: Variable "srv" is not freed or pointed-to in function "virMutexInit". /libvirt/src/util/threads-pthread.c:49:30: noescape: "virMutexInit" does not free or save its pointer parameter "m". /libvirt/src/util/virnetlink.c:404: leaked_storage: Variable "srv" going out of scope leaks the storage it points to. Error: RESOURCE_LEAK: /libvirt/src/qemu/qemu_driver.c:6968: alloc_fn: Calling allocation function "calloc". /libvirt/src/qemu/qemu_driver.c:6968: var_assign: Assigning: "nodeset" = storage returned from "calloc(1UL, 1UL)". /libvirt/src/qemu/qemu_driver.c:6977: noescape: Variable "nodeset" is not freed or pointed-to in function "virTypedParameterAssign". /libvirt/src/qemu/qemu_driver.c:6997: leaked_storage: Variable "nodeset" going out of scope leaks the storage it points to. Error: RESOURCE_LEAK: /libvirt/tests/qemuxml2argvtest.c:47: alloc_arg: Calling allocation function "virAlloc" on "ret". /libvirt/src/util/memory.c:101: alloc_fn: Storage is returned from allocation function "calloc". /libvirt/src/util/memory.c:101: var_assign: Assigning: "*((void **)ptrptr)" = "calloc(1UL, size)". /libvirt/tests/qemuxml2argvtest.c:54: leaked_storage: Variable "ret" going out of scope leaks the storage it points to. Error: STRING_NULL: /libvirt/src/util/uuid.c:273: string_null_argument: Function "getDMISystemUUID" does not terminate string "*dmiuuid". /libvirt/src/util/uuid.c:241: string_null_argument: Function "saferead" fills array "*uuid" with a non-terminated string. /libvirt/src/util/util.c:101: string_null_argument: Function "read" fills array "*buf" with a non-terminated string. /libvirt/src/util/uuid.c:274: string_null: Passing unterminated string "dmiuuid" to a function expecting a null-terminated string. /libvirt/src/util/uuid.c:138: var_assign_parm: Assigning: "cur" = "uuidstr". They now point to the same thing. /libvirt/src/util/uuid.c:164: string_null_sink_loop: Searching for null termination in an unterminated array "cur". Error: STRING_NULL: /libvirt/src/node_device/node_device_linux_sysfs.c:80: string_null_argument: Function "saferead" does not terminate string "*buf". /libvirt/src/util/util.c:101: string_null_argument: Function "read" fills array "*buf" with a non-terminated string. /libvirt/src/node_device/node_device_linux_sysfs.c:87: string_null: Passing unterminated string "buf" to a function expecting a null-terminated string. --- daemon/libvirtd.c | 24 +++++++++++++++++++----- src/node_device/node_device_linux_sysfs.c | 9 +++++---- src/nodeinfo.c | 1 + src/qemu/qemu_driver.c | 4 ++++ src/util/uuid.c | 5 +++-- src/util/virnetlink.c | 7 ++++++- src/vmx/vmx.c | 1 + tests/qemuxml2argvtest.c | 4 +++- 8 files changed, 42 insertions(+), 13 deletions(-) Index: libvirt-acl/daemon/libvirtd.c =================================================================== --- libvirt-acl.orig/daemon/libvirtd.c +++ libvirt-acl/daemon/libvirtd.c @@ -141,6 +141,7 @@ static int daemonForkIntoBackground(cons int stdinfd = -1; int stdoutfd = -1; int nextpid; + int tmpfd; VIR_FORCE_CLOSE(statuspipe[0]); @@ -151,16 +152,16 @@ static int daemonForkIntoBackground(cons if (dup2(stdinfd, STDIN_FILENO) != STDIN_FILENO) goto cleanup; if (dup2(stdoutfd, STDOUT_FILENO) != STDOUT_FILENO) - goto cleanup; + goto cleanup_close_stdin_fileno; if (dup2(stdoutfd, STDERR_FILENO) != STDERR_FILENO) - goto cleanup; + goto cleanup_close_stdout_fileno; if (stdinfd > STDERR_FILENO && VIR_CLOSE(stdinfd) < 0) - goto cleanup; + goto cleanup_close_stderr_fileno; if (stdoutfd > STDERR_FILENO && VIR_CLOSE(stdoutfd) < 0) - goto cleanup; + goto cleanup_close_stderr_fileno; if (setsid() < 0) - goto cleanup; + goto cleanup_close_stderr_fileno; nextpid = fork(); switch (nextpid) { @@ -172,6 +173,19 @@ static int daemonForkIntoBackground(cons _exit(EXIT_SUCCESS); } + + cleanup_close_stderr_fileno: + tmpfd = STDERR_FILENO; + VIR_FORCE_CLOSE(tmpfd); + + cleanup_close_stdout_fileno: + tmpfd = STDOUT_FILENO; + VIR_FORCE_CLOSE(tmpfd); + + cleanup_close_stdin_fileno: + tmpfd = STDIN_FILENO; + VIR_FORCE_CLOSE(tmpfd); + cleanup: VIR_FORCE_CLOSE(stdoutfd); VIR_FORCE_CLOSE(stdinfd); Index: libvirt-acl/src/node_device/node_device_linux_sysfs.c =================================================================== --- libvirt-acl.orig/src/node_device/node_device_linux_sysfs.c +++ libvirt-acl/src/node_device/node_device_linux_sysfs.c @@ -69,20 +69,21 @@ out: int read_wwn_linux(int host, const char *file, char **wwn) { char *p = NULL; - int fd = -1, retval = 0; - char buf[64]; + int fd = -1, retval = 0, len; + char buf[65]; if (open_wwn_file(LINUX_SYSFS_FC_HOST_PREFIX, host, file, &fd) < 0) { goto out; } - memset(buf, 0, sizeof(buf)); - if (saferead(fd, buf, sizeof(buf)) < 0) { + len = saferead(fd, buf, sizeof(buf) - 1); + if (len < 0) { retval = -1; VIR_DEBUG("Failed to read WWN for host%d '%s'", host, file); goto out; } + buf[len] = '\0'; p = strstr(buf, "0x"); if (p != NULL) { Index: libvirt-acl/src/nodeinfo.c =================================================================== --- libvirt-acl.orig/src/nodeinfo.c +++ libvirt-acl/src/nodeinfo.c @@ -636,6 +636,7 @@ int nodeGetInfo(virConnectPtr conn ATTRI } if (virAsprintf(&sysfs_cpuinfo, CPU_SYS_PATH) < 0) { + VIR_FORCE_FCLOSE(cpuinfo); virReportOOMError(); return -1; } Index: libvirt-acl/src/util/uuid.c =================================================================== --- libvirt-acl.orig/src/util/uuid.c +++ libvirt-acl/src/util/uuid.c @@ -269,8 +269,9 @@ virSetHostUUIDStr(const char *uuid) return EEXIST; if (!uuid) { - memset(dmiuuid, 0, sizeof(dmiuuid)); - if (!getDMISystemUUID(dmiuuid, sizeof(dmiuuid) - 1)) { + rc = getDMISystemUUID(dmiuuid, sizeof(dmiuuid) - 1); + dmiuuid[VIR_UUID_STRING_BUFLEN - 1] = '\0'; + if (!rc) { if (!virUUIDParse(dmiuuid, host_uuid)) return 0; } Index: libvirt-acl/src/util/virnetlink.c =================================================================== --- libvirt-acl.orig/src/util/virnetlink.c +++ libvirt-acl/src/util/virnetlink.c @@ -341,7 +341,7 @@ virNetlinkEventServiceStart(void) } if (virMutexInit(&srv->lock) < 0) - goto error; + goto error_free_srv; virNetlinkEventServerLock(srv); @@ -402,6 +402,11 @@ error_locked: } error: return ret; + +error_free_srv: + VIR_FREE(srv); + + return ret; } /** Index: libvirt-acl/src/vmx/vmx.c =================================================================== --- libvirt-acl.orig/src/vmx/vmx.c +++ libvirt-acl/src/vmx/vmx.c @@ -2485,6 +2485,7 @@ virVMXParseEthernet(virConfPtr conf, int *def = NULL; } + VIR_FREE(networkName); VIR_FREE(connectionType); VIR_FREE(addressType); VIR_FREE(generatedAddress); Index: libvirt-acl/tests/qemuxml2argvtest.c =================================================================== --- libvirt-acl.orig/tests/qemuxml2argvtest.c +++ libvirt-acl/tests/qemuxml2argvtest.c @@ -50,8 +50,10 @@ fakeSecretLookupByUsage(virConnectPtr co ret->magic = VIR_SECRET_MAGIC; ret->refs = 1; ret->usageID = strdup(usageID); - if (!ret->usageID) + if (!ret->usageID) { + VIR_FREE(ret); return NULL; + } ret->conn = conn; conn->refs++; return ret; Index: libvirt-acl/src/qemu/qemu_driver.c =================================================================== --- libvirt-acl.orig/src/qemu/qemu_driver.c +++ libvirt-acl/src/qemu/qemu_driver.c @@ -6984,6 +6984,9 @@ qemuDomainGetNumaParameters(virDomainPtr if (virTypedParameterAssign(param, VIR_DOMAIN_NUMA_NODESET, VIR_TYPED_PARAM_STRING, nodeset) < 0) goto cleanup; + + nodeset = NULL; + break; default: @@ -6997,6 +7000,7 @@ qemuDomainGetNumaParameters(virDomainPtr ret = 0; cleanup: + VIR_FREE(nodeset); virCgroupFree(&group); if (vm) virDomainObjUnlock(vm);
On Mon, Apr 30, 2012 at 12:28:26PM -0400, Stefan Berger wrote:
Addressing the following reports:
None of those reports mention daemon/libvirtd.c and yet
daemon/libvirtd.c | 24 +++++++++++++++++++-----
Index: libvirt-acl/daemon/libvirtd.c =================================================================== --- libvirt-acl.orig/daemon/libvirtd.c +++ libvirt-acl/daemon/libvirtd.c @@ -141,6 +141,7 @@ static int daemonForkIntoBackground(cons int stdinfd = -1; int stdoutfd = -1; int nextpid; + int tmpfd;
VIR_FORCE_CLOSE(statuspipe[0]);
@@ -151,16 +152,16 @@ static int daemonForkIntoBackground(cons if (dup2(stdinfd, STDIN_FILENO) != STDIN_FILENO) goto cleanup; if (dup2(stdoutfd, STDOUT_FILENO) != STDOUT_FILENO) - goto cleanup; + goto cleanup_close_stdin_fileno; if (dup2(stdoutfd, STDERR_FILENO) != STDERR_FILENO) - goto cleanup; + goto cleanup_close_stdout_fileno; if (stdinfd > STDERR_FILENO && VIR_CLOSE(stdinfd) < 0) - goto cleanup; + goto cleanup_close_stderr_fileno; if (stdoutfd > STDERR_FILENO && VIR_CLOSE(stdoutfd) < 0) - goto cleanup; + goto cleanup_close_stderr_fileno;
if (setsid() < 0) - goto cleanup; + goto cleanup_close_stderr_fileno;
nextpid = fork(); switch (nextpid) { @@ -172,6 +173,19 @@ static int daemonForkIntoBackground(cons _exit(EXIT_SUCCESS); }
+ + cleanup_close_stderr_fileno: + tmpfd = STDERR_FILENO; + VIR_FORCE_CLOSE(tmpfd); + + cleanup_close_stdout_fileno: + tmpfd = STDOUT_FILENO; + VIR_FORCE_CLOSE(tmpfd); + + cleanup_close_stdin_fileno: + tmpfd = STDIN_FILENO; + VIR_FORCE_CLOSE(tmpfd); + cleanup: VIR_FORCE_CLOSE(stdoutfd); VIR_FORCE_CLOSE(stdinfd);
This really seems like overkill & ugly. There is no real world leak here since this process will immediately exit. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 04/30/2012 12:34 PM, Daniel P. Berrange wrote:
On Mon, Apr 30, 2012 at 12:28:26PM -0400, Stefan Berger wrote:
Addressing the following reports:
None of those reports mention daemon/libvirtd.c and yet
These must have gotten lost: Error: RESOURCE_LEAK: /libvirt/daemon/libvirtd.c:147: open_fn: Calling opening function "open". /libvirt/daemon/libvirtd.c:147: var_assign: Assigning: "stdinfd" = handle returned from "open("/dev/null", 0)". /libvirt/daemon/libvirtd.c:151: noescape: Variable "stdinfd" is not closed or saved in function "dup2". /libvirt/daemon/libvirtd.c:168: leaked_handle: Handle variable "stdinfd" going out of scope leaks the handle. Error: RESOURCE_LEAK: /libvirt/daemon/libvirtd.c:149: open_fn: Calling opening function "open". /libvirt/daemon/libvirtd.c:149: var_assign: Assigning: "stdoutfd" = handle returned from "open("/dev/null", 1)". /libvirt/daemon/libvirtd.c:153: noescape: Variable "stdoutfd" is not closed or saved in function "dup2". /libvirt/daemon/libvirtd.c:155: noescape: Variable "stdoutfd" is not closed or saved in function "dup2". /libvirt/daemon/libvirtd.c:168: leaked_handle: Handle variable "stdoutfd" going out of scope leaks the handle.
daemon/libvirtd.c | 24 +++++++++++++++++++----- @@ -172,6 +173,19 @@ static int daemonForkIntoBackground(cons _exit(EXIT_SUCCESS); }
+ + cleanup_close_stderr_fileno: + tmpfd = STDERR_FILENO; + VIR_FORCE_CLOSE(tmpfd); + + cleanup_close_stdout_fileno: + tmpfd = STDOUT_FILENO; + VIR_FORCE_CLOSE(tmpfd); + + cleanup_close_stdin_fileno: + tmpfd = STDIN_FILENO; + VIR_FORCE_CLOSE(tmpfd); + cleanup: VIR_FORCE_CLOSE(stdoutfd); VIR_FORCE_CLOSE(stdinfd); This really seems like overkill& ugly. There is no real world leak here since this process will immediately exit.
Right, though this should make coverity quiet... Stefan
On Mon, Apr 30, 2012 at 12:42:37PM -0400, Stefan Berger wrote:
On 04/30/2012 12:34 PM, Daniel P. Berrange wrote:
On Mon, Apr 30, 2012 at 12:28:26PM -0400, Stefan Berger wrote:
Addressing the following reports:
None of those reports mention daemon/libvirtd.c and yet
These must have gotten lost:
Error: RESOURCE_LEAK: /libvirt/daemon/libvirtd.c:147: open_fn: Calling opening function "open". /libvirt/daemon/libvirtd.c:147: var_assign: Assigning: "stdinfd" = handle returned from "open("/dev/null", 0)". /libvirt/daemon/libvirtd.c:151: noescape: Variable "stdinfd" is not closed or saved in function "dup2". /libvirt/daemon/libvirtd.c:168: leaked_handle: Handle variable "stdinfd" going out of scope leaks the handle.
Error: RESOURCE_LEAK: /libvirt/daemon/libvirtd.c:149: open_fn: Calling opening function "open". /libvirt/daemon/libvirtd.c:149: var_assign: Assigning: "stdoutfd" = handle returned from "open("/dev/null", 1)". /libvirt/daemon/libvirtd.c:153: noescape: Variable "stdoutfd" is not closed or saved in function "dup2". /libvirt/daemon/libvirtd.c:155: noescape: Variable "stdoutfd" is not closed or saved in function "dup2". /libvirt/daemon/libvirtd.c:168: leaked_handle: Handle variable "stdoutfd" going out of scope leaks the handle.
daemon/libvirtd.c | 24 +++++++++++++++++++----- @@ -172,6 +173,19 @@ static int daemonForkIntoBackground(cons _exit(EXIT_SUCCESS); }
+ + cleanup_close_stderr_fileno: + tmpfd = STDERR_FILENO; + VIR_FORCE_CLOSE(tmpfd); + + cleanup_close_stdout_fileno: + tmpfd = STDOUT_FILENO; + VIR_FORCE_CLOSE(tmpfd); + + cleanup_close_stdin_fileno: + tmpfd = STDIN_FILENO; + VIR_FORCE_CLOSE(tmpfd); + cleanup: VIR_FORCE_CLOSE(stdoutfd); VIR_FORCE_CLOSE(stdinfd); This really seems like overkill& ugly. There is no real world leak here since this process will immediately exit.
Right, though this should make coverity quiet...
Really ? I'm not convinced.
/libvirt/daemon/libvirtd.c:168: leaked_handle: Handle variable "stdinfd" going out of scope leaks the handle.
This is refering to the 3rd line here: switch (nextpid) { case 0: /* grandchild */ return statuspipe[1]; Neither the original 'cleanup:' label, nor any of the 3 you added will ever execute in the codepath that coverity is complaining about. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 04/30/2012 12:55 PM, Daniel P. Berrange wrote:
On Mon, Apr 30, 2012 at 12:42:37PM -0400, Stefan Berger wrote:
/libvirt/daemon/libvirtd.c:168: leaked_handle: Handle variable "stdinfd" going out of scope leaks the handle.
This is refering to the 3rd line here:
switch (nextpid) { case 0: /* grandchild */ return statuspipe[1];
Neither the original 'cleanup:' label, nor any of the 3 you added will ever execute in the codepath that coverity is complaining about. Hm, dropping the libvirtd.c changes...
Stefan
participants (2)
-
Daniel P. Berrange -
Stefan Berger