"Vlad Georgescu" <vgeorgescu@gmail.com> writes:
> On 7/20/06, Philip Martin <philip@codematters.co.uk> wrote:
>> "Vlad Georgescu" <vgeorgescu@gmail.com> writes:
>> > +static void sasl_mutex_free_cb(void *mutex)
>> > +{
>> > + apr_thread_mutex_lock(array_mutex);
>>
>> Check for errors?
>
> It wouldn't do us any good here. That callback has a void return value.
>
>>
>> > + *((apr_thread_mutex_t**)apr_array_push(free_mutexes)) = mutex;
>> > + apr_thread_mutex_unlock(array_mutex);
>> > +}
Is it sensible to modify the array if the mutex lock failed? Is it
sensible to unlock the mutex if the lock failed?
>> > +apr_status_t svn_ra_svn__sasl_common_init()
>> > +{
>> > + apr_status_t apr_err = APR_SUCCESS;
>> > +
>> > + atexit(sasl_done);
>>
>> If the client calls this function more than once then it will register
>> more than one atexit handler. Is it safe to call sasl_done more than
>> once?
>
> svn_ra_svn__sasl_common_init() is an internal function, guaranteed to
> be called only once. See svn_ra_svn__sasl_init().
If Subversion is built with --enable-dso then I suspect it might get
called more than once, certainly static variable within the library
must get initialised each time the DSO is loaded.
> I attached a new version of my patch.
>
> Thanks,
> Vlad
>
> Index: subversion/libsvn_ra_svn/ra_svn_sasl.h
> ===================================================================
> --- subversion/libsvn_ra_svn/ra_svn_sasl.h (revision 0)
> +++ subversion/libsvn_ra_svn/ra_svn_sasl.h (revision 0)
> @@ -0,0 +1,50 @@
> +/*
> + * ra_svn_sasl.h : SASL-related declarations shared between the
> + * ra_svn and svnserve module
> + *
> + * ====================================================================
> + * Copyright (c) 2006 CollabNet. All rights reserved.
> + *
> + * This software is licensed as described in the file COPYING, which
> + * you should have received as part of this distribution. The terms
> + * are also available at http://subversion.tigris.org/license-1.html.
> + * If newer versions of this license are posted there, you may use a
> + * newer version instead, at your option.
> + *
> + * This software consists of voluntary contributions made by many
> + * individuals. For exact contribution history, see the revision
> + * history and logs, available at http://subversion.tigris.org/.
> + * ====================================================================
> + */
> +
> +
> +
> +#ifndef RA_SVN_SASL_H
> +#define RA_SVN_SASL_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +#include <sasl/sasl.h>
> +
> +/* Define sane defaults for a sasl_security_properties_t structure.
> + See sasl.h for details. 4096 is the size of our read buffer (SASL needs
> + to know this value when negotiating a security layer). */
> +#define DEFAULT_SECPROPS {0, 256, 4096, 0, NULL, NULL}
Does 256 correspond to "baseline AES"?
Is 4096 a number that can be changed or does it have to match some
buffer size within the Subversion code?
DEFAULT_SECPROPS is only used once, why is it in a header file?
> +
> +/* This function is called by the client and the server before
> + calling sasl_{client, server}_init */
> +apr_status_t svn_ra_svn__sasl_common_init();
> +
> +/* Sets local_addrport and remote_addrport to a string containing the
> + remote and local IP address and port, formatted like this: a.b.c.d;port. */
> +svn_error_t *svn_ra_svn__get_addresses(const char **local_addrport,
> + const char **remote_addrport,
> + apr_socket_t *sock,
> + apr_pool_t *pool);
> +#ifdef __cplusplus
> +}
> +#endif /* __cplusplus */
> +
> +#endif /* RA_SVN_SASL_H */
> Index: subversion/libsvn_ra_svn/sasl_auth.c
> ===================================================================
> --- subversion/libsvn_ra_svn/sasl_auth.c (revision 0)
> +++ subversion/libsvn_ra_svn/sasl_auth.c (revision 0)
> @@ -0,0 +1,548 @@
> +/*
> + * sasl_auth.c : functions for SASL-based authentication
> + *
> + * ====================================================================
> + * Copyright (c) 2006 CollabNet. All rights reserved.
> + *
> + * This software is licensed as described in the file COPYING, which
> + * you should have received as part of this distribution. The terms
> + * are also available at http://subversion.tigris.org/license-1.html.
> + * If newer versions of this license are posted there, you may use a
> + * newer version instead, at your option.
> + *
> + * This software consists of voluntary contributions made by many
> + * individuals. For exact contribution history, see the revision
> + * history and logs, available at http://subversion.tigris.org/.
> + * ====================================================================
> + */
> +
> +#define APR_WANT_STRFUNC
> +#include <apr_want.h>
> +#include <apr_general.h>
> +#include <apr_strings.h>
> +#include <apr_atomic.h>
> +#include <apr_thread_mutex.h>
> +#include <apr_version.h>
> +
> +#include "svn_types.h"
> +#include "svn_string.h"
> +#include "svn_error.h"
> +#include "svn_pools.h"
> +#include "svn_private_config.h"
> +#include "svn_ra.h"
> +#include "svn_ra_svn.h"
> +#include "svn_base64.h"
> +
> +#include "ra_svn.h"
> +#include "ra_svn_sasl.h"
> +
> +#ifdef SVN_HAVE_SASL
> +
> +#ifdef APR_HAS_THREADS
> +
> +/* Magic values for atomic initialization of the SASL library. */
> +#define SASL_UNINITIALIZED 0
> +#define SASL_START_INIT 1
> +#define SASL_INIT_FAILED 2
> +#define SASL_INITIALIZED 3
> +
> +/* ### These defines were taken from libsvn_fs_base/bdb/env.c.
> + * Perhaps they should be moved to svn_private_config.h as suggested there? */
> +#if APR_MAJOR_VERSION > 0
> +# define svn__atomic_t apr_uint32_t
> +# define svn__atomic_cas(mem, with, cmp) \
> + apr_atomic_cas32((mem), (with), (cmp))
> +#else
> +# define svn__atomic_t apr_atomic_t
> +# define svn__atomic_cas(mem, with, cmp) \
> + apr_atomic_cas((mem), (with), (cmp))
> +#endif
> +
> +static volatile svn__atomic_t sasl_status = SASL_UNINITIALIZED;
> +
> +/* Cyrus SASL is thread-safe only if we supply it with mutex functions
> + * (with sasl_set_mutex()). To make this work with APR, we need to use a
> + * global pool for the mutex allocations. Freeing a mutex actually
> + * returns it to a global array. We allocate mutexes from this
> + * array if it is non-empty, or directly from the pool otherwise.
> + * We also need a mutex to serialize accesses to the array itself.
> + */
> +
> +/* We allocate mutexes for SASL from this pool. */
> +static apr_pool_t *sasl_pool = NULL;
> +
> +/* An array of allocated, but unused, apr_thread_mutex_t's. */
> +static apr_array_header_t *free_mutexes = NULL;
> +
> +/* A mutex to serialize access to the array. */
> +static apr_thread_mutex_t *array_mutex = NULL;
> +
> +/* Callbacks we pass to sasl_set_mutex. */
> +
> +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;
> +}
> +
> +static int sasl_mutex_lock_cb(void *mutex)
> +{
> + return (apr_thread_mutex_lock(mutex) == APR_SUCCESS) ? 0 : -1;
> +}
> +
> +static int sasl_mutex_unlock_cb(void *mutex)
> +{
> + return (apr_thread_mutex_unlock(mutex) == APR_SUCCESS) ? 0 : -1;
> +}
> +
> +static void sasl_mutex_free_cb(void *mutex)
> +{
> + apr_thread_mutex_lock(array_mutex);
> + APR_ARRAY_PUSH(free_mutexes, apr_thread_mutex_t*) = mutex;
> + apr_thread_mutex_unlock(array_mutex);
> +}
> +
> +/* Pool cleanup called when sasl_pool is destroyed. */
> +apr_status_t sasl_done_cb(void *data)
> +{
> + /* Reset sasl_status, in case the client calls
> + apr_initialize()/apr_terminate() more than once. */
> + sasl_status = SASL_UNINITIALIZED;
> + sasl_done();
> + return APR_SUCCESS;
> +}
> +#else
> +static svn_boolean_t is_initialized = FALSE;
> +#endif /* APR_HAS_THREADS */
> +
> +apr_status_t svn_ra_svn__sasl_common_init()
> +{
> + apr_status_t apr_err = APR_SUCCESS;
> +
> +#ifdef APR_HAS_THREADS
> + sasl_set_mutex(sasl_mutex_alloc_cb,
> + sasl_mutex_lock_cb,
> + sasl_mutex_unlock_cb,
> + sasl_mutex_free_cb);
> + sasl_pool = svn_pool_create(NULL);
> + apr_pool_cleanup_register(sasl_pool, NULL, sasl_done_cb,
> + apr_pool_cleanup_null);
> + free_mutexes = apr_array_make(sasl_pool, 0, sizeof(apr_thread_mutex_t *));
> + apr_err = apr_thread_mutex_create(&array_mutex,
> + APR_THREAD_MUTEX_DEFAULT,
> + sasl_pool);
> +#else
> + atexit(sasl_done);
atexit still looks wrong, can this use the pool mechanism used by the
threaded case?
> +#endif /* APR_HAS_THREADS */
> + return apr_err;
> +}
> +
> +static sasl_callback_t interactions[] =
> +{
> + /* Use SASL interactions for username & password */
> + {SASL_CB_AUTHNAME, NULL, NULL},
> + {SASL_CB_PASS, NULL, NULL},
> + {SASL_CB_LIST_END, NULL, NULL}
> +};
> +
> +svn_error_t *svn_ra_svn__sasl_init()
> +{
> + /* We have to initialize the cache exactly once. Because APR
> + doesn't have statically-initialized mutexes, we implement a poor
> + man's spinlock using svn__atomic_cas. */
This still looks like code duplication, and it's tricky code at that.
How about some sort of svn_atomic_init_once(), give it a pointer to an
svn_atomic_t and some function callbacks and all the thread/cas/sleep
stuff would be hidden from the caller.
> +#ifdef APR_HAS_THREADS
> + svn__atomic_t status = svn__atomic_cas(&sasl_status,
> + SASL_START_INIT,
> + SASL_UNINITIALIZED);
> + if (status == SASL_UNINITIALIZED)
> + {
> +#else
> + if (!is_initialized)
> + {
> + is_initialized = TRUE;
> +#endif /* APR_HAS_THREADS */
> + if (svn_ra_svn__sasl_common_init() != APR_SUCCESS
> + || sasl_client_init(interactions) != SASL_OK)
> + {
> +#ifdef APR_HAS_THREADS
> + svn__atomic_cas(&sasl_status, SASL_INIT_FAILED, SASL_START_INIT);
> +#endif /* APR_HAS_THREADS */
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + _("Could not initialize the SASL library"));
> + }
> +#ifdef APR_HAS_THREADS
> + svn__atomic_cas(&sasl_status, SASL_INITIALIZED, SASL_START_INIT);
> +#endif /* APR_HAS_THREADS */
> + }
> +#ifdef APR_HAS_THREADS
> + /* Make sure that the other threads wait for
> + the initialization to finish */
> + else while (status != SASL_INITIALIZED)
> + {
> + if (status == SASL_INIT_FAILED)
> + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> + _("Could not initialize the SASL library"));
> + apr_sleep(APR_USEC_PER_SEC / 1000);
> + status = svn__atomic_cas(&sasl_status,
> + SASL_UNINITIALIZED,
> + SASL_UNINITIALIZED);
> + }
> +#endif /* APR_HAS_THREADS */
> + return SVN_NO_ERROR;
> +}
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Jul 30 13:30:23 2006