1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] rpc: ensure thread safe initialization of SASL library

Daniel P. Berrangé posted 1 patch 4 years, 9 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190708103916.25325-1-berrange@redhat.com
src/rpc/virnetsaslcontext.c | 50 ++++++++++++++++++++++++-------------
1 file changed, 33 insertions(+), 17 deletions(-)
[libvirt] [PATCH] rpc: ensure thread safe initialization of SASL library
Posted by Daniel P. Berrangé 4 years, 9 months ago 
Neither the sasl_client_init or sasl_server_init methods are even
remotely threadsafe. They do a bunch of one-time initialization and
merely use a simple integer counter to avoid repeated work, not even
using atomic increment/reads on the counter. This can easily race in a
threaded program. Protect the calls using a virOnce initializer function
which is guaranteed threadsafe at least from libvirt's POV.

If the application using libvirt also uses another library that makes
use of SASL then the race still exists. It is impossible to fix that
fully except in SASL code itself.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/rpc/virnetsaslcontext.c | 50 ++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/src/rpc/virnetsaslcontext.c b/src/rpc/virnetsaslcontext.c
index f49340f033..62b103a428 100644
--- a/src/rpc/virnetsaslcontext.c
+++ b/src/rpc/virnetsaslcontext.c
@@ -77,22 +77,46 @@ VIR_ONCE_GLOBAL_INIT(virNetSASLContext);
 VIR_WARNINGS_NO_DEPRECATED
 #endif
 
-virNetSASLContextPtr virNetSASLContextNewClient(void)
+static int virNetSASLContextClientOnceInit(void)
 {
-    virNetSASLContextPtr ctxt;
-    int err;
+    int err = sasl_client_init(NULL);
+    if (err != SASL_OK) {
+        virReportError(VIR_ERR_AUTH_FAILED,
+                       _("failed to initialize SASL library: %d (%s)"),
+                       err, sasl_errstring(err, NULL, NULL));
+        return -1;
+    }
+
+    return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(virNetSASLContextClient);
 
-    if (virNetSASLContextInitialize() < 0)
-        return NULL;
 
-    err = sasl_client_init(NULL);
+static int virNetSASLContextServerOnceInit(void)
+{
+    int err = sasl_server_init(NULL, "libvirt");
     if (err != SASL_OK) {
         virReportError(VIR_ERR_AUTH_FAILED,
                        _("failed to initialize SASL library: %d (%s)"),
                        err, sasl_errstring(err, NULL, NULL));
-        return NULL;
+        return -1;
     }
 
+    return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(virNetSASLContextServer);
+
+
+virNetSASLContextPtr virNetSASLContextNewClient(void)
+{
+    virNetSASLContextPtr ctxt;
+
+    if (virNetSASLContextInitialize() < 0 ||
+        virNetSASLContextClientInitialize() < 0)
+        return NULL;
+
     if (!(ctxt = virObjectLockableNew(virNetSASLContextClass)))
         return NULL;
 
@@ -102,19 +126,11 @@ virNetSASLContextPtr virNetSASLContextNewClient(void)
 virNetSASLContextPtr virNetSASLContextNewServer(const char *const*usernameWhitelist)
 {
     virNetSASLContextPtr ctxt;
-    int err;
 
-    if (virNetSASLContextInitialize() < 0)
+    if (virNetSASLContextInitialize() < 0 ||
+        virNetSASLContextServerInitialize() < 0)
         return NULL;
 
-    err = sasl_server_init(NULL, "libvirt");
-    if (err != SASL_OK) {
-        virReportError(VIR_ERR_AUTH_FAILED,
-                       _("failed to initialize SASL library: %d (%s)"),
-                       err, sasl_errstring(err, NULL, NULL));
-        return NULL;
-    }
-
     if (!(ctxt = virObjectLockableNew(virNetSASLContextClass)))
         return NULL;
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: ensure thread safe initialization of SASL library
Posted by Michal Privoznik 4 years, 9 months ago 
On 7/8/19 12:39 PM, Daniel P. Berrangé wrote:
> Neither the sasl_client_init or sasl_server_init methods are even
> remotely threadsafe. They do a bunch of one-time initialization and
> merely use a simple integer counter to avoid repeated work, not even
> using atomic increment/reads on the counter. This can easily race in a
> threaded program. Protect the calls using a virOnce initializer function
> which is guaranteed threadsafe at least from libvirt's POV.
> 
> If the application using libvirt also uses another library that makes
> use of SASL then the race still exists. It is impossible to fix that
> fully except in SASL code itself.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   src/rpc/virnetsaslcontext.c | 50 ++++++++++++++++++++++++-------------
>   1 file changed, 33 insertions(+), 17 deletions(-)

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Thanks Sahid for the report!

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: ensure thread safe initialization of SASL library
Posted by Daniel P. Berrangé 4 years, 9 months ago 
On Mon, Jul 08, 2019 at 01:12:06PM +0200, Michal Privoznik wrote:
> On 7/8/19 12:39 PM, Daniel P. Berrangé wrote:
> > Neither the sasl_client_init or sasl_server_init methods are even
> > remotely threadsafe. They do a bunch of one-time initialization and
> > merely use a simple integer counter to avoid repeated work, not even
> > using atomic increment/reads on the counter. This can easily race in a
> > threaded program. Protect the calls using a virOnce initializer function
> > which is guaranteed threadsafe at least from libvirt's POV.
> > 
> > If the application using libvirt also uses another library that makes
> > use of SASL then the race still exists. It is impossible to fix that
> > fully except in SASL code itself.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   src/rpc/virnetsaslcontext.c | 50 ++++++++++++++++++++++++-------------
> >   1 file changed, 33 insertions(+), 17 deletions(-)
> 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> Thanks Sahid for the report!

FYI i wrote a simple demo program that can reliably reproduce the problem
in isolation

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
#include <libvirt/libvirt.h>
#include <stdio.h>
#include <pthread.h>
#include <unistd.h>


pthread_cond_t condReady;
pthread_cond_t condRun;
pthread_mutex_t lock;
int running;

void *runme(void *arg)
{
  virConnectPtr conn;

  pthread_mutex_lock(&lock);
  running++;
  fprintf(stderr, "Notifying we are ready\n");
  pthread_cond_signal(&condReady);
  pthread_cond_wait(&condRun, &lock);
  pthread_mutex_unlock(&lock);
  fprintf(stderr, "Opening libvirt\n");
  conn = virConnectOpenAuth("qemu:///system", virConnectAuthPtrDefault, 0);
  fprintf(stderr, "Open %s\n", conn ? virConnectGetURI(conn) : "failed");
  if (conn)
    virConnectClose(conn);
}

int main(int argc, char **argv)
{
  int i;
  pthread_t th[20];

  pthread_mutex_init(&lock, NULL);
  pthread_cond_init(&condRun, NULL);
  pthread_cond_init(&condReady, NULL);

  for (i = 0; i < 20; i++) {
    pthread_create(&th[i], NULL, runme, NULL);
  }

  fprintf(stderr, "Waiting for threads to initialize\n");
  pthread_mutex_lock(&lock);
  while (running != 20)
    pthread_cond_wait(&condReady, &lock);
  fprintf(stderr, "Telling threads to proceed\n");
  pthread_cond_broadcast(&condRun);
  pthread_mutex_unlock(&lock);

  for (i = 0; i < 20; i++) {
    pthread_join(th[i], NULL);
  }
  return 0;
}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] rpc: ensure thread safe initialization of SASL library
Posted by Sahid Orentino Ferdjaoui 4 years, 9 months ago 
On Mon, Jul 08, 2019 at 01:04:59PM +0100, Daniel P. Berrangé wrote:
> On Mon, Jul 08, 2019 at 01:12:06PM +0200, Michal Privoznik wrote:
> > On 7/8/19 12:39 PM, Daniel P. Berrangé wrote:
> > > Neither the sasl_client_init or sasl_server_init methods are even
> > > remotely threadsafe. They do a bunch of one-time initialization and
> > > merely use a simple integer counter to avoid repeated work, not even
> > > using atomic increment/reads on the counter. This can easily race in a
> > > threaded program. Protect the calls using a virOnce initializer function
> > > which is guaranteed threadsafe at least from libvirt's POV.
> > > 
> > > If the application using libvirt also uses another library that makes
> > > use of SASL then the race still exists. It is impossible to fix that
> > > fully except in SASL code itself.
> > > 
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > >   src/rpc/virnetsaslcontext.c | 50 ++++++++++++++++++++++++-------------
> > >   1 file changed, 33 insertions(+), 17 deletions(-)
> > 
> > Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> > 
> > Thanks Sahid for the report!
> 
> FYI i wrote a simple demo program that can reliably reproduce the problem
> in isolation

Also tested in my side, the patch well fixed the issue.

Thanks Daniel and Michal.

s.

> Regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

> #include <libvirt/libvirt.h>
> #include <stdio.h>
> #include <pthread.h>
> #include <unistd.h>
> 
> 
> pthread_cond_t condReady;
> pthread_cond_t condRun;
> pthread_mutex_t lock;
> int running;
> 
> void *runme(void *arg)
> {
>   virConnectPtr conn;
> 
>   pthread_mutex_lock(&lock);
>   running++;
>   fprintf(stderr, "Notifying we are ready\n");
>   pthread_cond_signal(&condReady);
>   pthread_cond_wait(&condRun, &lock);
>   pthread_mutex_unlock(&lock);
>   fprintf(stderr, "Opening libvirt\n");
>   conn = virConnectOpenAuth("qemu:///system", virConnectAuthPtrDefault, 0);
>   fprintf(stderr, "Open %s\n", conn ? virConnectGetURI(conn) : "failed");
>   if (conn)
>     virConnectClose(conn);
> }
> 
> int main(int argc, char **argv)
> {
>   int i;
>   pthread_t th[20];
> 
>   pthread_mutex_init(&lock, NULL);
>   pthread_cond_init(&condRun, NULL);
>   pthread_cond_init(&condReady, NULL);
> 
>   for (i = 0; i < 20; i++) {
>     pthread_create(&th[i], NULL, runme, NULL);
>   }
> 
>   fprintf(stderr, "Waiting for threads to initialize\n");
>   pthread_mutex_lock(&lock);
>   while (running != 20)
>     pthread_cond_wait(&condReady, &lock);
>   fprintf(stderr, "Telling threads to proceed\n");
>   pthread_cond_broadcast(&condRun);
>   pthread_mutex_unlock(&lock);
> 
>   for (i = 0; i < 20; i++) {
>     pthread_join(th[i], NULL);
>   }
>   return 0;
> }

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.