On 7/19/06, Garrett Rooney <rooneg@electricjellyfish.net> wrote:
> 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.
Fixed.
> + *((apr_thread_mutex_t**)apr_array_push(free_mutexes)) = mutex;
>
> APR_ARRAY_PUSH() would make that more readable.
True.
> 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.
I actually solved this problem by registering sasl_done itself
(actually a wrapper for sasl_done) as a pool cleanup. This guarantees
that sasl_done gets called before the global pool is destroyed.
> +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.
Fixed.
> + 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.
Fixed.
I attached a revised 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 Wed Jul 19 22:34:14 2006