6:29 p.m.
* 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
9:59 a.m.
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...
10:50 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:57 a.m.
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
5094
days inactive
5096
days old
3 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump