From e2efe667ad176ed8ffcd920edbe420d6e8f52218 Mon Sep 17 00:00:00 2001

Message-Id: <e2efe667ad176ed8ffcd920edbe420d6e8f52218.1386932212.git.jdenemar@redhat.com>

From: Christophe Fergeau <cfergeau@redhat.com>

Date: Wed, 11 Dec 2013 13:39:11 +0100

Subject: [PATCH] Tie SASL callbacks lifecycle to virNetSessionSASLContext

https://bugzilla.redhat.com/show_bug.cgi?id=1039991

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.

(cherry picked from commit 13fdc6d63ef64f8e231a087e1dab7d90145c3c10)

Signed-off-by: Jiri Denemark <jdenemar@redhat.com>

---

 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 b3e86e1..557b408 100644

--- a/src/remote/remote_driver.c

+++ b/src/remote/remote_driver.c

@@ -4029,6 +4029,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.5.1