Re: [PATCH] Client-side Cyrus SASL support

On 8/9/06, Garrett Rooney <rooneg@electricjellyfish.net> wrote:
> > > +static void *sasl_mutex_alloc_cb(void)
> > > +{
> > > + apr_thread_mutex_t *mutex;
> > > + apr_status_t apr_err;
> > > + if (apr_is_empty_array(free_mutexes))
> > > + {
> > > + apr_err = apr_thread_mutex_create(&mutex,
> > > + APR_THREAD_MUTEX_DEFAULT,
> > > + sasl_pool);
> > > + if (apr_err != APR_SUCCESS)
> > > + return NULL;
> > > + }
> > > + else
> > > + {
> > > + apr_err = apr_thread_mutex_lock(array_mutex);
> > > + if (apr_err != APR_SUCCESS)
> > > + return NULL;
> > > + mutex = *((apr_thread_mutex_t**)apr_array_pop(free_mutexes));
> > > + apr_err = apr_thread_mutex_unlock(array_mutex);
> > > + if (apr_err != APR_SUCCESS)
> > > + return NULL;
> > > + }
> > > + return mutex;
> > > +}
> > >
> > > You can't access the array without locking the mutex. The lock needs
> > > to be pulled up outside the if statement, before you call
> > > apr_is_empty_array, otherwise none of it is thread safe. Basically
> > > you need to lock the mutex before ANY access to that array.
> >
> > Oops. Thanks for pointing that out.
>
> Still not quite right:
>
> +static void *sasl_mutex_alloc_cb(void)
> +{
> + apr_thread_mutex_t *mutex;
> + apr_status_t apr_err;
> +
> + apr_err = apr_thread_mutex_lock(array_mutex);
> + if (apr_err != APR_SUCCESS)
> + return NULL;
> +
> + if (apr_is_empty_array(free_mutexes))
> + {
> + apr_err = apr_thread_mutex_create(&mutex,
> + APR_THREAD_MUTEX_DEFAULT,
> + sasl_pool);
> + if (apr_err != APR_SUCCESS)
> + return NULL;
>
> If you return here you'll leave the mutex locked, you've gotta unlock
> it before returning.
>
> -garrett
>

I can't believe I missed that. See the new patched patch.

-- 
Vlad

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org