You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@wandisco.com> on 2015/05/01 18:12:30 UTC
client SEGV with --enable-runtime-module-search
When dynamic module loading is enabled I get a SEGV
svnadmin create repo
svnadmin create repo2
svnsync initialize file://`pwd`/repo2 file://`pwd`/repo
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff76db4c5 in svn_mutex__lock (mutex=0xd1)
at ../src-1.9/subversion/libsvn_subr/mutex.c:84
84 apr_status_t status = apr_thread_mutex_lock(mutex->mutex);
(gdb) bt
#0 0x00007ffff76db4c5 in svn_mutex__lock (mutex=0xd1)
at ../src-1.9/subversion/libsvn_subr/mutex.c:84
#1 0x00007ffff4cd60ab in with_lock (baton=0x62a5b0, pool=0x621930)
at ../src-1.9/subversion/libsvn_fs_fs/fs_fs.c:239
#2 0x00007ffff4cd63c9 in svn_fs_fs__with_write_lock (fs=0x61fc40,
body=0x7ffff4cda15d <change_rev_prop_body>, baton=0x7fffffffdeb0,
pool=0x621930) at ../src-1.9/subversion/libsvn_fs_fs/fs_fs.c:365
#3 0x00007ffff4cda367 in svn_fs_fs__change_rev_prop (fs=0x61fc40, rev=0,
name=0x40a303 "svn:sync-lock", old_value_p=0x7fffffffdf70, value=0x628b00,
pool=0x621930) at ../src-1.9/subversion/libsvn_fs_fs/fs_fs.c:2144
#4 0x00007ffff51370f0 in svn_fs_change_rev_prop2 (fs=0x61fc40, rev=0,
name=0x40a303 "svn:sync-lock", old_value_p=0x7fffffffdf70, value=0x628b00,
pool=0x621930) at ../src-1.9/subversion/libsvn_fs/fs-loader.c:1577
The problem is that the shared struct created in fs_serialized_init
doesn't live long enough:
==25549== Invalid read of size 8
==25549== at 0x8102166: init_lock_baton (fs_fs.c:265)
==25549== by 0x81022D4: create_lock_baton (fs_fs.c:317)
==25549== by 0x81023B6: svn_fs_fs__with_write_lock (fs_fs.c:365)
==25549== by 0x8106366: svn_fs_fs__change_rev_prop (fs_fs.c:2144)
==25549== by 0x7CE00EF: svn_fs_change_rev_prop2 (fs-loader.c:1577)
==25549== by 0x7AA8CC2: svn_repos_fs_change_rev_prop4 (fs-wrap.c:404)
==25549== by 0x7887C7C: svn_ra_local__change_rev_prop (ra_plugin.c:750)
==25549== by 0x4E3DC5D: svn_ra_change_rev_prop2 (ra_loader.c:576)
==25549== by 0x4E409EE: svn_ra__get_operational_lock (util.c:250)
==25549== by 0x402A60: get_lock (svnsync.c:399)
==25549== by 0x402AEA: with_locked (svnsync.c:457)
==25549== by 0x403C7C: initialize_cmd (svnsync.c:909)
==25549== Address 0x74be778 is 24 bytes inside a block of size 56 free'd
==25549== at 0x4C29E90: free (vg_replace_malloc.c:473)
==25549== by 0x55C7096: pool_clear_debug (apr_pools.c:1576)
==25549== by 0x55C71EA: pool_destroy_debug (apr_pools.c:1638)
==25549== by 0x55C6FAA: pool_clear_debug (apr_pools.c:1550)
==25549== by 0x55C71EA: pool_destroy_debug (apr_pools.c:1638)
==25549== by 0x55C72D3: apr_pool_destroy_debug (apr_pools.c:1680)
==25549== by 0x4E3D696: svn_ra_open4 (ra_loader.c:431)
==25549== by 0x403F57: open_target_session (svnsync.c:996)
==25549== by 0x403BE6: initialize_cmd (svnsync.c:905)
==25549== by 0x407384: sub_main (svnsync.c:2380)
==25549== by 0x407463: main (svnsync.c:2414)
This is with 1.9.x.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: client SEGV with --enable-runtime-module-search
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@wandisco.com> writes:
> When dynamic module loading is enabled I get a SEGV
>
> svnadmin create repo
> svnadmin create repo2
> svnsync initialize file://`pwd`/repo2 file://`pwd`/repo
I found that because svnsync_authz_tests.py 7 fails over ra-svn as that
test also uses ra-local, however the bug affects most ra-local write
operations:
$ svnadmin create repo
$ svn mkdir -mm file://`pwd`/repo/A
Segmentation fault
--
Philip
Re: client SEGV with --enable-runtime-module-search
Posted by Branko Čibej <br...@wandisco.com>.
On 01.05.2015 18:44, Philip Martin wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
>> The problem is that the shared struct created in fs_serialized_init
>> doesn't live long enough:
>>
>> ==25549== Invalid read of size 8
>> ==25549== at 0x8102166: init_lock_baton (fs_fs.c:265)
>> ==25549== by 0x81022D4: create_lock_baton (fs_fs.c:317)
>> ==25549== by 0x81023B6: svn_fs_fs__with_write_lock (fs_fs.c:365)
>> ==25549== by 0x8106366: svn_fs_fs__change_rev_prop (fs_fs.c:2144)
>> ==25549== by 0x7CE00EF: svn_fs_change_rev_prop2 (fs-loader.c:1577)
>> ==25549== by 0x7AA8CC2: svn_repos_fs_change_rev_prop4 (fs-wrap.c:404)
>> ==25549== by 0x7887C7C: svn_ra_local__change_rev_prop (ra_plugin.c:750)
>> ==25549== by 0x4E3DC5D: svn_ra_change_rev_prop2 (ra_loader.c:576)
>> ==25549== by 0x4E409EE: svn_ra__get_operational_lock (util.c:250)
>> ==25549== by 0x402A60: get_lock (svnsync.c:399)
>> ==25549== by 0x402AEA: with_locked (svnsync.c:457)
>> ==25549== by 0x403C7C: initialize_cmd (svnsync.c:909)
>> ==25549== Address 0x74be778 is 24 bytes inside a block of size 56 free'd
>> ==25549== at 0x4C29E90: free (vg_replace_malloc.c:473)
>> ==25549== by 0x55C7096: pool_clear_debug (apr_pools.c:1576)
>> ==25549== by 0x55C71EA: pool_destroy_debug (apr_pools.c:1638)
>> ==25549== by 0x55C6FAA: pool_clear_debug (apr_pools.c:1550)
>> ==25549== by 0x55C71EA: pool_destroy_debug (apr_pools.c:1638)
>> ==25549== by 0x55C72D3: apr_pool_destroy_debug (apr_pools.c:1680)
>> ==25549== by 0x4E3D696: svn_ra_open4 (ra_loader.c:431)
>> ==25549== by 0x403F57: open_target_session (svnsync.c:996)
>> ==25549== by 0x403BE6: initialize_cmd (svnsync.c:905)
>> ==25549== by 0x407384: sub_main (svnsync.c:2380)
>> ==25549== by 0x407463: main (svnsync.c:2414)
>>
>> This is with 1.9.x.
> This worked in 1.8 and the breaking change is r1667829:
>
> Merge the r1664078 group from trunk:
>
> * r1664078,r1664080,r1664187,r1664191,r1664200,r1664344,r1664588,r1664927,r1665886
> Instead of making more changes to the auth batons from ra sessions, reduce
> the number of changes by introducing an internal slave auth baton feature.
> Justification:
> Without this patch (or a complete redesign of the auth layer), the
> ra sessions cache (currently on a feature branch), will open the ra
> sessions from outside configuration changes caused by opening other
> ra sessions. This patch not only reverts the additional changes to the
> auth baton on init that are new in 1.9, but also removes cases where we
> already applied similar changes inside specific ra providers.
> Notes:
> The reason I group this under release blockers, is to avoid the behavior
> change introduced in r1609499 from reaching released versions. The changes
> itself are safe for a later backport as it only affects ra-session
> internal state.
> Votes:
> +1: rhuijben, brane, philip
>
> in particular passing scratch_pool to the RA initfunc. Reverting to
> passing the parent sesspool like 1.8:
>
> Index: ../src-1.9/subversion/libsvn_ra/ra_loader.c
> ===================================================================
> --- ../src-1.9/subversion/libsvn_ra/ra_loader.c (revision 1677108)
> +++ ../src-1.9/subversion/libsvn_ra/ra_loader.c (working copy)
> @@ -355,7 +355,7 @@ svn_error_t *svn_ra_open4(svn_ra_session_t **sessi
> /* Library not found. */
> continue;
>
> - SVN_ERR(initfunc(svn_ra_version(), &vtable, scratch_pool));
> + SVN_ERR(initfunc(svn_ra_version(), &vtable, sesspool));
>
> SVN_ERR(check_ra_version(vtable->get_version(), scheme));
>
> allows the initialize to work again.
It gets worse, actually. The FSFS 'common pool' in which that shared
data is allocated is created as a subpool of whatever is sent to the
initfunc (in svn_fs_initialize). That's really broken IMO, because a
second session created from a different pool will get its FS struct
pulled from under it, if I'm reading the code correctly.
See libsvn_fs/fs-loader.c lines 390 to 420.
-- Brane
Re: client SEGV with --enable-runtime-module-search
Posted by Branko Čibej <br...@wandisco.com>.
On 01.05.2015 19:11, Philip Martin wrote:
> Philip Martin <ph...@wandisco.com> writes:
>
>> Philip Martin <ph...@wandisco.com> writes:
>>
>>> Index: ../src-1.9/subversion/libsvn_ra/ra_loader.c
>>> ===================================================================
>>> --- ../src-1.9/subversion/libsvn_ra/ra_loader.c (revision 1677108)
>>> +++ ../src-1.9/subversion/libsvn_ra/ra_loader.c (working copy)
>>> @@ -355,7 +355,7 @@ svn_error_t *svn_ra_open4(svn_ra_session_t **sessi
>>> /* Library not found. */
>>> continue;
>>>
>>> - SVN_ERR(initfunc(svn_ra_version(), &vtable, scratch_pool));
>>> + SVN_ERR(initfunc(svn_ra_version(), &vtable, sesspool));
>>>
>>> SVN_ERR(check_ra_version(vtable->get_version(), scheme));
>>>
>>> allows the initialize to work again.
>> For ra_local the initfunc is svn_ra_local__init and it contains a
>> comment:
>>
>> #ifndef SVN_LIBSVN_CLIENT_LINKS_RA_LOCAL
>> /* This assumes that POOL was the pool used to load the dso. */
>> SVN_ERR(svn_fs_initialize(pool));
>> #endif
>>
>> Passing scratch_pool breaks the assumption in the comment since
>> scratch_pool was not used to load the DSO.
> Perhaps something like this:
>
> Index: subversion/include/private/svn_subr_private.h
> ===================================================================
> --- subversion/include/private/svn_subr_private.h (revision 1677165)
> +++ subversion/include/private/svn_subr_private.h (working copy)
> @@ -681,6 +681,10 @@ svn_boolean_t
> svn_bit_array__get(svn_bit_array__t *array,
> apr_size_t idx);
>
> +/* Return the global pool used the DSO loader, this pool may be NULL. */
> +apr_pool_t *
> +svn_dso__pool(void);
> +
> /** @} */
>
> #ifdef __cplusplus
> Index: subversion/libsvn_ra_local/ra_plugin.c
> ===================================================================
> --- subversion/libsvn_ra_local/ra_plugin.c (revision 1677165)
> +++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
> @@ -1869,7 +1869,7 @@ svn_ra_local__init(const svn_version_t *loader_ver
>
> #ifndef SVN_LIBSVN_CLIENT_LINKS_RA_LOCAL
> /* This assumes that POOL was the pool used to load the dso. */
> - SVN_ERR(svn_fs_initialize(pool));
> + SVN_ERR(svn_fs_initialize(svn_dso__pool()));
> #endif
>
> *vtable = &ra_local_vtable;
> Index: subversion/libsvn_subr/dso.c
> ===================================================================
> --- subversion/libsvn_subr/dso.c (revision 1677165)
> +++ subversion/libsvn_subr/dso.c (working copy)
> @@ -122,4 +122,11 @@ svn_dso_load(apr_dso_handle_t **dso, const char *f
>
> return SVN_NO_ERROR;
> }
> +
> +apr_pool_t *
> +svn_dso__pool(void)
> +{
> + return dso_pool;
> +}
> +
> #endif /* APR_HAS_DSO */
This looks like a much better solution, yes. Strictly speaking, we'll
"leak" the shared FSFS structs into the DSO pool, but would have to use
a really unreasonable number of repositories from a single process
before this becomes a problem.
+1
-- Brane
Re: client SEGV with --enable-runtime-module-search
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@wandisco.com> writes:
> Philip Martin <ph...@wandisco.com> writes:
>
>> Index: ../src-1.9/subversion/libsvn_ra/ra_loader.c
>> ===================================================================
>> --- ../src-1.9/subversion/libsvn_ra/ra_loader.c (revision 1677108)
>> +++ ../src-1.9/subversion/libsvn_ra/ra_loader.c (working copy)
>> @@ -355,7 +355,7 @@ svn_error_t *svn_ra_open4(svn_ra_session_t **sessi
>> /* Library not found. */
>> continue;
>>
>> - SVN_ERR(initfunc(svn_ra_version(), &vtable, scratch_pool));
>> + SVN_ERR(initfunc(svn_ra_version(), &vtable, sesspool));
>>
>> SVN_ERR(check_ra_version(vtable->get_version(), scheme));
>>
>> allows the initialize to work again.
>
> For ra_local the initfunc is svn_ra_local__init and it contains a
> comment:
>
> #ifndef SVN_LIBSVN_CLIENT_LINKS_RA_LOCAL
> /* This assumes that POOL was the pool used to load the dso. */
> SVN_ERR(svn_fs_initialize(pool));
> #endif
>
> Passing scratch_pool breaks the assumption in the comment since
> scratch_pool was not used to load the DSO.
Perhaps something like this:
Index: subversion/include/private/svn_subr_private.h
===================================================================
--- subversion/include/private/svn_subr_private.h (revision 1677165)
+++ subversion/include/private/svn_subr_private.h (working copy)
@@ -681,6 +681,10 @@ svn_boolean_t
svn_bit_array__get(svn_bit_array__t *array,
apr_size_t idx);
+/* Return the global pool used the DSO loader, this pool may be NULL. */
+apr_pool_t *
+svn_dso__pool(void);
+
/** @} */
#ifdef __cplusplus
Index: subversion/libsvn_ra_local/ra_plugin.c
===================================================================
--- subversion/libsvn_ra_local/ra_plugin.c (revision 1677165)
+++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
@@ -1869,7 +1869,7 @@ svn_ra_local__init(const svn_version_t *loader_ver
#ifndef SVN_LIBSVN_CLIENT_LINKS_RA_LOCAL
/* This assumes that POOL was the pool used to load the dso. */
- SVN_ERR(svn_fs_initialize(pool));
+ SVN_ERR(svn_fs_initialize(svn_dso__pool()));
#endif
*vtable = &ra_local_vtable;
Index: subversion/libsvn_subr/dso.c
===================================================================
--- subversion/libsvn_subr/dso.c (revision 1677165)
+++ subversion/libsvn_subr/dso.c (working copy)
@@ -122,4 +122,11 @@ svn_dso_load(apr_dso_handle_t **dso, const char *f
return SVN_NO_ERROR;
}
+
+apr_pool_t *
+svn_dso__pool(void)
+{
+ return dso_pool;
+}
+
#endif /* APR_HAS_DSO */
--
Philip
Re: client SEGV with --enable-runtime-module-search
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> Index: ../src-1.9/subversion/libsvn_ra/ra_loader.c
> ===================================================================
> --- ../src-1.9/subversion/libsvn_ra/ra_loader.c (revision 1677108)
> +++ ../src-1.9/subversion/libsvn_ra/ra_loader.c (working copy)
> @@ -355,7 +355,7 @@ svn_error_t *svn_ra_open4(svn_ra_session_t **sessi
> /* Library not found. */
> continue;
>
> - SVN_ERR(initfunc(svn_ra_version(), &vtable, scratch_pool));
> + SVN_ERR(initfunc(svn_ra_version(), &vtable, sesspool));
>
> SVN_ERR(check_ra_version(vtable->get_version(), scheme));
>
> allows the initialize to work again.
For ra_local the initfunc is svn_ra_local__init and it contains a
comment:
#ifndef SVN_LIBSVN_CLIENT_LINKS_RA_LOCAL
/* This assumes that POOL was the pool used to load the dso. */
SVN_ERR(svn_fs_initialize(pool));
#endif
Passing scratch_pool breaks the assumption in the comment since
scratch_pool was not used to load the DSO.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: client SEGV with --enable-runtime-module-search
Posted by Philip Martin <ph...@wandisco.com>.
Philip Martin <ph...@wandisco.com> writes:
> The problem is that the shared struct created in fs_serialized_init
> doesn't live long enough:
>
> ==25549== Invalid read of size 8
> ==25549== at 0x8102166: init_lock_baton (fs_fs.c:265)
> ==25549== by 0x81022D4: create_lock_baton (fs_fs.c:317)
> ==25549== by 0x81023B6: svn_fs_fs__with_write_lock (fs_fs.c:365)
> ==25549== by 0x8106366: svn_fs_fs__change_rev_prop (fs_fs.c:2144)
> ==25549== by 0x7CE00EF: svn_fs_change_rev_prop2 (fs-loader.c:1577)
> ==25549== by 0x7AA8CC2: svn_repos_fs_change_rev_prop4 (fs-wrap.c:404)
> ==25549== by 0x7887C7C: svn_ra_local__change_rev_prop (ra_plugin.c:750)
> ==25549== by 0x4E3DC5D: svn_ra_change_rev_prop2 (ra_loader.c:576)
> ==25549== by 0x4E409EE: svn_ra__get_operational_lock (util.c:250)
> ==25549== by 0x402A60: get_lock (svnsync.c:399)
> ==25549== by 0x402AEA: with_locked (svnsync.c:457)
> ==25549== by 0x403C7C: initialize_cmd (svnsync.c:909)
> ==25549== Address 0x74be778 is 24 bytes inside a block of size 56 free'd
> ==25549== at 0x4C29E90: free (vg_replace_malloc.c:473)
> ==25549== by 0x55C7096: pool_clear_debug (apr_pools.c:1576)
> ==25549== by 0x55C71EA: pool_destroy_debug (apr_pools.c:1638)
> ==25549== by 0x55C6FAA: pool_clear_debug (apr_pools.c:1550)
> ==25549== by 0x55C71EA: pool_destroy_debug (apr_pools.c:1638)
> ==25549== by 0x55C72D3: apr_pool_destroy_debug (apr_pools.c:1680)
> ==25549== by 0x4E3D696: svn_ra_open4 (ra_loader.c:431)
> ==25549== by 0x403F57: open_target_session (svnsync.c:996)
> ==25549== by 0x403BE6: initialize_cmd (svnsync.c:905)
> ==25549== by 0x407384: sub_main (svnsync.c:2380)
> ==25549== by 0x407463: main (svnsync.c:2414)
>
> This is with 1.9.x.
This worked in 1.8 and the breaking change is r1667829:
Merge the r1664078 group from trunk:
* r1664078,r1664080,r1664187,r1664191,r1664200,r1664344,r1664588,r1664927,r1665886
Instead of making more changes to the auth batons from ra sessions, reduce
the number of changes by introducing an internal slave auth baton feature.
Justification:
Without this patch (or a complete redesign of the auth layer), the
ra sessions cache (currently on a feature branch), will open the ra
sessions from outside configuration changes caused by opening other
ra sessions. This patch not only reverts the additional changes to the
auth baton on init that are new in 1.9, but also removes cases where we
already applied similar changes inside specific ra providers.
Notes:
The reason I group this under release blockers, is to avoid the behavior
change introduced in r1609499 from reaching released versions. The changes
itself are safe for a later backport as it only affects ra-session
internal state.
Votes:
+1: rhuijben, brane, philip
in particular passing scratch_pool to the RA initfunc. Reverting to
passing the parent sesspool like 1.8:
Index: ../src-1.9/subversion/libsvn_ra/ra_loader.c
===================================================================
--- ../src-1.9/subversion/libsvn_ra/ra_loader.c (revision 1677108)
+++ ../src-1.9/subversion/libsvn_ra/ra_loader.c (working copy)
@@ -355,7 +355,7 @@ svn_error_t *svn_ra_open4(svn_ra_session_t **sessi
/* Library not found. */
continue;
- SVN_ERR(initfunc(svn_ra_version(), &vtable, scratch_pool));
+ SVN_ERR(initfunc(svn_ra_version(), &vtable, sesspool));
SVN_ERR(check_ra_version(vtable->get_version(), scheme));
allows the initialize to work again.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*