[libvirt] [PATCH 0/3] SASL valgrind fixes
Hey, While running virsh through valgrind for some SASL tests, I triggered some leaks/invalid reads, this patch series fixes these. Christophe
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
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
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
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
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
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 :|
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
participants (3)
-
Christophe Fergeau -
Daniel P. Berrange -
Eric Blake