On 12.02.2015 16:58, Ivan Zhakov wrote:
> On 11 February 2015 at 20:48, <philip_at_apache.org> wrote:
>> Author: philip
>> Date: Wed Feb 11 17:48:06 2015
>> New Revision: 1659013
>>
>> URL: http://svn.apache.org/r1659013
>> Log:
>> Wrap svn_dso_initialize2 call with svn_atomic__init_once, this
>> fixes a crash in the DSO hash code when running the C tests in
>> parallel.
>>
>> * subversion/libsvn_subr/dso.c
>> (atomic_init_func): New.
>> (svn_dso_load): Use svn_atomic__init_once.
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_subr/dso.c
>>
>> Modified: subversion/trunk/subversion/libsvn_subr/dso.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/dso.c?rev=1659013&r1=1659012&r2=1659013&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_subr/dso.c (original)
>> +++ subversion/trunk/subversion/libsvn_subr/dso.c Wed Feb 11 17:48:06 2015
>> @@ -28,6 +28,7 @@
>> #include "svn_private_config.h"
>>
>> #include "private/svn_mutex.h"
>> +#include "private/svn_atomic.h"
>>
>> /* A mutex to protect our global pool and cache. */
>> static svn_mutex__t *dso_mutex = NULL;
>> @@ -41,6 +42,8 @@ static apr_hash_t *dso_cache;
>> /* Just an arbitrary location in memory... */
>> static int not_there_sentinel;
>>
>> +static volatile svn_atomic_t atomic_init_status = 0;
>> +
>> /* A specific value we store in the dso_cache to indicate that the
>> library wasn't found. This keeps us from allocating extra memory
>> from dso_pool when trying to find libraries we already know aren't
>> @@ -61,6 +64,14 @@ svn_dso_initialize2(void)
>> return SVN_NO_ERROR;
>> }
>>
>> +static svn_error_t *
>> +atomic_init_func(void *baton,
>> + apr_pool_t *pool)
>> +{
>> + SVN_ERR(svn_dso_initialize2());
>> + return SVN_NO_ERROR;
>> +}
>> +
>> #if APR_HAS_DSO
>> static svn_error_t *
>> svn_dso_load_internal(apr_dso_handle_t **dso, const char *fname)
>> @@ -107,8 +118,8 @@ svn_dso_load_internal(apr_dso_handle_t *
>> svn_error_t *
>> svn_dso_load(apr_dso_handle_t **dso, const char *fname)
>> {
>> - if (! dso_pool)
>> - SVN_ERR(svn_dso_initialize2());
>> + SVN_ERR(svn_atomic__init_once(&atomic_init_status, atomic_init_func,
>> + NULL, NULL));
>>
> Philip,
>
> The fix itself makes sense, but I agree with Evgeny that long-term fix
> should be add call to svn_dso_initialize2() in the test suite like we
> do in all application and mod_dav_svn.
>
> Also current fix doesn't protect from svn_dso_initialize2() executing
> concurrently if one thread call svn_dso_initialize2() directly, while
> other thread will call it indirectly through svn_dso_load().
>
> Why you didn't make svn_atomic__init_once() internal implementation
> details of svn_dso_initialize2()?
Yup, Ivan's right; the atomic init has to be hidden away within
svn_dso_initialize2's implementation, because this is a public function.
As to the other point, unfortunately I don't see an alternative to
adding explicit calls to dso-init just after apr_initialize() at this
point. Lack of control over global pool destruction is a serious flaw in
APR, but not one we can work around (in general) as long as we support
APR-1.3+.
-- Brane
Received on 2015-02-12 17:11:26 CET