On 01.05.2015 18:44, Philip Martin wrote:
> Philip Martin <philip.martin_at_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
Received on 2015-05-02 23:46:32 CEST