11 a.m.
Hey,
While running virsh through valgrind for some SASL tests, I triggered some
leaks/invalid reads, this patch series fixes these.
Christophe
11 a.m.
New subject: [libvirt] [PATCH 1/3] remote: Don't leak priv->tls object on connection failure
When testing SASL authentication over TLS with
virsh -c qemu+tls:///system list --all
I got this valgrind trace after entering wrong credentials:
==30540== 26,903 (88 direct, 26,815 indirect) bytes in 1 blocks are definitely lost in
loss record 289 of 293
==30540== at 0x4A081D4: calloc (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==30540== by 0x4C7379A: virAllocVar (viralloc.c:558)
==30540== by 0x4CBC178: virObjectNew (virobject.c:190)
==30540== by 0x4CBC329: virObjectLockableNew (virobject.c:216)
==30540== by 0x4E2D003: virNetTLSContextNew (virnettlscontext.c:719)
==30540== by 0x4E2DC3F: virNetTLSContextNewPath (virnettlscontext.c:930)
==30540== by 0x4E2DD5B: virNetTLSContextNewClientPath (virnettlscontext.c:957)
==30540== by 0x4DDB618: doRemoteOpen (remote_driver.c:627)
==30540== by 0x4DDC8BA: remoteConnectOpen (remote_driver.c:1031)
==30540== by 0x4D8595F: do_open (libvirt.c:1239)
==30540== by 0x4D863F3: virConnectOpenAuth (libvirt.c:1481)
==30540== by 0x12762B: vshReconnect (virsh.c:337)
==30540== by 0x12C9B0: vshInit (virsh.c:2470)
==30540== by 0x12E9A5: main (virsh.c:3338)
---
src/remote/remote_driver.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 38da119..5c559ee 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -910,6 +910,10 @@ doRemoteOpen(virConnectPtr conn,
virNetClientClose(priv->client);
virObjectUnref(priv->client);
priv->client = NULL;
+#ifdef WITH_GNUTLS
+ virObjectUnref(priv->tls);
+ priv->tls = NULL;
+#endif
VIR_FREE(priv->hostname);
goto cleanup;
--
1.8.4.2
11 a.m.
New subject: [libvirt] [PATCH 2/3] Tie SASL callbacks lifecycle to virNetSessionSASLContext
The array of sasl_callback_t callbacks which is passed to sasl_client_new()
must be kept alive as long as the created sasl_conn_t object is alive as
cyrus-sasl uses this structure internally for things like logging, so
the memory used for callbacks must only be freed after sasl_dispose() has
been called.
During testing of successful SASL logins with
virsh -c qemu+tls:///system list --all
I've been getting invalid read reports from valgrind
==9237== Invalid read of size 8
==9237== at 0x6E93B6F: _sasl_getcallback (common.c:1745)
==9237== by 0x6E95430: _sasl_log (common.c:1850)
==9237== by 0x16593D87: digestmd5_client_mech_dispose (digestmd5.c:4580)
==9237== by 0x6E91653: client_dispose (client.c:332)
==9237== by 0x6E9476A: sasl_dispose (common.c:851)
==9237== by 0x4E225A1: virNetSASLSessionDispose (virnetsaslcontext.c:678)
==9237== by 0x4CBC551: virObjectUnref (virobject.c:262)
==9237== by 0x4E254D1: virNetSocketDispose (virnetsocket.c:1042)
==9237== by 0x4CBC551: virObjectUnref (virobject.c:262)
==9237== by 0x4E2701C: virNetSocketEventFree (virnetsocket.c:1794)
==9237== by 0x4C965D3: virEventPollCleanupHandles (vireventpoll.c:583)
==9237== by 0x4C96987: virEventPollRunOnce (vireventpoll.c:652)
==9237== by 0x4C94730: virEventRunDefaultImpl (virevent.c:274)
==9237== by 0x12C7BA: vshEventLoop (virsh.c:2407)
==9237== by 0x4CD3D04: virThreadHelper (virthreadpthread.c:161)
==9237== by 0x7DAEF32: start_thread (pthread_create.c:309)
==9237== by 0x8C86EAC: clone (clone.S:111)
==9237== Address 0xe2d61b0 is 0 bytes inside a block of size 168 free'd
==9237== at 0x4A07577: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==9237== by 0x4C73827: virFree (viralloc.c:580)
==9237== by 0x4DE4BC7: remoteAuthSASL (remote_driver.c:4219)
==9237== by 0x4DE33D0: remoteAuthenticate (remote_driver.c:3639)
==9237== by 0x4DDBFAA: doRemoteOpen (remote_driver.c:832)
==9237== by 0x4DDC8DC: remoteConnectOpen (remote_driver.c:1031)
==9237== by 0x4D8595F: do_open (libvirt.c:1239)
==9237== by 0x4D863F3: virConnectOpenAuth (libvirt.c:1481)
==9237== by 0x12762B: vshReconnect (virsh.c:337)
==9237== by 0x12C9B0: vshInit (virsh.c:2470)
==9237== by 0x12E9A5: main (virsh.c:3338)
This commit changes virNetSASLSessionNewClient() to take ownership of the SASL
callbacks. Then we can free them in virNetSASLSessionDispose() after the corresponding
sasl_conn_t has been freed.
---
src/remote/remote_driver.c | 2 ++
src/rpc/virnetsaslcontext.c | 6 ++++--
src/rpc/virnetsaslcontext.h | 2 +-
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 5c559ee..3a22cdf 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -4033,6 +4033,8 @@ remoteAuthSASL(virConnectPtr conn, struct private_data *priv,
virNetClientRemoteAddrString(priv->client),
saslcb)))
goto cleanup;
+ /* saslcb is now owned by sasl */
+ saslcb = NULL;
# ifdef WITH_GNUTLS
/* Initialize some connection props we care about */
diff --git a/src/rpc/virnetsaslcontext.c b/src/rpc/virnetsaslcontext.c
index 35dc6cf..1baf41e 100644
--- a/src/rpc/virnetsaslcontext.c
+++ b/src/rpc/virnetsaslcontext.c
@@ -44,6 +44,7 @@ struct _virNetSASLSession {
sasl_conn_t *conn;
size_t maxbufsize;
+ sasl_callback_t *callbacks;
};
@@ -167,7 +168,7 @@ virNetSASLSessionPtr virNetSASLSessionNewClient(virNetSASLContextPtr
ctxt ATTRIB
const char *hostname,
const char *localAddr,
const char *remoteAddr,
- const sasl_callback_t *cbs)
+ sasl_callback_t *cbs)
{
virNetSASLSessionPtr sasl = NULL;
int err;
@@ -191,6 +192,7 @@ virNetSASLSessionPtr virNetSASLSessionNewClient(virNetSASLContextPtr
ctxt ATTRIB
err, sasl_errstring(err, NULL, NULL));
goto cleanup;
}
+ sasl->callbacks = cbs;
return sasl;
@@ -674,5 +676,5 @@ void virNetSASLSessionDispose(void *obj)
if (sasl->conn)
sasl_dispose(&sasl->conn);
-
+ VIR_FREE(sasl->callbacks);
}
diff --git a/src/rpc/virnetsaslcontext.h b/src/rpc/virnetsaslcontext.h
index e726302..54634d5 100644
--- a/src/rpc/virnetsaslcontext.h
+++ b/src/rpc/virnetsaslcontext.h
@@ -49,7 +49,7 @@ virNetSASLSessionPtr virNetSASLSessionNewClient(virNetSASLContextPtr
ctxt,
const char *hostname,
const char *localAddr,
const char *remoteAddr,
- const sasl_callback_t *cbs);
+ sasl_callback_t *cbs);
virNetSASLSessionPtr virNetSASLSessionNewServer(virNetSASLContextPtr ctxt,
const char *service,
const char *localAddr,
--
1.8.4.2
11 a.m.
New subject: [libvirt] [PATCH 3/3] Fix invalid read in virNetSASLSessionClientStep debug log
virNetSASLSessionClientStep logs the data that is going to be passed to
sasl_client_step as input data. However, it tries to log it as a string,
while there is no guarantee that this data is going to be nul-terminated.
This leads to this valgrind log:
==20938== Invalid read of size 1
==20938== at 0x8BDB08F: vfprintf (vfprintf.c:1635)
==20938== by 0x8C06DF2: vasprintf (vasprintf.c:62)
==20938== by 0x4CCEDF9: virVasprintfInternal (virstring.c:337)
==20938== by 0x4CA9516: virLogVMessage (virlog.c:842)
==20938== by 0x4CA939A: virLogMessage (virlog.c:778)
==20938== by 0x4E21E0D: virNetSASLSessionClientStep (virnetsaslcontext.c:458)
==20938== by 0x4DE47B8: remoteAuthSASL (remote_driver.c:4136)
==20938== by 0x4DE33AE: remoteAuthenticate (remote_driver.c:3635)
==20938== by 0x4DDBFAA: doRemoteOpen (remote_driver.c:832)
==20938== by 0x4DDC8BA: remoteConnectOpen (remote_driver.c:1027)
==20938== by 0x4D8595F: do_open (libvirt.c:1239)
==20938== by 0x4D863F3: virConnectOpenAuth (libvirt.c:1481)
==20938== by 0x12762B: vshReconnect (virsh.c:337)
==20938== by 0x12C9B0: vshInit (virsh.c:2470)
==20938== by 0x12E9A5: main (virsh.c:3338)
==20938== Address 0xe329ccd is 0 bytes after a block of size 141 alloc'd
==20938== at 0x4A081D4: calloc (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==20938== by 0x8CB91B4: xdr_array (xdr_array.c:94)
==20938== by 0x4E039C2: xdr_remote_auth_sasl_start_ret (remote_protocol.c:3134)
==20938== by 0x4E1F8AA: virNetMessageDecodePayload (virnetmessage.c:405)
==20938== by 0x4E119F5: virNetClientProgramCall (virnetclientprogram.c:377)
==20938== by 0x4DF8141: callFull (remote_driver.c:5794)
==20938== by 0x4DF821A: call (remote_driver.c:5816)
==20938== by 0x4DE46CF: remoteAuthSASL (remote_driver.c:4112)
==20938== by 0x4DE33AE: remoteAuthenticate (remote_driver.c:3635)
==20938== by 0x4DDBFAA: doRemoteOpen (remote_driver.c:832)
==20938== by 0x4DDC8BA: remoteConnectOpen (remote_driver.c:1027)
==20938== by 0x4D8595F: do_open (libvirt.c:1239)
==20938== by 0x4D863F3: virConnectOpenAuth (libvirt.c:1481)
==20938== by 0x12762B: vshReconnect (virsh.c:337)
==20938== by 0x12C9B0: vshInit (virsh.c:2470)
==20938== by 0x12E9A5: main (virsh.c:3338)
---
src/rpc/virnetsaslcontext.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/rpc/virnetsaslcontext.c b/src/rpc/virnetsaslcontext.c
index 1baf41e..dbb9a25 100644
--- a/src/rpc/virnetsaslcontext.c
+++ b/src/rpc/virnetsaslcontext.c
@@ -457,7 +457,7 @@ int virNetSASLSessionClientStep(virNetSASLSessionPtr sasl,
int err;
int ret = -1;
- VIR_DEBUG("sasl=%p serverin=%s serverinlen=%zu prompt_need=%p clientout=%p
clientoutlen=%p",
+ VIR_DEBUG("sasl=%p serverin=%p serverinlen=%zu prompt_need=%p clientout=%p
clientoutlen=%p",
sasl, serverin, serverinlen, prompt_need, clientout, clientoutlen);
virObjectLock(sasl);
--
1.8.4.2
2 p.m.
On 11/22/2013 10:00 AM, Christophe Fergeau wrote:
Hey,
While running virsh through valgrind for some SASL tests, I triggered some
leaks/invalid reads, this patch series fixes these.
ACK series.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:41 a.m.
On Fri, Nov 22, 2013 at 01:00:59PM -0700, Eric Blake wrote:
On 11/22/2013 10:00 AM, Christophe Fergeau wrote:
> Hey,
>
> While running virsh through valgrind for some SASL tests, I triggered some
> leaks/invalid reads, this patch series fixes these.
ACK series.
I did not get to push these before the freeze, I assume this is still ok as
they were ACK'ed before, and as they also are bug fixes? I prefer to
double-check it's fine before pushing :)
Christophe
4:43 a.m.
On Tue, Nov 26, 2013 at 11:41:12AM +0100, Christophe Fergeau wrote:
On Fri, Nov 22, 2013 at 01:00:59PM -0700, Eric Blake wrote:
> On 11/22/2013 10:00 AM, Christophe Fergeau wrote:
> > Hey,
> >
> > While running virsh through valgrind for some SASL tests, I triggered some
> > leaks/invalid reads, this patch series fixes these.
>
> ACK series.
I did not get to push these before the freeze, I assume this is still ok as
they were ACK'ed before, and as they also are bug fixes? I prefer to
double-check it's fine before pushing :)
Yes, for bugfixes like this it is fine.
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 :|
5:11 a.m.
On Tue, Nov 26, 2013 at 10:43:15AM +0000, Daniel P. Berrange wrote:
On Tue, Nov 26, 2013 at 11:41:12AM +0100, Christophe Fergeau wrote:
> On Fri, Nov 22, 2013 at 01:00:59PM -0700, Eric Blake wrote:
> > On 11/22/2013 10:00 AM, Christophe Fergeau wrote:
> > > Hey,
> > >
> > > While running virsh through valgrind for some SASL tests, I triggered
some
> > > leaks/invalid reads, this patch series fixes these.
> >
> > ACK series.
>
> I did not get to push these before the freeze, I assume this is still ok as
> they were ACK'ed before, and as they also are bug fixes? I prefer to
> double-check it's fine before pushing :)
Yes, for bugfixes like this it is fine.
Thanks, I've pushed this now.
Christophe
4016
days inactive
4020
days old
7 comments
3 participants
participants (3)
-
Christophe Fergeau -
Daniel P. Berrange -
Eric Blake