Re: [PATCH] Server-side Cyrus SASL support

On 8/24/06, Garrett Rooney <rooneg@electricjellyfish.net> wrote:
>
> Hey Vlad, I spent a fair amount of time looking at this today, and
> it's looking pretty good, but I think I found a problem in the
> existing sasl client code. I'm getting some strage deadlocks while
> running the svnsync tests, specifically test number 13, "detect
> non-svnsync commits in destination". The problem appears to be one of
> pool destruction ordering. We end up calling sasl_done before we
> destroy some of the pools that hold sasl contexts, so when sasl tries
> to use the mutex wrappers the pool has already been destroyed, so it
> tries to lock/unlock mutexes that are allocated in memory that's been
> reused. Oops.
>
> The following patch shows the problem:
> Index: subversion/libsvn_ra_svn/sasl_auth.c
> ===================================================================
> --- subversion/libsvn_ra_svn/sasl_auth.c (revision 21225)
> +++ subversion/libsvn_ra_svn/sasl_auth.c (working copy)
> @@ -40,6 +40,21 @@
> #include "ra_svn.h"
> #include "ra_svn_sasl.h"
>
> +static void
> +debug(const char *fmt, ...)
> +{
> + FILE *f = fopen("/tmp/debug.out", "a+");
> + va_list ap;
> +
> + fprintf(f, "%d ", getpid());
> +
> + va_start(ap, fmt);
> + vfprintf(f, fmt, ap);
> + va_end(ap);
> +
> + fclose(f);
> +}
> +
> static volatile svn_atomic_t sasl_status;
>
> static apr_pool_t *sasl_pool = NULL;
> @@ -47,6 +62,7 @@
> /* Pool cleanup called when sasl_pool is destroyed. */
> static apr_status_t sasl_done_cb(void *data)
> {
> + debug("sasl_done\n");
> /* Reset sasl_status, in case the client calls
> apr_initialize()/apr_terminate() more than once. */
> sasl_status = 0;
> @@ -76,6 +92,9 @@
> apr_thread_mutex_t *mutex;
> apr_status_t apr_err;
>
> + if (! sasl_status)
> + return NULL;
> +
> apr_err = apr_thread_mutex_lock(array_mutex);
> if (apr_err != APR_SUCCESS)
> return NULL;
> @@ -164,6 +183,7 @@
>
> static apr_status_t sasl_dispose_cb(void *data)
> {
> + debug("sasl_dispose_cb: %d\n", (int) data);
> sasl_conn_t *sasl_ctx = data;
> sasl_dispose(&sasl_ctx);
> return APR_SUCCESS;
>
> Run it with svnsync_tests.py --url=svn://localhost/ 13
>
> Then look in /tmp/debug.log, and you'll see the ordering problem. The
> if (! sasl_status) bit is a hack so that we don't do the bogus stuff,
> which avoids the deadlock, but obviously isn't the "right" solution.
> Good enough to see the debug trace though...
>
> I'm not entirely sure what the solution here is, need to think about
> it some more.
>
> -garrett
>

Hi Garrett,

Could you test the following patch?

It solves the problem by maintaining a count of how many sasl contexts
were created, and only calling sasl_done() when all of them are
destroyed (the count starts from 1, because we also decrement it in
sasl_done_cb). It seems to work for me (i.e. the test runs
successfully).

[[[
Ensure that sasl_done() is always called _after_ all the SASL contexts
are disposed of, regardless of the order in which sasl_pool and the
connection pool are destroyed.

* subversion/include/private/svn_atomic.h
  (svn_atomic_inc, svn_atomic_dec): New macro definitions.
* subversion/libsvn_ra_svn/sasl_auth.c
  (sasl_ctx_count): New static variable.
  (svn_ra_svn__sasl_common_init): Initialize sasl_ctx_count.
  (sasl_done_cb, sasl_dispose_cb): Decrement sasl_ctx_count. If it is
0, call sasl_done().
  (new_sasl_ctx): Increment sasl_ctx_count..
]]]

Index: subversion/include/private/svn_atomic.h
===================================================================
--- subversion/include/private/svn_atomic.h (revision 21257)
+++ subversion/include/private/svn_atomic.h (working copy)
@@ -60,6 +60,20 @@
 #define svn_atomic_set(mem, val) apr_atomic_set((mem), (val))
 #endif /* APR_MAJOR_VERSION */

+/** Atomically increment an #svn_atomic_t. */
+#if APR_MAJOR_VERSION > 0
+#define svn_atomic_inc(mem) apr_atomic_inc32(mem)
+#else
+#define svn_atomic_inc(mem) apr_atomic_inc(mem)
+#endif /* APR_MAJOR_VERSION */
+
+/** Atomically decrement an #svn_atomic_t. */
+#if APR_MAJOR_VERSION > 0
+#define svn_atomic_dec(mem) apr_atomic_dec32(mem)
+#else
+#define svn_atomic_dec(mem) apr_atomic_dec(mem)
+#endif /* APR_MAJOR_VERSION */
+
 /**
  * Atomic compare-and-swap.
  *
Index: subversion/libsvn_ra_svn/sasl_auth.c
===================================================================
--- subversion/libsvn_ra_svn/sasl_auth.c (revision 21257)
+++ subversion/libsvn_ra_svn/sasl_auth.c (working copy)
@@ -42,6 +42,8 @@

 static volatile svn_atomic_t sasl_status;

+static volatile svn_atomic_t sasl_ctx_count;
+
 static apr_pool_t *sasl_pool = NULL;

 /* Pool cleanup called when sasl_pool is destroyed. */
@@ -50,7 +52,8 @@
   /* Reset sasl_status, in case the client calls
      apr_initialize()/apr_terminate() more than once. */
   sasl_status = 0;
- sasl_done();
+ if (svn_atomic_dec(&sasl_ctx_count) == 0)
+ sasl_done();
   return APR_SUCCESS;
 }

@@ -124,6 +127,7 @@
   apr_status_t apr_err = APR_SUCCESS;

   sasl_pool = svn_pool_create(NULL);
+ sasl_ctx_count = 1;
   apr_pool_cleanup_register(sasl_pool, NULL, sasl_done_cb,
                             apr_pool_cleanup_null);
 #ifdef APR_HAS_THREADS
@@ -166,6 +170,8 @@
 {
   sasl_conn_t *sasl_ctx = data;
   sasl_dispose(&sasl_ctx);
+ if (svn_atomic_dec(&sasl_ctx_count) == 0)
+ sasl_done();
   return APR_SUCCESS;
 }

@@ -187,6 +193,7 @@
     return svn_error_create(SVN_ERR_RA_NOT_AUTHORIZED, NULL,
                             sasl_errstring(result, NULL, NULL));

+ svn_atomic_inc(&sasl_ctx_count);
   apr_pool_cleanup_register(pool, *sasl_ctx, sasl_dispose_cb,
                             apr_pool_cleanup_null);

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