On 7/18/06, Vlad Georgescu <vgeorgescu@gmail.com> wrote:
> Hi,
>
> Please review and test the attached patch.
>
> [[[
> Add client-side support for Cyrus SASL.
>
> * configure.in: Define SVN_HAVE_SASL.
>
> * subversion/libsvn_ra_svn/client.c
> (svn_ra_svn__init): Call svn_ra_svn__sasl_init.
>
> * subversion/libsvn_ra_svn/ra_svn_sasl.h: New file.
>
> * subversion/libsvn_ra_svn/sasl_auth.c: New file.
>
> * subversion/libsvn_ra_svn/simple_auth.c:
> Enclose content within a #ifndef SVN_HAVE_SASL .. #endif pair.
>
> * subversion/libsvn_ra_svn/ra_svn.h
> (svn_ra_svn__sasl_init): New declaration.
> ]]]
A few comments:
+ apr_thread_mutex_t *mutex;
+ if (apr_is_empty_array(free_mutexes))
+ {
+ int r = apr_thread_mutex_create(&mutex,
+ APR_THREAD_MUTEX_DEFAULT,
+ sasl_pool);
+ if (r != APR_SUCCESS)
+ return NULL;
+ }
The r variabel should be an apr_status_t.
+ *((apr_thread_mutex_t**)apr_array_push(free_mutexes)) = mutex;
APR_ARRAY_PUSH() would make that more readable.
Also, for all of the mutex stuff I'm concerned about its behavior
during global pool destruction. I don't know if sasl tries to free
all its mutexes at shutdown, but if it does, and it runs after the
pool you allocated the mutexes out of is destroyed, then it's going to
access invalid memory. You should probably use the same sort of trick
as I added to the bdb global cache yesterday, where the pool has a
cleanup callback that sets some variable to NULL, and the functions
that access stuff inside the pool check if that variable is NULL
before doing so. That should be safe because that cleanup will only
get called in a single threaded context, during atexit processing.
+void svn_ra_svn__get_addresses(apr_socket_t *sock,
+ char **local_addrport,
+ char **remote_addrport,
+ apr_pool_t *pool)
We usually put the output parameters first in the argument order. And
since those strings are never actually modified, you could return
const char *s instead of char *s.
+ subpool = svn_pool_create(pool);
+
+ /* Create a string containing the list of mechanisms, separated by spaces. */
+ for (i = 0; i < mechlist->nelts; i++)
+ {
+ elt = &APR_ARRAY_IDX(mechlist, i, svn_ra_svn_item_t);
+
+ /* Force the client to use ANONYMOUS if it is available. */
+ if (strcmp(elt->u.word, "ANONYMOUS") == 0)
+ {
+ mechstring = "ANONYMOUS";
+ break;
+ }
+
+ mechstring = apr_pstrcat(subpool,
+ mechstring,
+ i == 0 ? "" : " ",
+ elt->u.word, NULL);
+ }
+
+ do
+ {
+ /* We shouldn't have to create a new SASL context for each
+ * authentication attempt, but it seems that sasl_client_start
+ * forgets to initialise some data structures. This means that
+ * the library will treat calls to sasl_client_step as if they were part
+ * of the previous auth exchange, which will obviously fail. */
+ SVN_ERR(new_sasl_ctx(&sasl_ctx, sess->is_tunneled,
+ hostname, local_addrport, remote_addrport,
+ pool));
+
+ SVN_ERR(try_auth(sess,
+ sasl_ctx,
+ &iterstate,
+ &success,
+ &last_err,
+ realm,
+ mechstring,
+ compat,
+ subpool));
+
+ /* Dispose of this SASL context before creating a new one. */
+ sasl_dispose(&sasl_ctx);
+ }
+ while (!success);
It's weird that you create a subpool here, then use it in a loop, but
never clear it. I'd expect mechstring to be allocated in pool, not
subpool, and subpool to be cleared at the top of the while loop.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jul 19 02:18:23 2006