8:54 a.m.
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
8:54 a.m.
New subject: [libvirt] [PATCH 01/14] phyp: Resolve some file descriptor leaks
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
8:08 p.m.
New subject: [libvirt] [PATCH 01/14] phyp: Resolve some file descriptor leaks
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
4:06 a.m.
New subject: [libvirt] [PATCH 01/14] phyp: Resolve some file descriptor leaks
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 :|
1:21 p.m.
New subject: [libvirt] [PATCH v2 01/14] phyp: Resolve some file descriptor leaks
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
6:04 p.m.
New subject: [libvirt] [PATCH v2 01/14] phyp: Resolve some file descriptor leaks
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
8:54 a.m.
New subject: [libvirt] [PATCH 02/14] esx: Address resource leak found by Coverity
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
9:27 a.m.
New subject: [libvirt] [PATCH 02/14] esx: Address resource leak found by Coverity
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
9:49 a.m.
New subject: [libvirt] [PATCH 02/14] esx: Address resource leak found by Coverity
2013/1/10 Eric Blake <eblake(a)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
9:55 a.m.
New subject: [libvirt] [PATCH 02/14] esx: Address resource leak found by Coverity
On 01/10/2013 10:49 AM, Matthias Bolte wrote:
2013/1/10 Eric Blake <eblake(a)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.
3:55 p.m.
New subject: [libvirt] [PATCH 02/14] esx: Address resource leak found by Coverity
2013/1/10 John Ferlan <jferlan(a)redhat.com>:
On 01/10/2013 10:49 AM, Matthias Bolte wrote:
> 2013/1/10 Eric Blake <eblake(a)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
6:21 p.m.
New subject: [libvirt] [PATCH 02/14] esx: Address resource leak found by Coverity
On 01/10/2013 04:55 PM, Matthias Bolte wrote:
2013/1/10 John Ferlan <jferlan(a)redhat.com>:
> On 01/10/2013 10:49 AM, Matthias Bolte wrote:
>> 2013/1/10 Eric Blake <eblake(a)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
8:54 a.m.
New subject: [libvirt] [PATCH 03/14] storage: Resolve resource leak using 'vol' buffer
---
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
6:14 p.m.
New subject: [libvirt] [PATCH 03/14] storage: Resolve resource leak using 'vol' buffer
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
8:54 a.m.
New subject: [libvirt] [PATCH 04/14] util: Resolve resource leak for 'res' in virSetInherit error path.
---
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
8:54 a.m.
New subject: [libvirt] [PATCH 05/14] locking: Resolve resource leak with 'dir' on non error path
---
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
7:03 a.m.
New subject: [libvirt] [PATCH 05/14] locking: Resolve resource leak with 'dir' on non error path
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.
8:23 a.m.
New subject: [libvirt] [PATCH v2 5/14] locking: Resolve resource leaks on non error path
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
8:54 a.m.
New subject: [libvirt] [PATCH 06/14] rpc: Avoid resource leak of 'socks' if any object append fails
---
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
8:54 a.m.
New subject: [libvirt] [PATCH 07/14] uml: Avoid resource leak of event in umlInofityEvent
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
8:54 a.m.
New subject: [libvirt] [PATCH 08/14] test: Resource resource leak with 'tmp_vols'
---
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
8:54 a.m.
New subject: [libvirt] [PATCH 09/14] storage: Resource resource leak using 'tmp_vols'
---
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
8:54 a.m.
New subject: [libvirt] [PATCH 10/14] interface: Resolve resource leak wth 'tmp_iface_objs'
---
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
8:54 a.m.
New subject: [libvirt] [PATCH 11/14] xen: Resource resource leak with 'cpuset'
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
10:55 a.m.
New subject: [libvirt] [PATCH 11/14] xen: Resource resource leak with 'cpuset'
(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.
12:50 p.m.
New subject: [libvirt] [PATCH 11/14] xen: Resource resource leak with 'cpuset'
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:07 p.m.
New subject: [libvirt] [PATCH 11/14] xen: Resource resource leak with 'cpuset'
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.
8:32 p.m.
New subject: [libvirt] [PATCH 11/14] xen: Resource resource leak with 'cpuset'
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
7:42 a.m.
New subject: [libvirt] [PATCH v2 11/14] xen: Resolve resource leak with 'cpuset'
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
10:42 a.m.
New subject: [libvirt] [PATCH v2 11/14] xen: Resolve resource leak with 'cpuset'
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).
11:46 a.m.
New subject: [libvirt] [PATCH v3 11/14] xen: Resolve resource leak with 'cpuset'
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
8:54 a.m.
New subject: [libvirt] [PATCH 12/14] parallels: Resolve some resource leaks
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
8:54 a.m.
New subject: [libvirt] [PATCH 13/14] storage: Need to also VIR_FREE(reg)
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
9:31 a.m.
New subject: [libvirt] [PATCH 13/14] storage: Need to also VIR_FREE(reg)
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
1:44 p.m.
New subject: [libvirt] [PATCH v2 13/14] storage: Need to also VIR_FREE(reg)
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
5:07 a.m.
New subject: [libvirt] [PATCH v2 13/14] storage: Need to also VIR_FREE(reg)
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
5:15 a.m.
New subject: [libvirt] [PATCH v2 13/14] storage: Need to also VIR_FREE(reg)
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.
8:54 a.m.
New subject: [libvirt] [PATCH 14/14] hyperv: Address resource leak found by Coverityo
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
9:50 a.m.
New subject: [libvirt] [PATCH 14/14] hyperv: Address resource leak found by Coverityo
2013/1/9 John Ferlan <jferlan(a)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
3:58 p.m.
New subject: [libvirt] [PATCH 14/14] hyperv: Address resource leak found by Coverityo
2013/1/10 Matthias Bolte <matthias.bolte(a)googlemail.com>:
2013/1/9 John Ferlan <jferlan(a)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
9:54 a.m.
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(-)
7:54 a.m.
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.
4329
days inactive
4336
days old
47 comments
6 participants
participants (6)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan -
Ján Tomko -
Laine Stump -
Matthias Bolte