[libvirt] [PATCH] daemon: plug a memory leak
* daemon/libvirtd.c (qemudFreeClient): Avoid a leak. (qemudDispatchServer): Avoid null dereference. --- I keep finding more of these. daemon/libvirtd.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 791b3dc..2914f2f 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1410,7 +1410,7 @@ static int qemudDispatchServer(struct qemud_server *server, struct qemud_socket /* Client is running as root, so disable auth */ if (uid == 0) { VIR_INFO(_("Turn off polkit auth for privileged client pid %d from %s"), - pid, addrstr); + pid, client->addrstr); client->auth = REMOTE_AUTH_NONE; } } @@ -1451,7 +1451,7 @@ static int qemudDispatchServer(struct qemud_server *server, struct qemud_socket } else { PROBE(CLIENT_TLS_FAIL, "fd=%d", client->fd); VIR_ERROR(_("TLS handshake failed for client %s: %s"), - addrstr, gnutls_strerror (ret)); + client->addrstr, gnutls_strerror (ret)); goto error; } } @@ -2283,6 +2283,7 @@ static void qemudFreeClient(struct qemud_client *client) { if (client->conn) virConnectClose(client->conn); virMutexDestroy(&client->lock); + VIR_FREE(client->addrstr); VIR_FREE(client); } -- 1.7.3.2
On 12/10/2010 07:29 PM, Eric Blake wrote:
* daemon/libvirtd.c (qemudFreeClient): Avoid a leak. (qemudDispatchServer): Avoid null dereference. ---
I keep finding more of these.
daemon/libvirtd.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 791b3dc..2914f2f 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1410,7 +1410,7 @@ static int qemudDispatchServer(struct qemud_server *server, struct qemud_socket /* Client is running as root, so disable auth */ if (uid == 0) { VIR_INFO(_("Turn off polkit auth for privileged client pid %d from %s"), - pid, addrstr); + pid, client->addrstr); client->auth = REMOTE_AUTH_NONE; } } @@ -1451,7 +1451,7 @@ static int qemudDispatchServer(struct qemud_server *server, struct qemud_socket } else { PROBE(CLIENT_TLS_FAIL, "fd=%d", client->fd); VIR_ERROR(_("TLS handshake failed for client %s: %s"), - addrstr, gnutls_strerror (ret)); + client->addrstr, gnutls_strerror (ret)); goto error; } } @@ -2283,6 +2283,7 @@ static void qemudFreeClient(struct qemud_client *client) { if (client->conn) virConnectClose(client->conn); virMutexDestroy(&client->lock); + VIR_FREE(client->addrstr); VIR_FREE(client); }
ACK. Have you looked into some of the other stuff in the qemud_client struct? For example, nothing inside #ifdef HAVE_SASL gets freed during qemudFreeClient(). Quickly looking at the use of those items, it appears it might be possible to be freeing up the client struct when one of those is non-NULL, but I didn't read very carefully...
On 12/13/2010 08:59 AM, Laine Stump wrote:
On 12/10/2010 07:29 PM, Eric Blake wrote:
* daemon/libvirtd.c (qemudFreeClient): Avoid a leak. (qemudDispatchServer): Avoid null dereference. ---
I keep finding more of these.
ACK.
Thanks; pushed.
Have you looked into some of the other stuff in the qemud_client struct? For example, nothing inside #ifdef HAVE_SASL gets freed during qemudFreeClient(). Quickly looking at the use of those items, it appears it might be possible to be freeing up the client struct when one of those is non-NULL, but I didn't read very carefully...
I saw that in code inspection as well, but have not yet seen it show up in a valgrind run (that's not to say it is leak-free, so much as I haven't yet exercised libvirt under the right scenarios to trigger the use of sasl in the first place). So there may indeed be a leak, and I don't know how hard it would be to chase it down. I'd almost rather see danpb's rewrite to move all SASL handling into new server/client wrappers (right now, it's duplicated across daemon and remote handling), then double check that the refactored code correctly cleans up SASL data. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, Dec 13, 2010 at 09:50:40AM -0700, Eric Blake wrote:
On 12/13/2010 08:59 AM, Laine Stump wrote:
On 12/10/2010 07:29 PM, Eric Blake wrote:
* daemon/libvirtd.c (qemudFreeClient): Avoid a leak. (qemudDispatchServer): Avoid null dereference. ---
I keep finding more of these.
ACK.
Thanks; pushed.
Have you looked into some of the other stuff in the qemud_client struct? For example, nothing inside #ifdef HAVE_SASL gets freed during qemudFreeClient(). Quickly looking at the use of those items, it appears it might be possible to be freeing up the client struct when one of those is non-NULL, but I didn't read very carefully...
I saw that in code inspection as well, but have not yet seen it show up in a valgrind run (that's not to say it is leak-free, so much as I haven't yet exercised libvirt under the right scenarios to trigger the use of sasl in the first place). So there may indeed be a leak, and I don't know how hard it would be to chase it down.
I'd almost rather see danpb's rewrite to move all SASL handling into new server/client wrappers (right now, it's duplicated across daemon and remote handling), then double check that the refactored code correctly cleans up SASL data.
I've re-written all the SASL auth code from scratch in a shared module between client + server. All the SASL I/O layer is also being pushed down into a shared socket module. This will make it possible to actually unit test >95% of the RPC code without actually running daemons/drivers. Regards, Daniel
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump