[libvirt] [PATCH 00/14] Resolve some RESOURCE_LEAK errors found by Coverity
This patch addresses some "Error: RESOURCE_LEAK (CWE-404)" errors found by Coverity. There are 100+ remaining; however, many seem to be false positives which will be addressed in a separate patch. The esx_driver.c and hyperv_driver.c changes could be tagged as false positives; however, the changes made will avoid the Coverity error since the freeing of the resource marked isn't based soley on the result. The phyp_driver.c changes in openSSHSession() and its callers were the most involved since the code with now use connection_data->sock in order to track the socket so as to allow it to be closed once done with it. Of interest in this change is that phypExec() used connection_data->sock even though it had never been initialized. John Ferlan (14): phyp: Resolve some file descriptor leaks esx: Address resource leak found by Coverity storage: Resolve resource leak using 'vol' buffer util: Resolve resource leak for 'res' in virSetInherit error path. locking: Resolve resource leak with 'dir' on non error path rpc: Avoid resource leak of 'socks' if any object append fails uml: Avoid resource leak of event in umlInofityEvent test: Resource resource leak with 'tmp_vols' storage: Resource resource leak using 'tmp_vols' interface: Resolve resource leak wth 'tmp_iface_objs' xen: Resource resource leak with 'cpuset' parallels: Resolve some resource leaks storage: Need to also VIR_FREE(reg) hyperv: Address resource leak found by Coverityo src/esx/esx_driver.c | 8 ++++---- src/hyperv/hyperv_driver.c | 10 +++++----- src/interface/interface_backend_netcf.c | 1 + src/locking/lock_driver_sanlock.c | 1 + src/parallels/parallels_driver.c | 30 +++++++++++++++++++----------- src/phyp/phyp_driver.c | 19 ++++++++++++++----- src/rpc/virnetserverservice.c | 6 +++--- src/storage/storage_backend_logical.c | 1 + src/storage/storage_backend_rbd.c | 13 ++++++++++--- src/storage/storage_driver.c | 1 + src/test/test_driver.c | 1 + src/uml/uml_driver.c | 6 ++++-- src/util/virlockspace.c | 1 + src/xen/xend_internal.c | 12 ++++++------ 14 files changed, 71 insertions(+), 39 deletions(-) -- 1.7.11.7
The phypUUIDTable_Push and phypUUIDTable_Pull leaked their fd's on normal return. I also noted that the Read function had a cut-n-paste error from the write function on a couple of VIR_WARN's The openSSHSession leaked the sock on the failure path. Additionally that turns into the internal_socket in the phypOpen code. That was neither saved nor closed on any path. So I used the connnection_data->sock field to save the socket for eventual close. Of interest here is that phypExec used the connection_data->sock field even though it had never been initialized. --- src/phyp/phyp_driver.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index f89871f..b74cab8 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -577,6 +577,7 @@ phypUUIDTable_Push(virConnectPtr conn) libssh2_channel_free(channel); channel = NULL; } + VIR_FORCE_FCLOSE(fd); virBufferFreeAndReset(&username); return 0; @@ -665,7 +666,7 @@ phypUUIDTable_ReadFile(virConnectPtr conn) int id; if ((fd = open(local_file, O_RDONLY)) == -1) { - VIR_WARN("Unable to write information to local file."); + VIR_WARN("Unable to read information from local file."); goto err; } @@ -682,13 +683,13 @@ phypUUIDTable_ReadFile(virConnectPtr conn) uuid_table->lpars[i]->id = id; } else { VIR_WARN - ("Unable to read from information to local file."); + ("Unable to read from information from local file."); goto err; } rc = read(fd, uuid_table->lpars[i]->uuid, VIR_UUID_BUFLEN); if (rc != VIR_UUID_BUFLEN) { - VIR_WARN("Unable to read information to local file."); + VIR_WARN("Unable to read information from local file."); goto err; } } @@ -713,7 +714,7 @@ phypUUIDTable_Pull(virConnectPtr conn) struct stat fileinfo; char buffer[1024]; int rc = 0; - int fd; + int fd = -1; int got = 0; int amount = 0; int total = 0; @@ -820,6 +821,7 @@ err: libssh2_channel_free(channel); channel = NULL; } + VIR_FORCE_CLOSE(fd); return -1; } @@ -985,7 +987,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, const char *hostname = conn->uri->server; char *username = NULL; char *password = NULL; - int sock; + int sock = -1; int rc; struct addrinfo *ai = NULL, *cur; struct addrinfo hints; @@ -1135,6 +1137,7 @@ disconnect: libssh2_session_disconnect(session, "Disconnecting..."); libssh2_session_free(session); err: + VIR_FORCE_CLOSE(sock); VIR_FREE(userhome); VIR_FREE(pubkey); VIR_FREE(pvtkey); @@ -1191,6 +1194,7 @@ phypOpen(virConnectPtr conn, virReportOOMError(); goto failure; } + connection_data->sock = -1; if (conn->uri->path) { /* need to shift one byte in order to remove the first "/" of URI component */ @@ -1227,6 +1231,7 @@ phypOpen(virConnectPtr conn, } connection_data->session = session; + connection_data->sock = internal_socket; uuid_table->nlpars = 0; uuid_table->lpars = NULL; @@ -1270,6 +1275,8 @@ failure: libssh2_session_free(session); } + if (connection_data) + VIR_FORCE_CLOSE(connection_data->sock); VIR_FREE(connection_data); return VIR_DRV_OPEN_ERROR; @@ -1289,6 +1296,8 @@ phypClose(virConnectPtr conn) phypUUIDTable_Free(phyp_driver->uuid_table); VIR_FREE(phyp_driver->managed_system); VIR_FREE(phyp_driver); + + VIR_FORCE_CLOSE(connection_data->sock); VIR_FREE(connection_data); return 0; } -- 1.7.11.7
On 01/09/2013 07:54 AM, John Ferlan wrote:
The phypUUIDTable_Push and phypUUIDTable_Pull leaked their fd's on normal return. I also noted that the Read function had a cut-n-paste error from the write function on a couple of VIR_WARN's
The openSSHSession leaked the sock on the failure path. Additionally that turns into the internal_socket in the phypOpen code. That was neither saved nor closed on any path. So I used the connnection_data->sock field to save the socket for eventual close. Of interest here is that phypExec used the connection_data->sock field even though it had never been initialized. --- src/phyp/phyp_driver.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index f89871f..b74cab8 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -577,6 +577,7 @@ phypUUIDTable_Push(virConnectPtr conn) libssh2_channel_free(channel); channel = NULL; } + VIR_FORCE_FCLOSE(fd);
Who named their FILE* 'fd' anyways? But not your fault that you are trying to clean up some notoriously hairy code.
virBufferFreeAndReset(&username); return 0;
Hmm, the code feels rather repetitive: if (channel) { libssh2_channel_send_eof(channel); libssh2_channel_wait_eof(channel); libssh2_channel_wait_closed(channel); libssh2_channel_free(channel); channel = NULL; } VIR_FORCE_FCLOSE(fd); virBufferFreeAndReset(&username); return 0; err: if (channel) { libssh2_channel_send_eof(channel); libssh2_channel_wait_eof(channel); libssh2_channel_wait_closed(channel); libssh2_channel_free(channel); channel = NULL; } VIR_FORCE_FCLOSE(fd); return -1; } Also, the virBufferFreeAndReset(&username) on the success path is useless (the only way to get there is if username was already freed earlier), and the rest of the code is identical except for the return value. Oh, and the entire 'username' variable is pointless - it is either empty or precisely conn->uri->user; no need to malloc a buffer to copy a string when we can just use the string directly. How about we do this instead: diff --git i/src/phyp/phyp_driver.c w/src/phyp/phyp_driver.c index f89871f..87a94c0 100644 --- i/src/phyp/phyp_driver.c +++ w/src/phyp/phyp_driver.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2012 Red Hat, Inc. + * Copyright (C) 2010-2013 Red Hat, Inc. * Copyright IBM Corp. 2009 * * phyp_driver.c: ssh layer to access Power Hypervisors @@ -493,42 +493,30 @@ phypUUIDTable_Push(virConnectPtr conn) ConnectionData *connection_data = conn->networkPrivateData; LIBSSH2_SESSION *session = connection_data->session; LIBSSH2_CHANNEL *channel = NULL; - virBuffer username = VIR_BUFFER_INITIALIZER; struct stat local_fileinfo; char buffer[1024]; int rc = 0; - FILE *fd = NULL; + FILE *f = NULL; size_t nread, sent; char *ptr; char local_file[] = "./uuid_table"; char *remote_file = NULL; + int ret = -1; - if (conn->uri->user != NULL) { - virBufferAdd(&username, conn->uri->user, -1); - - if (virBufferError(&username)) { - virBufferFreeAndReset(&username); - virReportOOMError(); - goto err; - } - } - - if (virAsprintf - (&remote_file, "/home/%s/libvirt_uuid_table", - virBufferContentAndReset(&username)) - < 0) { + if (virAsprintf(&remote_file, "/home/%s/libvirt_uuid_table", + NULLSTR(conn->uri->user)) < 0) { virReportOOMError(); - goto err; + goto cleanup; } if (stat(local_file, &local_fileinfo) == -1) { VIR_WARN("Unable to stat local file."); - goto err; + goto cleanup; } - if (!(fd = fopen(local_file, "rb"))) { + if (!(f = fopen(local_file, "rb"))) { VIR_WARN("Unable to open local file."); - goto err; + goto cleanup; } do { @@ -539,18 +527,18 @@ phypUUIDTable_Push(virConnectPtr conn) if ((!channel) && (libssh2_session_last_errno(session) != LIBSSH2_ERROR_EAGAIN)) - goto err; + goto cleanup; } while (!channel); do { - nread = fread(buffer, 1, sizeof(buffer), fd); + nread = fread(buffer, 1, sizeof(buffer), f); if (nread <= 0) { - if (feof(fd)) { + if (feof(f)) { /* end of file */ break; } else { VIR_ERROR(_("Failed to read from %s"), local_file); - goto err; + goto cleanup; } } ptr = buffer; @@ -570,17 +558,9 @@ phypUUIDTable_Push(virConnectPtr conn) } while (rc > 0 && sent < nread); } while (1); - if (channel) { - libssh2_channel_send_eof(channel); - libssh2_channel_wait_eof(channel); - libssh2_channel_wait_closed(channel); - libssh2_channel_free(channel); - channel = NULL; - } - virBufferFreeAndReset(&username); - return 0; + ret = 0; -err: +cleanup: if (channel) { libssh2_channel_send_eof(channel); libssh2_channel_wait_eof(channel); @@ -588,8 +568,8 @@ err: libssh2_channel_free(channel); channel = NULL; } - VIR_FORCE_FCLOSE(fd); - return -1; + VIR_FORCE_FCLOSE(f); + return ret; } static int [I didn't look at the rest of the patch, I was so distracted by the poor quality of this one function] -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Jan 09, 2013 at 07:08:28PM -0700, Eric Blake wrote:
On 01/09/2013 07:54 AM, John Ferlan wrote:
The phypUUIDTable_Push and phypUUIDTable_Pull leaked their fd's on normal return. I also noted that the Read function had a cut-n-paste error from the write function on a couple of VIR_WARN's
The openSSHSession leaked the sock on the failure path. Additionally that turns into the internal_socket in the phypOpen code. That was neither saved nor closed on any path. So I used the connnection_data->sock field to save the socket for eventual close. Of interest here is that phypExec used the connection_data->sock field even though it had never been initialized. --- src/phyp/phyp_driver.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index f89871f..b74cab8 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -577,6 +577,7 @@ phypUUIDTable_Push(virConnectPtr conn) libssh2_channel_free(channel); channel = NULL; } + VIR_FORCE_FCLOSE(fd);
Who named their FILE* 'fd' anyways? But not your fault that you are trying to clean up some notoriously hairy code.
For anyone feeling brave, with access to suitable hardware, it would be desirable to port this code over to use virNetSocket which now has built-in support for libssh2 based tunnelling. 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 :|
The phypUUIDTable_Push and phypUUIDTable_Pull leaked their file descriptors on normal return. Each function had an unnecessary use of creating a buffer to print conn->uri->user and needed a bit better flow control. I also noted that the Read function had a cut-n-paste error from the write function on a couple of VIR_WARN's. The openSSHSession leaked the sock on the failure path. Additionally that turns into the internal_socket in the phypOpen code. That was neither saved nor closed on any path. So I used the connnection_data->sock field to save the socket for eventual close. Of interest here is that phypExec used the connection_data->sock field even though it had never been initialized. --- src/phyp/phyp_driver.c | 114 ++++++++++++++++++------------------------------- 1 file changed, 41 insertions(+), 73 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index f89871f..f6c2579 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -493,42 +493,30 @@ phypUUIDTable_Push(virConnectPtr conn) ConnectionData *connection_data = conn->networkPrivateData; LIBSSH2_SESSION *session = connection_data->session; LIBSSH2_CHANNEL *channel = NULL; - virBuffer username = VIR_BUFFER_INITIALIZER; struct stat local_fileinfo; char buffer[1024]; int rc = 0; - FILE *fd = NULL; + FILE *f = NULL; size_t nread, sent; char *ptr; char local_file[] = "./uuid_table"; char *remote_file = NULL; + int ret = -1; - if (conn->uri->user != NULL) { - virBufferAdd(&username, conn->uri->user, -1); - - if (virBufferError(&username)) { - virBufferFreeAndReset(&username); - virReportOOMError(); - goto err; - } - } - - if (virAsprintf - (&remote_file, "/home/%s/libvirt_uuid_table", - virBufferContentAndReset(&username)) - < 0) { + if (virAsprintf(&remote_file, "/home/%s/libvirt_uuid_table", + NULLSTR(conn->uri->user)) < 0) { virReportOOMError(); - goto err; + goto cleanup; } if (stat(local_file, &local_fileinfo) == -1) { VIR_WARN("Unable to stat local file."); - goto err; + goto cleanup; } - if (!(fd = fopen(local_file, "rb"))) { + if (!(f = fopen(local_file, "rb"))) { VIR_WARN("Unable to open local file."); - goto err; + goto cleanup; } do { @@ -539,18 +527,18 @@ phypUUIDTable_Push(virConnectPtr conn) if ((!channel) && (libssh2_session_last_errno(session) != LIBSSH2_ERROR_EAGAIN)) - goto err; + goto cleanup; } while (!channel); do { - nread = fread(buffer, 1, sizeof(buffer), fd); + nread = fread(buffer, 1, sizeof(buffer), f); if (nread <= 0) { - if (feof(fd)) { + if (feof(f)) { /* end of file */ break; } else { VIR_ERROR(_("Failed to read from %s"), local_file); - goto err; + goto cleanup; } } ptr = buffer; @@ -570,17 +558,9 @@ phypUUIDTable_Push(virConnectPtr conn) } while (rc > 0 && sent < nread); } while (1); - if (channel) { - libssh2_channel_send_eof(channel); - libssh2_channel_wait_eof(channel); - libssh2_channel_wait_closed(channel); - libssh2_channel_free(channel); - channel = NULL; - } - virBufferFreeAndReset(&username); - return 0; + ret = 0; -err: +cleanup: if (channel) { libssh2_channel_send_eof(channel); libssh2_channel_wait_eof(channel); @@ -588,8 +568,8 @@ err: libssh2_channel_free(channel); channel = NULL; } - VIR_FORCE_FCLOSE(fd); - return -1; + VIR_FORCE_FCLOSE(f); + return ret; } static int @@ -665,7 +645,7 @@ phypUUIDTable_ReadFile(virConnectPtr conn) int id; if ((fd = open(local_file, O_RDONLY)) == -1) { - VIR_WARN("Unable to write information to local file."); + VIR_WARN("Unable to read information from local file."); goto err; } @@ -682,13 +662,13 @@ phypUUIDTable_ReadFile(virConnectPtr conn) uuid_table->lpars[i]->id = id; } else { VIR_WARN - ("Unable to read from information to local file."); + ("Unable to read from information from local file."); goto err; } rc = read(fd, uuid_table->lpars[i]->uuid, VIR_UUID_BUFLEN); if (rc != VIR_UUID_BUFLEN) { - VIR_WARN("Unable to read information to local file."); + VIR_WARN("Unable to read information from local file."); goto err; } } @@ -709,34 +689,22 @@ phypUUIDTable_Pull(virConnectPtr conn) ConnectionData *connection_data = conn->networkPrivateData; LIBSSH2_SESSION *session = connection_data->session; LIBSSH2_CHANNEL *channel = NULL; - virBuffer username = VIR_BUFFER_INITIALIZER; struct stat fileinfo; char buffer[1024]; int rc = 0; - int fd; + int fd = -1; int got = 0; int amount = 0; int total = 0; int sock = 0; char local_file[] = "./uuid_table"; char *remote_file = NULL; + int ret = -1; - if (conn->uri->user != NULL) { - virBufferAdd(&username, conn->uri->user, -1); - - if (virBufferError(&username)) { - virBufferFreeAndReset(&username); - virReportOOMError(); - goto err; - } - } - - if (virAsprintf - (&remote_file, "/home/%s/libvirt_uuid_table", - virBufferContentAndReset(&username)) - < 0) { + if (virAsprintf(&remote_file, "/home/%s/libvirt_uuid_table", + NULLSTR(conn->uri->user)) < 0) { virReportOOMError(); - goto err; + goto cleanup; } /* Trying to stat the remote file. */ @@ -746,12 +714,12 @@ phypUUIDTable_Pull(virConnectPtr conn) if (!channel) { if (libssh2_session_last_errno(session) != LIBSSH2_ERROR_EAGAIN) { - goto err; + goto cleanup; } else { if (waitsocket(sock, session) < 0 && errno != EINTR) { virReportSystemError(errno, "%s", _("unable to wait on libssh2 socket")); - goto err; + goto cleanup; } } } @@ -759,7 +727,7 @@ phypUUIDTable_Pull(virConnectPtr conn) /* Creating a new data base based on remote file */ if ((fd = creat(local_file, 0755)) == -1) - goto err; + goto cleanup; /* Request a file via SCP */ while (got < fileinfo.st_size) { @@ -790,7 +758,7 @@ phypUUIDTable_Pull(virConnectPtr conn) if (waitsocket(sock, session) < 0 && errno != EINTR) { virReportSystemError(errno, "%s", _("unable to wait on libssh2 socket")); - goto err; + goto cleanup; } continue; } @@ -799,20 +767,12 @@ phypUUIDTable_Pull(virConnectPtr conn) if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, _("Could not close %s"), local_file); - goto err; + goto cleanup; } - if (channel) { - libssh2_channel_send_eof(channel); - libssh2_channel_wait_eof(channel); - libssh2_channel_wait_closed(channel); - libssh2_channel_free(channel); - channel = NULL; - } - virBufferFreeAndReset(&username); - return 0; + ret = 0; -err: +cleanup: if (channel) { libssh2_channel_send_eof(channel); libssh2_channel_wait_eof(channel); @@ -820,7 +780,8 @@ err: libssh2_channel_free(channel); channel = NULL; } - return -1; + VIR_FORCE_CLOSE(fd); + return ret; } static int @@ -985,7 +946,7 @@ openSSHSession(virConnectPtr conn, virConnectAuthPtr auth, const char *hostname = conn->uri->server; char *username = NULL; char *password = NULL; - int sock; + int sock = -1; int rc; struct addrinfo *ai = NULL, *cur; struct addrinfo hints; @@ -1135,6 +1096,7 @@ disconnect: libssh2_session_disconnect(session, "Disconnecting..."); libssh2_session_free(session); err: + VIR_FORCE_CLOSE(sock); VIR_FREE(userhome); VIR_FREE(pubkey); VIR_FREE(pvtkey); @@ -1191,6 +1153,7 @@ phypOpen(virConnectPtr conn, virReportOOMError(); goto failure; } + connection_data->sock = -1; if (conn->uri->path) { /* need to shift one byte in order to remove the first "/" of URI component */ @@ -1227,6 +1190,7 @@ phypOpen(virConnectPtr conn, } connection_data->session = session; + connection_data->sock = internal_socket; uuid_table->nlpars = 0; uuid_table->lpars = NULL; @@ -1270,6 +1234,8 @@ failure: libssh2_session_free(session); } + if (connection_data) + VIR_FORCE_CLOSE(connection_data->sock); VIR_FREE(connection_data); return VIR_DRV_OPEN_ERROR; @@ -1289,6 +1255,8 @@ phypClose(virConnectPtr conn) phypUUIDTable_Free(phyp_driver->uuid_table); VIR_FREE(phyp_driver->managed_system); VIR_FREE(phyp_driver); + + VIR_FORCE_CLOSE(connection_data->sock); VIR_FREE(connection_data); return 0; } -- 1.7.11.7
On 01/10/2013 12:21 PM, John Ferlan wrote:
The phypUUIDTable_Push and phypUUIDTable_Pull leaked their file descriptors on normal return. Each function had an unnecessary use of creating a buffer to print conn->uri->user and needed a bit better flow control. I also noted that the Read function had a cut-n-paste error from the write function on a couple of VIR_WARN's.
The openSSHSession leaked the sock on the failure path. Additionally that turns into the internal_socket in the phypOpen code. That was neither saved nor closed on any path. So I used the connnection_data->sock field to save the socket for eventual close. Of interest here is that phypExec used the connection_data->sock field even though it had never been initialized. --- src/phyp/phyp_driver.c | 114 ++++++++++++++++++------------------------------- 1 file changed, 41 insertions(+), 73 deletions(-)
+ if (virAsprintf(&remote_file, "/home/%s/libvirt_uuid_table", + NULLSTR(conn->uri->user)) < 0) {
Indentation - NULLSTR is an argument to virAsprintf, not another condition of the if. (twice) ACK with this squashed in, so I pushed. (If you use emacs and want to know how to automatically update copyright years on any file you touch, I could propose a patch to HACKING to share that tip.) diff --git i/src/phyp/phyp_driver.c w/src/phyp/phyp_driver.c index f6c2579..74f04ff 100644 --- i/src/phyp/phyp_driver.c +++ w/src/phyp/phyp_driver.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2012 Red Hat, Inc. + * Copyright (C) 2010-2013 Red Hat, Inc. * Copyright IBM Corp. 2009 * * phyp_driver.c: ssh layer to access Power Hypervisors @@ -504,7 +504,7 @@ phypUUIDTable_Push(virConnectPtr conn) int ret = -1; if (virAsprintf(&remote_file, "/home/%s/libvirt_uuid_table", - NULLSTR(conn->uri->user)) < 0) { + NULLSTR(conn->uri->user)) < 0) { virReportOOMError(); goto cleanup; } @@ -702,7 +702,7 @@ phypUUIDTable_Pull(virConnectPtr conn) int ret = -1; if (virAsprintf(&remote_file, "/home/%s/libvirt_uuid_table", - NULLSTR(conn->uri->user)) < 0) { + NULLSTR(conn->uri->user)) < 0) { virReportOOMError(); goto cleanup; } -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Because result was used to determine whether or not to free 'priv' resources Coverity tagged the code as having a resource leak. This change addresses that concern. --- src/esx/esx_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1366c81..9befa38 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -998,6 +998,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, virReportOOMError(); goto cleanup; } + conn->privateData = priv; if (esxUtil_ParseUri(&priv->parsedUri, conn->uri) < 0) { goto cleanup; @@ -1008,8 +1009,6 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, priv->supportsLongMode = esxVI_Boolean_Undefined; priv->usedCpuTimeCounterId = -1; - conn->privateData = priv; - /* * Set the port dependent on the transport protocol if no port is * specified. This allows us to rely on the port parameter being @@ -1104,9 +1103,10 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, result = VIR_DRV_OPEN_SUCCESS; cleanup: - if (result == VIR_DRV_OPEN_ERROR) { + if (result == VIR_DRV_OPEN_ERROR) + conn->privateData = NULL; + if (priv && !conn->privateData) esxFreePrivate(&priv); - } VIR_FREE(potentialVCenterIpAddress); -- 1.7.11.7
On 01/09/2013 07:54 AM, John Ferlan wrote:
Because result was used to determine whether or not to free 'priv' resources Coverity tagged the code as having a resource leak. This change addresses that concern. --- src/esx/esx_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1366c81..9befa38 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -998,6 +998,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, virReportOOMError(); goto cleanup; } + conn->privateData = priv;
if (esxUtil_ParseUri(&priv->parsedUri, conn->uri) < 0) { goto cleanup;
I
@@ -1008,8 +1009,6 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, priv->supportsLongMode = esxVI_Boolean_Undefined; priv->usedCpuTimeCounterId = -1;
- conn->privateData = priv; - /* * Set the port dependent on the transport protocol if no port is * specified. This allows us to rely on the port parameter being @@ -1104,9 +1103,10 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, result = VIR_DRV_OPEN_SUCCESS;
cleanup: - if (result == VIR_DRV_OPEN_ERROR) { + if (result == VIR_DRV_OPEN_ERROR) + conn->privateData = NULL; + if (priv && !conn->privateData) esxFreePrivate(&priv); - }
This feels a bit complex; I had to go read esxFreePrivate() to make sure it would behave if called on a partial object. Would it be any easier to delay the assignment to conn->privateData, and use transfer of ownership semantics, so that the cleanup is unconditional? conn->privateData = priv; priv = NULL; result = VIR_DRV_OPEN_SUCCESS; cleanup: esxFreePrivate(&priv); -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
2013/1/10 Eric Blake <eblake@redhat.com>:
On 01/09/2013 07:54 AM, John Ferlan wrote:
Because result was used to determine whether or not to free 'priv' resources Coverity tagged the code as having a resource leak. This change addresses that concern. --- src/esx/esx_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1366c81..9befa38 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -998,6 +998,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, virReportOOMError(); goto cleanup; } + conn->privateData = priv;
if (esxUtil_ParseUri(&priv->parsedUri, conn->uri) < 0) { goto cleanup;
I
@@ -1008,8 +1009,6 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, priv->supportsLongMode = esxVI_Boolean_Undefined; priv->usedCpuTimeCounterId = -1;
- conn->privateData = priv; - /* * Set the port dependent on the transport protocol if no port is * specified. This allows us to rely on the port parameter being @@ -1104,9 +1103,10 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, result = VIR_DRV_OPEN_SUCCESS;
cleanup: - if (result == VIR_DRV_OPEN_ERROR) { + if (result == VIR_DRV_OPEN_ERROR) + conn->privateData = NULL; + if (priv && !conn->privateData) esxFreePrivate(&priv); - }
This feels a bit complex;
I agree.
I had to go read esxFreePrivate() to make sure it would behave if called on a partial object. Would it be any easier to delay the assignment to conn->privateData, and use transfer of ownership semantics, so that the cleanup is unconditional?
conn->privateData = priv; priv = NULL; result = VIR_DRV_OPEN_SUCCESS; cleanup: esxFreePrivate(&priv);
No! This cannot be done that easily, see commit b126715a48cd0cbe32ec6468c267cd8cf2961c55 "esx: Fix segfault in esxConnectToHost". We cannot move conn->privateData = priv; further down with the current code, because it is already used in between. But I like the simplicity of your suggestion. I'll me come up with a patch for this later today. -- Matthias Bolte http://photron.blogspot.com
On 01/10/2013 10:49 AM, Matthias Bolte wrote:
2013/1/10 Eric Blake <eblake@redhat.com>:
On 01/09/2013 07:54 AM, John Ferlan wrote:
Because result was used to determine whether or not to free 'priv' resources Coverity tagged the code as having a resource leak. This change addresses that concern. --- src/esx/esx_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1366c81..9befa38 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -998,6 +998,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, virReportOOMError(); goto cleanup; } + conn->privateData = priv;
if (esxUtil_ParseUri(&priv->parsedUri, conn->uri) < 0) { goto cleanup;
I
@@ -1008,8 +1009,6 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, priv->supportsLongMode = esxVI_Boolean_Undefined; priv->usedCpuTimeCounterId = -1;
- conn->privateData = priv; - /* * Set the port dependent on the transport protocol if no port is * specified. This allows us to rely on the port parameter being @@ -1104,9 +1103,10 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, result = VIR_DRV_OPEN_SUCCESS;
cleanup: - if (result == VIR_DRV_OPEN_ERROR) { + if (result == VIR_DRV_OPEN_ERROR) + conn->privateData = NULL; + if (priv && !conn->privateData) esxFreePrivate(&priv); - }
This feels a bit complex;
I agree.
+1... for a reason though...
I had to go read esxFreePrivate() to make sure it would behave if called on a partial object. Would it be any easier to delay the assignment to conn->privateData, and use transfer of ownership semantics, so that the cleanup is unconditional?
conn->privateData = priv; priv = NULL; result = VIR_DRV_OPEN_SUCCESS; cleanup: esxFreePrivate(&priv);
No! This cannot be done that easily, see commit b126715a48cd0cbe32ec6468c267cd8cf2961c55 "esx: Fix segfault in esxConnectToHost".
And this is why... It is a "false positive" and possibly could be dealt with by adding a /* coverity[leaked_storage] */ in the right place as well. Although I've had some issues trying to do that for another set of changes because the going out of scope happens on the return and putting a coverity commit prior to a return hasn't done what I'd expect.
We cannot move conn->privateData = priv; further down with the current code, because it is already used in between. But I like the simplicity of your suggestion.
I'll me come up with a patch for this later today.
2013/1/10 John Ferlan <jferlan@redhat.com>:
On 01/10/2013 10:49 AM, Matthias Bolte wrote:
2013/1/10 Eric Blake <eblake@redhat.com>:
On 01/09/2013 07:54 AM, John Ferlan wrote:
Because result was used to determine whether or not to free 'priv' resources Coverity tagged the code as having a resource leak. This change addresses that concern. --- src/esx/esx_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1366c81..9befa38 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -998,6 +998,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, virReportOOMError(); goto cleanup; } + conn->privateData = priv;
if (esxUtil_ParseUri(&priv->parsedUri, conn->uri) < 0) { goto cleanup;
I
@@ -1008,8 +1009,6 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, priv->supportsLongMode = esxVI_Boolean_Undefined; priv->usedCpuTimeCounterId = -1;
- conn->privateData = priv; - /* * Set the port dependent on the transport protocol if no port is * specified. This allows us to rely on the port parameter being @@ -1104,9 +1103,10 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, result = VIR_DRV_OPEN_SUCCESS;
cleanup: - if (result == VIR_DRV_OPEN_ERROR) { + if (result == VIR_DRV_OPEN_ERROR) + conn->privateData = NULL; + if (priv && !conn->privateData) esxFreePrivate(&priv); - }
This feels a bit complex;
I agree.
+1... for a reason though...
I had to go read esxFreePrivate() to make sure it would behave if called on a partial object. Would it be any easier to delay the assignment to conn->privateData, and use transfer of ownership semantics, so that the cleanup is unconditional?
conn->privateData = priv; priv = NULL; result = VIR_DRV_OPEN_SUCCESS; cleanup: esxFreePrivate(&priv);
No! This cannot be done that easily, see commit b126715a48cd0cbe32ec6468c267cd8cf2961c55 "esx: Fix segfault in esxConnectToHost".
And this is why... It is a "false positive" and possibly could be dealt with by adding a /* coverity[leaked_storage] */ in the right place as well. Although I've had some issues trying to do that for another set of changes because the going out of scope happens on the return and putting a coverity commit prior to a return hasn't done what I'd expect.
Well, you're commit message didn't state this as a false positive, so I looked carefully at esxOpen whether or not Coverity was right here, but couldn't find anything. Anyway, I'd like to simplify the logic in esxOpen a bit the way Eric suggested. Here's a patch that does this: https://www.redhat.com/archives/libvir-list/2013-January/msg00657.html -- Matthias Bolte http://photron.blogspot.com
On 01/10/2013 04:55 PM, Matthias Bolte wrote:
2013/1/10 John Ferlan <jferlan@redhat.com>:
On 01/10/2013 10:49 AM, Matthias Bolte wrote:
2013/1/10 Eric Blake <eblake@redhat.com>:
On 01/09/2013 07:54 AM, John Ferlan wrote:
Because result was used to determine whether or not to free 'priv' resources Coverity tagged the code as having a resource leak. This change addresses that concern. --- src/esx/esx_driver.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/esx/esx_driver.c b/src/esx/esx_driver.c index 1366c81..9befa38 100644 --- a/src/esx/esx_driver.c +++ b/src/esx/esx_driver.c @@ -998,6 +998,7 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, virReportOOMError(); goto cleanup; } + conn->privateData = priv;
if (esxUtil_ParseUri(&priv->parsedUri, conn->uri) < 0) { goto cleanup;
I
@@ -1008,8 +1009,6 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, priv->supportsLongMode = esxVI_Boolean_Undefined; priv->usedCpuTimeCounterId = -1;
- conn->privateData = priv; - /* * Set the port dependent on the transport protocol if no port is * specified. This allows us to rely on the port parameter being @@ -1104,9 +1103,10 @@ esxOpen(virConnectPtr conn, virConnectAuthPtr auth, result = VIR_DRV_OPEN_SUCCESS;
cleanup: - if (result == VIR_DRV_OPEN_ERROR) { + if (result == VIR_DRV_OPEN_ERROR) + conn->privateData = NULL; + if (priv && !conn->privateData) esxFreePrivate(&priv); - }
This feels a bit complex;
I agree.
+1... for a reason though...
I had to go read esxFreePrivate() to make sure it would behave if called on a partial object. Would it be any easier to delay the assignment to conn->privateData, and use transfer of ownership semantics, so that the cleanup is unconditional?
conn->privateData = priv; priv = NULL; result = VIR_DRV_OPEN_SUCCESS; cleanup: esxFreePrivate(&priv);
No! This cannot be done that easily, see commit b126715a48cd0cbe32ec6468c267cd8cf2961c55 "esx: Fix segfault in esxConnectToHost".
And this is why... It is a "false positive" and possibly could be dealt with by adding a /* coverity[leaked_storage] */ in the right place as well. Although I've had some issues trying to do that for another set of changes because the going out of scope happens on the return and putting a coverity commit prior to a return hasn't done what I'd expect.
Well, you're commit message didn't state this as a false positive, so I looked carefully at esxOpen whether or not Coverity was right here, but couldn't find anything.
Sorry- I had put that in the cover letter
Anyway, I'd like to simplify the logic in esxOpen a bit the way Eric suggested. Here's a patch that does this:
https://www.redhat.com/archives/libvir-list/2013-January/msg00657.html
--- src/storage/storage_backend_rbd.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index f5c6b0f..f916de1 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -329,16 +329,23 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, goto out_of_memory; vol->name = strdup(name); - if (vol->name == NULL) + if (vol->name == NULL) { + VIR_FREE(vol); goto out_of_memory; + } - if (STREQ(vol->name, "")) + if (STREQ(vol->name, "")) { + VIR_FREE(vol->name); + VIR_FREE(vol); break; + } name += strlen(name) + 1; - if (volStorageBackendRBDRefreshVolInfo(vol, pool, ptr) < 0) + if (volStorageBackendRBDRefreshVolInfo(vol, pool, ptr) < 0) { + virStorageVolDefFree(vol); goto cleanup; + } pool->volumes.objs[pool->volumes.count++] = vol; } -- 1.7.11.7
On 01/09/2013 07:54 AM, John Ferlan wrote:
--- src/storage/storage_backend_rbd.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)
vol->name = strdup(name); - if (vol->name == NULL) + if (vol->name == NULL) { + VIR_FREE(vol); goto out_of_memory; + }
- if (STREQ(vol->name, "")) + if (STREQ(vol->name, "")) { + VIR_FREE(vol->name); + VIR_FREE(vol); break; + }
It feels a bit awkward having to free more and more for each break. I rearranged the code slightly to hoist the validation prior to the malloc. ACK and pushed with this squashed in: diff --git i/src/storage/storage_backend_rbd.c w/src/storage/storage_backend_rbd.c index f916de1..8a0e517 100644 --- i/src/storage/storage_backend_rbd.c +++ w/src/storage/storage_backend_rbd.c @@ -1,6 +1,7 @@ /* * storage_backend_rbd.c: storage backend for RBD (RADOS Block Device) handling * + * Copyright (C) 2013 Red Hat, Inc. * Copyright (C) 2012 Wido den Hollander * * This library is free software; you can redistribute it and/or @@ -319,12 +320,16 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, } for (i = 0, name = names; name < names + max_size; i++) { + virStorageVolDefPtr vol; + if (VIR_REALLOC_N(pool->volumes.objs, pool->volumes.count + 1) < 0) { virStoragePoolObjClearVols(pool); goto out_of_memory; } - virStorageVolDefPtr vol; + if (STREQ(name, "")) + break; + if (VIR_ALLOC(vol) < 0) goto out_of_memory; @@ -334,12 +339,6 @@ static int virStorageBackendRBDRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, goto out_of_memory; } - if (STREQ(vol->name, "")) { - VIR_FREE(vol->name); - VIR_FREE(vol); - break; - } - name += strlen(name) + 1; if (volStorageBackendRBDRefreshVolInfo(vol, pool, ptr) < 0) { -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- src/util/virlockspace.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virlockspace.c b/src/util/virlockspace.c index 9ada6a6..04aeebe 100644 --- a/src/util/virlockspace.c +++ b/src/util/virlockspace.c @@ -387,6 +387,7 @@ virLockSpacePtr virLockSpaceNewPostExecRestart(virJSONValuePtr object) if (virSetInherit(res->fd, false) < 0) { virReportSystemError(errno, "%s", _("Cannot enable close-on-exec flag")); + virLockSpaceResourceFree(res); goto error; } if (virJSONValueObjectGetBoolean(child, "lockHeld", &res->lockHeld) < 0) { -- 1.7.11.7
--- src/locking/lock_driver_sanlock.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index d06fa66..df6bee9 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -235,6 +235,7 @@ static int virLockManagerSanlockSetupLockspace(void) path); goto error; } + VIR_FREE(dir); if (driver->group != (gid_t) -1) perms |= 0060; -- 1.7.11.7
On 01/09/13 15:54, John Ferlan wrote:
--- src/locking/lock_driver_sanlock.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index d06fa66..df6bee9 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -235,6 +235,7 @@ static int virLockManagerSanlockSetupLockspace(void) path); goto error; } + VIR_FREE(dir);
if (driver->group != (gid_t) -1) perms |= 0060;
That does fix the 'dir' leak, but it seems we still leak 'path' on non-error path this way.
Both 'dir' and 'path' were not free'd on successful return --- src/locking/lock_driver_sanlock.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index d06fa66..0b7c6d5 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -365,6 +365,8 @@ retry: VIR_DEBUG("Lockspace %s has been registered", path); } + VIR_FREE(path); + VIR_FREE(dir); return 0; error_unlink: -- 1.7.11.7
On 01/15/13 15:23, John Ferlan wrote:
Both 'dir' and 'path' were not free'd on successful return --- src/locking/lock_driver_sanlock.c | 2 ++ 1 file changed, 2 insertions(+)
ACK and pushed.
--- src/rpc/virnetserverservice.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rpc/virnetserverservice.c b/src/rpc/virnetserverservice.c index 61dd682..5deacaa 100644 --- a/src/rpc/virnetserverservice.c +++ b/src/rpc/virnetserverservice.c @@ -356,9 +356,6 @@ virJSONValuePtr virNetServerServicePreExecRestart(virNetServerServicePtr svc) if (!object) return NULL; - if (!(socks = virJSONValueNewArray())) - goto error; - if (virJSONValueObjectAppendNumberInt(object, "auth", svc->auth) < 0) goto error; if (virJSONValueObjectAppendBoolean(object, "readonly", svc->readonly) < 0) @@ -366,6 +363,9 @@ virJSONValuePtr virNetServerServicePreExecRestart(virNetServerServicePtr svc) if (virJSONValueObjectAppendNumberUint(object, "nrequests_client_max", svc->nrequests_client_max) < 0) goto error; + if (!(socks = virJSONValueNewArray())) + goto error; + if (virJSONValueObjectAppend(object, "socks", socks) < 0) { virJSONValueFree(socks); goto error; -- 1.7.11.7
If there was more than one inotify_event found in the read/while loop, then only the last event found would have been queued. --- src/uml/uml_driver.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 448d292..db6e466 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -410,11 +410,13 @@ reread: } if (dom) virDomainObjUnlock(dom); + if (event) { + umlDomainEventQueue(driver, event); + event = NULL; + } } cleanup: - if (event) - umlDomainEventQueue(driver, event); umlDriverUnlock(driver); } -- 1.7.11.7
--- src/test/test_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 3e082a4..0e1d90c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4687,6 +4687,7 @@ testStoragePoolListAllVolumes(virStoragePoolPtr obj, if (tmp_vols[i]) virStorageVolFree(tmp_vols[i]); } + VIR_FREE(tmp_vols); } if (pool) -- 1.7.11.7
--- src/storage/storage_driver.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index ff56f4f..e98c18c 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1197,6 +1197,7 @@ storagePoolListAllVolumes(virStoragePoolPtr pool, if (tmp_vols[i]) virStorageVolFree(tmp_vols[i]); } + VIR_FREE(tmp_vols); } if (obj) -- 1.7.11.7
--- src/interface/interface_backend_netcf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c index 35335c2..8671717 100644 --- a/src/interface/interface_backend_netcf.c +++ b/src/interface/interface_backend_netcf.c @@ -399,6 +399,7 @@ cleanup: if (tmp_iface_objs[i]) virInterfaceFree(tmp_iface_objs[i]); } + VIR_FREE(tmp_iface_objs); } interfaceDriverUnlock(driver); -- 1.7.11.7
Make cpuset local to the while loop and free it once done with it each time through the loop. --- src/xen/xend_internal.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 84a25e8..c6b800b 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1113,7 +1113,6 @@ sexpr_to_xend_topology(const struct sexpr *root, { const char *nodeToCpu; const char *cur; - virBitmapPtr cpuset = NULL; int *cpuNums = NULL; int cell, cpu, nb_cpus; int n = 0; @@ -1131,6 +1130,7 @@ sexpr_to_xend_topology(const struct sexpr *root, cur = nodeToCpu; while (*cur != 0) { + virBitmapPtr cpuset = NULL; /* * Find the next NUMA cell described in the xend output */ @@ -1152,8 +1152,10 @@ sexpr_to_xend_topology(const struct sexpr *root, goto memory_error; } else { nb_cpus = virBitmapParse(cur, 'n', &cpuset, numCpus); - if (nb_cpus < 0) + if (nb_cpus < 0) { + virBitmapFree(cpuset); goto error; + } } for (n = 0, cpu = 0; cpu < numCpus; cpu++) { @@ -1163,28 +1165,26 @@ sexpr_to_xend_topology(const struct sexpr *root, if (used) cpuNums[n++] = cpu; } + virBitmapFree(cpuset); if (virCapabilitiesAddHostNUMACell(caps, cell, nb_cpus, cpuNums) < 0) goto memory_error; + } VIR_FREE(cpuNums); - virBitmapFree(cpuset); return 0; parse_error: virReportError(VIR_ERR_XEN_CALL, "%s", _("topology syntax error")); error: VIR_FREE(cpuNums); - virBitmapFree(cpuset); - return -1; memory_error: VIR_FREE(cpuNums); - virBitmapFree(cpuset); virReportOOMError(); return -1; } -- 1.7.11.7
(you duplicated "resource" in the subject line) On 01/09/2013 09:54 AM, John Ferlan wrote:
Make cpuset local to the while loop and free it once done with it each time through the loop. --- src/xen/xend_internal.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 84a25e8..c6b800b 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1113,7 +1113,6 @@ sexpr_to_xend_topology(const struct sexpr *root, { const char *nodeToCpu; const char *cur; - virBitmapPtr cpuset = NULL; int *cpuNums = NULL; int cell, cpu, nb_cpus; int n = 0; @@ -1131,6 +1130,7 @@ sexpr_to_xend_topology(const struct sexpr *root,
cur = nodeToCpu; while (*cur != 0) { + virBitmapPtr cpuset = NULL; /* * Find the next NUMA cell described in the xend output */ @@ -1152,8 +1152,10 @@ sexpr_to_xend_topology(const struct sexpr *root, goto memory_error; } else { nb_cpus = virBitmapParse(cur, 'n', &cpuset, numCpus); - if (nb_cpus < 0) + if (nb_cpus < 0) { + virBitmapFree(cpuset);
This virBitmapFree() isn't necessary - virBitmapParse is guaranteed to have nothing allocated (and will set cpuset = NULL) if it fails.
goto error; + } }
for (n = 0, cpu = 0; cpu < numCpus; cpu++) { @@ -1163,28 +1165,26 @@ sexpr_to_xend_topology(const struct sexpr *root, if (used) cpuNums[n++] = cpu; } + virBitmapFree(cpuset);
if (virCapabilitiesAddHostNUMACell(caps, cell, nb_cpus, cpuNums) < 0) goto memory_error; + } VIR_FREE(cpuNums); - virBitmapFree(cpuset); return 0;
parse_error: virReportError(VIR_ERR_XEN_CALL, "%s", _("topology syntax error")); error: VIR_FREE(cpuNums); - virBitmapFree(cpuset); - return -1;
memory_error: VIR_FREE(cpuNums); - virBitmapFree(cpuset); virReportOOMError(); return -1; }
ACK with the above nits fixed.
On 01/09/2013 11:55 AM, Laine Stump wrote:
(you duplicated "resource" in the subject line)
Missed that one... Will fix.
On 01/09/2013 09:54 AM, John Ferlan wrote:
Make cpuset local to the while loop and free it once done with it each time through the loop. --- src/xen/xend_internal.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 84a25e8..c6b800b 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1113,7 +1113,6 @@ sexpr_to_xend_topology(const struct sexpr *root, { const char *nodeToCpu; const char *cur; - virBitmapPtr cpuset = NULL; int *cpuNums = NULL; int cell, cpu, nb_cpus; int n = 0; @@ -1131,6 +1130,7 @@ sexpr_to_xend_topology(const struct sexpr *root,
cur = nodeToCpu; while (*cur != 0) { + virBitmapPtr cpuset = NULL; /* * Find the next NUMA cell described in the xend output */ @@ -1152,8 +1152,10 @@ sexpr_to_xend_topology(const struct sexpr *root, goto memory_error; } else { nb_cpus = virBitmapParse(cur, 'n', &cpuset, numCpus); - if (nb_cpus < 0) + if (nb_cpus < 0) { + virBitmapFree(cpuset);
This virBitmapFree() isn't necessary - virBitmapParse is guaranteed to have nothing allocated (and will set cpuset = NULL) if it fails.
According to Coverity's analysis this may not be true since it's "possible" to hit the "ret--" line (more than once) in virBitmapParse() while hitting either "ret++" line less times returning a negative value on the "success" path. The example Coverity had shows 6 passes through the loop, 4 negatives, 1 positive, and 1 nothing. Whether realistically this could be true, I am not sure. How Coverity determined what the value of 'cpuSet' is a mystery as the output I have doesn't show what's being used for parsing, just that we go through the loop 6 times. Perhaps something like "^1,^2,^3,4,^5,^6" where 1,2,3,4,5,6 pass the virBitmapIsSet() call changing the 'ret' value to -3.
goto error; + } }
for (n = 0, cpu = 0; cpu < numCpus; cpu++) { @@ -1163,28 +1165,26 @@ sexpr_to_xend_topology(const struct sexpr *root, if (used) cpuNums[n++] = cpu; } + virBitmapFree(cpuset);
if (virCapabilitiesAddHostNUMACell(caps, cell, nb_cpus, cpuNums) < 0) goto memory_error; + } VIR_FREE(cpuNums); - virBitmapFree(cpuset); return 0;
parse_error: virReportError(VIR_ERR_XEN_CALL, "%s", _("topology syntax error")); error: VIR_FREE(cpuNums); - virBitmapFree(cpuset); - return -1;
memory_error: VIR_FREE(cpuNums); - virBitmapFree(cpuset); virReportOOMError(); return -1; }
ACK with the above nits fixed.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 01/09/2013 01:50 PM, John Ferlan wrote:
On 01/09/2013 11:55 AM, Laine Stump wrote:
(you duplicated "resource" in the subject line)
Missed that one... Will fix.
On 01/09/2013 09:54 AM, John Ferlan wrote:
Make cpuset local to the while loop and free it once done with it each time through the loop. --- src/xen/xend_internal.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 84a25e8..c6b800b 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1113,7 +1113,6 @@ sexpr_to_xend_topology(const struct sexpr *root, { const char *nodeToCpu; const char *cur; - virBitmapPtr cpuset = NULL; int *cpuNums = NULL; int cell, cpu, nb_cpus; int n = 0; @@ -1131,6 +1130,7 @@ sexpr_to_xend_topology(const struct sexpr *root,
cur = nodeToCpu; while (*cur != 0) { + virBitmapPtr cpuset = NULL; /* * Find the next NUMA cell described in the xend output */ @@ -1152,8 +1152,10 @@ sexpr_to_xend_topology(const struct sexpr *root, goto memory_error; } else { nb_cpus = virBitmapParse(cur, 'n', &cpuset, numCpus); - if (nb_cpus < 0) + if (nb_cpus < 0) { + virBitmapFree(cpuset); This virBitmapFree() isn't necessary - virBitmapParse is guaranteed to have nothing allocated (and will set cpuset = NULL) if it fails.
According to Coverity's analysis this may not be true since it's "possible" to hit the "ret--" line (more than once) in virBitmapParse() while hitting either "ret++" line less times returning a negative value on the "success" path. The example Coverity had shows 6 passes through the loop, 4 negatives, 1 positive, and 1 nothing.
Whether realistically this could be true, I am not sure.
How Coverity determined what the value of 'cpuSet' is a mystery as the output I have doesn't show what's being used for parsing, just that we go through the loop 6 times. Perhaps something like "^1,^2,^3,4,^5,^6" where 1,2,3,4,5,6 pass the virBitmapIsSet() call changing the 'ret' value to -3.
I don't think that is possible. In order for virBitmapIsSet() to return true for a particular bit, that bit must be set, and in order for that bit to be set, it must have been set in a previous iteration of this same loop (remember that the bitmap is initialized to all empty at the top of the function), which means that ret++ must have been executed. So ret-- can't happen without a previous corresponding ret++, therefore the value of ret can't be < 0. If it was possible to have a return < 0 on success, that would be a bug in the function that would need to be fixed.
On 01/09/2013 07:07 PM, Laine Stump wrote:
According to Coverity's analysis this may not be true since it's "possible" to hit the "ret--" line (more than once) in virBitmapParse() while hitting either "ret++" line less times returning a negative value on the "success" path. The example Coverity had shows 6 passes through the loop, 4 negatives, 1 positive, and 1 nothing.
Whether realistically this could be true, I am not sure.
How Coverity determined what the value of 'cpuSet' is a mystery as the output I have doesn't show what's being used for parsing, just that we go through the loop 6 times. Perhaps something like "^1,^2,^3,4,^5,^6" where 1,2,3,4,5,6 pass the virBitmapIsSet() call changing the 'ret' value to -3.
Except that we would have rejected '^1' outright up front.
I don't think that is possible. In order for virBitmapIsSet() to return true for a particular bit, that bit must be set, and in order for that bit to be set, it must have been set in a previous iteration of this same loop (remember that the bitmap is initialized to all empty at the top of the function), which means that ret++ must have been executed. So ret-- can't happen without a previous corresponding ret++, therefore the value of ret can't be < 0.
Maybe a well-placed: sa_assert(ret >= 0); will help Coverity learn a bit more of the logic flow, which proves that we cannot reach the success path with too many ret-- lines.
If it was possible to have a return < 0 on success, that would be a bug in the function that would need to be fixed.
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Make cpuset local to the while loop and free it once done with it each time through the loop. Add a sa_assert() to virBitmapParse() to keep Coverity from believing there could be a negative return and possible resource leak. --- src/util/virbitmap.c | 1 + src/xen/xend_internal.c | 8 +++----- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index e032374..ca82d1b 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -373,6 +373,7 @@ int virBitmapParse(const char *str, } } + sa_assert(ret >= 0); return ret; parse_error: diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 84a25e8..445d336 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1113,7 +1113,6 @@ sexpr_to_xend_topology(const struct sexpr *root, { const char *nodeToCpu; const char *cur; - virBitmapPtr cpuset = NULL; int *cpuNums = NULL; int cell, cpu, nb_cpus; int n = 0; @@ -1131,6 +1130,7 @@ sexpr_to_xend_topology(const struct sexpr *root, cur = nodeToCpu; while (*cur != 0) { + virBitmapPtr cpuset = NULL; /* * Find the next NUMA cell described in the xend output */ @@ -1163,28 +1163,26 @@ sexpr_to_xend_topology(const struct sexpr *root, if (used) cpuNums[n++] = cpu; } + virBitmapFree(cpuset); if (virCapabilitiesAddHostNUMACell(caps, cell, nb_cpus, cpuNums) < 0) goto memory_error; + } VIR_FREE(cpuNums); - virBitmapFree(cpuset); return 0; parse_error: virReportError(VIR_ERR_XEN_CALL, "%s", _("topology syntax error")); error: VIR_FREE(cpuNums); - virBitmapFree(cpuset); - return -1; memory_error: VIR_FREE(cpuNums); - virBitmapFree(cpuset); virReportOOMError(); return -1; } -- 1.7.11.7
On 01/10/2013 08:42 AM, John Ferlan wrote:
Make cpuset local to the while loop and free it once done with it each time through the loop. Add a sa_assert() to virBitmapParse() to keep Coverity from believing there could be a negative return and possible resource leak. --- src/util/virbitmap.c | 1 + src/xen/xend_internal.c | 8 +++----- 2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index e032374..ca82d1b 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -373,6 +373,7 @@ int virBitmapParse(const char *str, } }
+ sa_assert(ret >= 0); return ret;
parse_error: diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 84a25e8..445d336 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1113,7 +1113,6 @@ sexpr_to_xend_topology(const struct sexpr *root, { const char *nodeToCpu; const char *cur; - virBitmapPtr cpuset = NULL; int *cpuNums = NULL; int cell, cpu, nb_cpus; int n = 0; @@ -1131,6 +1130,7 @@ sexpr_to_xend_topology(const struct sexpr *root,
cur = nodeToCpu; while (*cur != 0) { + virBitmapPtr cpuset = NULL; /* * Find the next NUMA cell described in the xend output */ @@ -1163,28 +1163,26 @@ sexpr_to_xend_topology(const struct sexpr *root, if (used) cpuNums[n++] = cpu; } + virBitmapFree(cpuset);
if (virCapabilitiesAddHostNUMACell(caps, cell, nb_cpus, cpuNums) < 0) goto memory_error; +
Seeing this spurious addition of whitespace (which you may have added to make it more obvious that it wasn't closing the immediately preceding if() clause) made me notice that the preceding if() was unnecessarily split onto multiple lines. Can either you, or whoever pushes this patch, take this chance to combine the multiple lines of if (virCapabilitiesAddHostNUMACell(....) < 0) onto a single line? (it easily fits in 80 columns).
} VIR_FREE(cpuNums); - virBitmapFree(cpuset); return 0;
parse_error: virReportError(VIR_ERR_XEN_CALL, "%s", _("topology syntax error")); error: VIR_FREE(cpuNums); - virBitmapFree(cpuset); - return -1;
memory_error: VIR_FREE(cpuNums); - virBitmapFree(cpuset); virReportOOMError(); return -1; }
ACK (with or without the above suggestion).
Make cpuset local to the while loop and free it once done with it each time through the loop. Add a sa_assert() to virBitmapParse() to keep Coverity from believing there could be a negative return and possible resource leak. --- src/util/virbitmap.c | 1 + src/xen/xend_internal.c | 12 +++--------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index e032374..ca82d1b 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -373,6 +373,7 @@ int virBitmapParse(const char *str, } } + sa_assert(ret >= 0); return ret; parse_error: diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 84a25e8..959225c 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1113,7 +1113,6 @@ sexpr_to_xend_topology(const struct sexpr *root, { const char *nodeToCpu; const char *cur; - virBitmapPtr cpuset = NULL; int *cpuNums = NULL; int cell, cpu, nb_cpus; int n = 0; @@ -1131,6 +1130,7 @@ sexpr_to_xend_topology(const struct sexpr *root, cur = nodeToCpu; while (*cur != 0) { + virBitmapPtr cpuset = NULL; /* * Find the next NUMA cell described in the xend output */ @@ -1163,28 +1163,22 @@ sexpr_to_xend_topology(const struct sexpr *root, if (used) cpuNums[n++] = cpu; } + virBitmapFree(cpuset); - if (virCapabilitiesAddHostNUMACell(caps, - cell, - nb_cpus, - cpuNums) < 0) + if (virCapabilitiesAddHostNUMACell(caps, cell, nb_cpus, cpuNums) < 0) goto memory_error; } VIR_FREE(cpuNums); - virBitmapFree(cpuset); return 0; parse_error: virReportError(VIR_ERR_XEN_CALL, "%s", _("topology syntax error")); error: VIR_FREE(cpuNums); - virBitmapFree(cpuset); - return -1; memory_error: VIR_FREE(cpuNums); - virBitmapFree(cpuset); virReportOOMError(); return -1; } -- 1.7.11.7
Be sure to VIR_FREE(accel) and moved virDomainVideoDefFree() within no_memory label to be consistent Resolve resource leak in parallelsApplyIfaceParams() when the 'oldnet' is allocated locally. --- src/parallels/parallels_driver.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 6f33080..bf27e54 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -304,8 +304,9 @@ parallelsAddVideoInfo(virDomainDefPtr def, virJSONValuePtr value) no_memory: virReportOOMError(); -cleanup: + VIR_FREE(accel); virDomainVideoDefFree(video); +cleanup: return -1; } @@ -1812,58 +1813,58 @@ static int parallelsApplyIfaceParams(parallelsDomObjPtr pdom, if (!create && oldnet->type != newnet->type) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Changing network type is not supported")); - return -1; + goto error; } if (!STREQ_NULLABLE(oldnet->model, newnet->model)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Changing network device model is not supported")); - return -1; + goto error; } if (!STREQ_NULLABLE(oldnet->data.network.portgroup, newnet->data.network.portgroup)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Changing network portgroup is not supported")); - return -1; + goto error; } if (!virNetDevVPortProfileEqual(oldnet->virtPortProfile, newnet->virtPortProfile)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Changing virtual port profile is not supported")); - return -1; + goto error; } if (newnet->tune.sndbuf_specified) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Setting send buffer size is not supported")); - return -1; + goto error; } if (!STREQ_NULLABLE(oldnet->script, newnet->script)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Setting startup script is not supported")); - return -1; + goto error; } if (!STREQ_NULLABLE(oldnet->filter, newnet->filter)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Changing filter params is not supported")); - return -1; + goto error; } if (newnet->bandwidth != NULL) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Setting bandwidth params is not supported")); - return -1; + goto error; } for (i = 0; i < sizeof(newnet->vlan); i++) { if (((char *)&newnet->vlan)[i] != 0) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Setting vlan params is not supported")); - return -1; + goto error; } } @@ -1905,15 +1906,22 @@ static int parallelsApplyIfaceParams(parallelsDomObjPtr pdom, is_changed = true; } + if (create) + VIR_FREE(oldnet); + if (!create && !is_changed) { /* nothing changed - no need to run prlctl */ return 0; } if (virCommandRun(cmd, NULL)) - return -1; + goto error; return 0; +error: + if (create) + VIR_FREE(oldnet); + return -1; } static int -- 1.7.11.7
Commit-id 'afc4631b' added the regfree(reg) to free resources alloc'd during regcomp; however, reg still needed to be VIR_FREE()'d. --- src/storage/storage_backend_logical.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index bd902fe..38b9f08 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -265,6 +265,7 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, cleanup: VIR_FREE(regex); regfree(reg); + VIR_FREE(reg); VIR_FREE(vars); if (is_new_vol && (ret == -1)) virStorageVolDefFree(vol); -- 1.7.11.7
On 01/09/13 15:54, John Ferlan wrote:
Commit-id 'afc4631b' added the regfree(reg) to free resources alloc'd during regcomp; however, reg still needed to be VIR_FREE()'d. --- src/storage/storage_backend_logical.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index bd902fe..38b9f08 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -265,6 +265,7 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, cleanup: VIR_FREE(regex); regfree(reg); + VIR_FREE(reg); VIR_FREE(vars); if (is_new_vol && (ret == -1)) virStorageVolDefFree(vol);
Eww, who did that? O:-) Now I see it's not the only thing wrong with my fix, since regfree doesn't accept NULL. Jan
Commit-id 'afc4631b' added the regfree(reg) to free resources alloc'd during regcomp; however, reg still needed to be VIR_FREE()'d. The call to regfree() also didn't account for possible NULL value. Reformatted the call to be closer to usage. --- src/storage/storage_backend_logical.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index bd902fe..91db3fd 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -208,13 +208,16 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, if (err != 0) { char error[100]; regerror(err, reg, error, sizeof(error)); + regfree(reg); virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to compile regex %s"), error); goto cleanup; } - if (regexec(reg, groups[3], nvars, vars, 0) != 0) { + err = regexec(reg, groups[3], nvars, vars, 0); + regfree(reg); + if (err != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("malformed volume extent devices value")); goto cleanup; @@ -264,7 +267,7 @@ virStorageBackendLogicalMakeVol(virStoragePoolObjPtr pool, cleanup: VIR_FREE(regex); - regfree(reg); + VIR_FREE(reg); VIR_FREE(vars); if (is_new_vol && (ret == -1)) virStorageVolDefFree(vol); -- 1.7.11.7
On 01/10/13 20:44, John Ferlan wrote:
Commit-id 'afc4631b' added the regfree(reg) to free resources alloc'd during regcomp; however, reg still needed to be VIR_FREE()'d. The call to regfree() also didn't account for possible NULL value. Reformatted the call to be closer to usage. --- src/storage/storage_backend_logical.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
ACK
On 01/15/13 12:07, Ján Tomko wrote:
On 01/10/13 20:44, John Ferlan wrote:
Commit-id 'afc4631b' added the regfree(reg) to free resources alloc'd during regcomp; however, reg still needed to be VIR_FREE()'d. The call to regfree() also didn't account for possible NULL value. Reformatted the call to be closer to usage. --- src/storage/storage_backend_logical.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
ACK
And pushed.
Because result was used to determine whether or not to free 'priv' resources Coverity tagged the code as having a resource leak. This change addresses that concern. --- src/hyperv/hyperv_driver.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 601a85a..e69a232 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -121,6 +121,7 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) virReportOOMError(); goto cleanup; } + conn->privateData = priv; if (hypervParseUri(&priv->parsedUri, conn->uri) < 0) { goto cleanup; @@ -199,18 +200,17 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) goto cleanup; } - conn->privateData = priv; - result = VIR_DRV_OPEN_SUCCESS; cleanup: - if (result == VIR_DRV_OPEN_ERROR) { - hypervFreePrivate(&priv); - } + if (result == VIR_DRV_OPEN_ERROR) + conn->privateData = NULL; VIR_FREE(username); VIR_FREE(password); hypervFreeObject(priv, (hypervObject *)computerSystem); + if (priv && !conn->privateData) + hypervFreePrivate(&priv); return result; } -- 1.7.11.7
2013/1/9 John Ferlan <jferlan@redhat.com>:
Because result was used to determine whether or not to free 'priv' resources Coverity tagged the code as having a resource leak. This change addresses that concern. --- src/hyperv/hyperv_driver.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 601a85a..e69a232 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -121,6 +121,7 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) virReportOOMError(); goto cleanup; } + conn->privateData = priv;
if (hypervParseUri(&priv->parsedUri, conn->uri) < 0) { goto cleanup; @@ -199,18 +200,17 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) goto cleanup; }
- conn->privateData = priv; - result = VIR_DRV_OPEN_SUCCESS;
cleanup: - if (result == VIR_DRV_OPEN_ERROR) { - hypervFreePrivate(&priv); - } + if (result == VIR_DRV_OPEN_ERROR) + conn->privateData = NULL;
VIR_FREE(username); VIR_FREE(password); hypervFreeObject(priv, (hypervObject *)computerSystem); + if (priv && !conn->privateData) + hypervFreePrivate(&priv);
return result; }
The same comment as for the ESX driver in this series applies here. I'll come up with a patch for this later today. -- Matthias Bolte http://photron.blogspot.com
2013/1/10 Matthias Bolte <matthias.bolte@googlemail.com>:
2013/1/9 John Ferlan <jferlan@redhat.com>:
Because result was used to determine whether or not to free 'priv' resources Coverity tagged the code as having a resource leak. This change addresses that concern. --- src/hyperv/hyperv_driver.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c index 601a85a..e69a232 100644 --- a/src/hyperv/hyperv_driver.c +++ b/src/hyperv/hyperv_driver.c @@ -121,6 +121,7 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) virReportOOMError(); goto cleanup; } + conn->privateData = priv;
if (hypervParseUri(&priv->parsedUri, conn->uri) < 0) { goto cleanup; @@ -199,18 +200,17 @@ hypervOpen(virConnectPtr conn, virConnectAuthPtr auth, unsigned int flags) goto cleanup; }
- conn->privateData = priv; - result = VIR_DRV_OPEN_SUCCESS;
cleanup: - if (result == VIR_DRV_OPEN_ERROR) { - hypervFreePrivate(&priv); - } + if (result == VIR_DRV_OPEN_ERROR) + conn->privateData = NULL;
VIR_FREE(username); VIR_FREE(password); hypervFreeObject(priv, (hypervObject *)computerSystem); + if (priv && !conn->privateData) + hypervFreePrivate(&priv);
return result; }
The same comment as for the ESX driver in this series applies here.
I'll come up with a patch for this later today.
And here it is: https://www.redhat.com/archives/libvir-list/2013-January/msg00658.html -- Matthias Bolte http://photron.blogspot.com
ping on 4 -> 13 1 & 3 were pushed 2 is replaced by https://www.redhat.com/archives/libvir-list/2013-January/msg00657.html 14 is replaced by https://www.redhat.com/archives/libvir-list/2013-January/msg00658.html On 01/09/2013 09:54 AM, John Ferlan wrote:
This patch addresses some "Error: RESOURCE_LEAK (CWE-404)" errors found by Coverity. There are 100+ remaining; however, many seem to be false positives which will be addressed in a separate patch.
The esx_driver.c and hyperv_driver.c changes could be tagged as false positives; however, the changes made will avoid the Coverity error since the freeing of the resource marked isn't based soley on the result.
The phyp_driver.c changes in openSSHSession() and its callers were the most involved since the code with now use connection_data->sock in order to track the socket so as to allow it to be closed once done with it. Of interest in this change is that phypExec() used connection_data->sock even though it had never been initialized.
John Ferlan (14): phyp: Resolve some file descriptor leaks esx: Address resource leak found by Coverity storage: Resolve resource leak using 'vol' buffer util: Resolve resource leak for 'res' in virSetInherit error path. locking: Resolve resource leak with 'dir' on non error path rpc: Avoid resource leak of 'socks' if any object append fails uml: Avoid resource leak of event in umlInofityEvent test: Resource resource leak with 'tmp_vols' storage: Resource resource leak using 'tmp_vols' interface: Resolve resource leak wth 'tmp_iface_objs' xen: Resource resource leak with 'cpuset' parallels: Resolve some resource leaks storage: Need to also VIR_FREE(reg) hyperv: Address resource leak found by Coverityo
src/esx/esx_driver.c | 8 ++++---- src/hyperv/hyperv_driver.c | 10 +++++----- src/interface/interface_backend_netcf.c | 1 + src/locking/lock_driver_sanlock.c | 1 + src/parallels/parallels_driver.c | 30 +++++++++++++++++++----------- src/phyp/phyp_driver.c | 19 ++++++++++++++----- src/rpc/virnetserverservice.c | 6 +++--- src/storage/storage_backend_logical.c | 1 + src/storage/storage_backend_rbd.c | 13 ++++++++++--- src/storage/storage_driver.c | 1 + src/test/test_driver.c | 1 + src/uml/uml_driver.c | 6 ++++-- src/util/virlockspace.c | 1 + src/xen/xend_internal.c | 12 ++++++------ 14 files changed, 71 insertions(+), 39 deletions(-)
On 01/14/13 16:54, John Ferlan wrote:
ping on 4 -> 13
1 & 3 were pushed
2 is replaced by https://www.redhat.com/archives/libvir-list/2013-January/msg00657.html
14 is replaced by https://www.redhat.com/archives/libvir-list/2013-January/msg00658.html
I've ACKed and pushed 4, 6, 8-10, 13 and pushed 11. That leaves 5, 7 and 12.
participants (6)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan -
Ján Tomko -
Laine Stump -
Matthias Bolte