Thanks for the review!
On 7/20/06, Philip Martin <philip@codematters.co.uk> wrote:
> "Vlad Georgescu" <vgeorgescu@gmail.com> writes:
>
> > --- subversion/libsvn_ra_svn/ra_svn_sasl.h (revision 0)
> > +++ subversion/libsvn_ra_svn/ra_svn_sasl.h (revision 0)
> > @@ -0,0 +1,48 @@
> > +/*
> > + * ra_svn_sasl.h : SASL-related declarations shared between the
> > + * ra_svn and svnserve module
> > + *
> > + * ====================================================================
> > + * Copyright (c) 2000-2004 CollabNet. All rights reserved.
>
> Wrong date.
Ok, I fixed the copyright dates here, and also in sasl_auth.c.
> > +#define DEFAULT_SECPROPS {0, 256, 4096, 0, NULL, NULL}
>
> If you are going to put "magic" numbers into a header file it's a good
> idea to have a comment saying what they are and how you chose them.
I added a comment explaining these values.
> > +/* 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 */
> > +void svn_ra_svn__get_addresses(apr_socket_t *sock,
> > + char **local_addrport,
> > + char **remote_addrport,
> > + apr_pool_t *pool);
>
> Is it really impossible for this function to fail?
Fixed.
> > +
> > +/* An array of allocated, but unused, mutexes. */
>
> Mention the type of the elements in the array.
Done.
>
> > +static apr_array_header_t *free_mutexes = NULL;
> > +
> > +/* A mutex to serialize access to the array. */
> > +static apr_thread_mutex_t *array_mutex;
>
> = NULL for consistency with the two above.
Ok.
> > + }
> > + else
> > + {
> > + apr_thread_mutex_lock(array_mutex);
>
> A POSIX mutex won't usually fail, but ignoring errors is bad practice.
>
> > + mutex = *((apr_thread_mutex_t**)apr_array_pop(free_mutexes));
> > + apr_thread_mutex_unlock(array_mutex);
>
> ditto
Fixed.
> > +static int sasl_mutex_lock_cb(void *mutex)
> > +{
> > + if (mutex)
> > + return (apr_thread_mutex_lock(mutex) == APR_SUCCESS) ? 0 : -1;
> > + else
> > + return 0;
>
> Is returning success the right thing to do if the mutex doesn't exist?
I removed those checks. The mutex should never be NULL anyway.
> > +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);
> > +}
> > +#endif /* APR_HAS_THREADS */
> > +
> > +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}
> > +};
> > +
> > +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().
> Is it safe to call sasl_done at all? What if some other library is
> using SASL?
Cyrus SASL maintains a reference count of how many times sasl_{client,
server}_init and sasl_done were called. So this should be OK. However,
if that other library calls sasl_client_init() before us, and doesn't
use mutex callbacks (or uses callbacks that are incompatible with
APR's implementation), then things will stiil break, and there's not
much we can do about it.
> > +#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);
> > + 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);
> > +#endif /* APR_HAS_THREADS */
> > + return apr_err;
>
> It all looks a bit dodgy. Does it work if the application calls
> apr_init/apr_terminate more than once? Does sasl_done work if
> apr_terminate has deleted the pools?
(I think) I fixed these problems.
> > +}
> > +
> > +/* ### 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
> > +
> > +/* 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
> > +
> > +static volatile svn__atomic_t sasl_status = SASL_UNINITIALIZED;
> > +
> > +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 is obviously based on the BDB code. It would be much better if
> this code was shared somehow.
No argument from me...
> > + while (result == SASL_CONTINUE)
> > + {
> > + /* Read the server response */
> > + SVN_ERR(svn_ra_svn_read_tuple(sess->conn, pool, "w(?s)",
> > + &status, &in));
> > +
> > + if (strcmp(status, "failure") == 0)
> > + {
> > + /* Authentication failed. Use the next set of credentials */
> > + *success = FALSE;
> > + /* Remember the message sent by the server because we'll want to
> > + * return a meaningful error if we run out of auth providers. */
> > + *last_err = in ? in->data : "";
> > + return SVN_NO_ERROR;
> > + }
> > +
> > + if ((strcmp(status, "success") != 0 && strcmp(status, "step") != 0)
> > + || in == NULL)
> > + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> > + _("Unexpected server response"
> > + " to authentication"));
> > +
> > + /* If the mech is CRAM-MD5 we don't base64-decode the server response. */
> > + if (strcmp(mech, "CRAM-MD5") != 0)
> > + in = svn_base64_decode_string(in, pool);
> > +
> > + do
> > + {
> > + result = sasl_client_step(sasl_ctx,
> > + in->data,
> > + in->len,
> > + &client_interact,
> > + &out, /* Filled in by SASL. */
> > + &outlen);
> > +
> > + /* Fill in username and password, if required. */
> > + if (result == SASL_INTERACT)
> > + SVN_ERR(handle_interact(iterstate_p, sess, client_interact,
> > + realm, *last_err, pool));
> > + }
> > + while (result == SASL_INTERACT);
> > +
> > + if (result != SASL_OK && result != SASL_CONTINUE)
> > + return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
> > + sasl_errdetail(sasl_ctx));
> > +
> > + if (outlen > 0)
> > + {
> > + arg = svn_string_ncreate(out, outlen, pool);
> > + /* Write our response. */
> > + /* For CRAM-MD5, we don't use base64-encoding. */
> > + if (strcmp(mech, "CRAM-MD5") != 0)
> > + arg = svn_base64_encode_string(arg, pool);
> > + SVN_ERR(svn_ra_svn_write_cstring(sess->conn, pool, arg->data));
> > + }
> > + }
>
> Lots of loops, do you need subpools?
They're not unbounded.
I attached a new version of my patch.
Thanks,
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jul 20 19:38:43 2006