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

[Sahid Orentino Ferdjaoui]

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 at redhat.com>
> > > ---
> > >   src/rpc/virnetsaslcontext.c | 50 ++++++++++++++++++++++++-------------
> > >   1 file changed, 33 insertions(+), 17 deletions(-)
> > 
> > Reviewed-by: Michal Privoznik <mprivozn at 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;
> }