Re: svn commit: r1175903 - in /subversion/trunk/subversion: include/svn_mergeinfo.h libsvn_client/merge.c libsvn_repos/log.c libsvn_subr/deprecated.c libsvn_subr/mergeinfo.c

[Stefan Sperling <st...@elego.de>]

On Mon, Sep 26, 2011 at 03:15:21PM -0000, pburba@apache.org wrote:
> Author: pburba
> Date: Mon Sep 26 15:15:21 2011
> New Revision: 1175903
> URL: http://svn.apache.org/viewvc?rev=1175903&view=rev
> Log:
> Rev svn_mergeinfo_merge API to use the two-pool paradigm.

> * subversion/libsvn_subr/mergeinfo.c
> 
>   (svn_mergeinfo_merge2): New.

Hi Paul,

I think this commit introduced a pool lifetime issue. See below.

> ==============================================================================
> --- subversion/trunk/subversion/libsvn_subr/mergeinfo.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/mergeinfo.c Mon Sep 26 15:15:21 2011
> @@ -1320,8 +1320,10 @@ svn_mergeinfo__equals(svn_boolean_t *is_
>  }
>  
>  svn_error_t *
> -svn_mergeinfo_merge(svn_mergeinfo_t mergeinfo, svn_mergeinfo_t changes,
> -                    apr_pool_t *pool)
> +svn_mergeinfo_merge2(svn_mergeinfo_t mergeinfo,
> +                     svn_mergeinfo_t changes,
> +                     apr_pool_t *result_pool,
> +                     apr_pool_t *scratch_pool)
>  {
>    apr_array_header_t *sorted1, *sorted2;
>    int i, j;
> @@ -1330,12 +1332,14 @@ svn_mergeinfo_merge(svn_mergeinfo_t merg
>    if (!apr_hash_count(changes))
>      return SVN_NO_ERROR;
>  
> -  sorted1 = svn_sort__hash(mergeinfo, svn_sort_compare_items_as_paths, pool);
> -  sorted2 = svn_sort__hash(changes, svn_sort_compare_items_as_paths, pool);
> +  sorted1 = svn_sort__hash(mergeinfo, svn_sort_compare_items_as_paths,
> +                           scratch_pool);
> +  sorted2 = svn_sort__hash(changes, svn_sort_compare_items_as_paths,
> +                           scratch_pool);

Here, a temporary array of sorted keys is allocated in the scratch pool.

>  
>    i = 0;
>    j = 0;
> -  iterpool = svn_pool_create(pool);
> +  iterpool = svn_pool_create(scratch_pool);
>    while (i < sorted1->nelts && j < sorted2->nelts)
>      {
>        svn_sort__item_t elt1, elt2;
> @@ -1354,7 +1358,7 @@ svn_mergeinfo_merge(svn_mergeinfo_t merg
>            rl1 = elt1.value;
>            rl2 = elt2.value;

elt1 and elt2 are elements in the temporary array.

>  
> -          SVN_ERR(svn_rangelist_merge2(rl1, rl2, pool, iterpool));
> +          SVN_ERR(svn_rangelist_merge2(rl1, rl2, result_pool, iterpool));
>            apr_hash_set(mergeinfo, elt1.key, elt1.klen, rl1);

This sets the key pointer in the mergeinfo hash to data from elt1.
But the key has less lifetime than the hash itself, so this isn't safe.
Instead, the keys should be allocated in the same pool the mergeinfo
hash was allocated in.

Before your commit, they keys were stored in 'pool' which was treated
as a result pool.

The temporary keys are also used elsewhere in this function in the same
unsafe way.

I would suggest to aprpstr_dup() keys into the mergeinfo hash's pool.

Re: svn commit: r1175903 - in /subversion/trunk/subversion: include/svn_mergeinfo.h libsvn_client/merge.c libsvn_repos/log.c libsvn_subr/deprecated.c libsvn_subr/mergeinfo.c

[Stefan Sperling <st...@elego.de>]

On Wed, Sep 28, 2011 at 06:16:18PM +0200, Stefan Sperling wrote:
> On Mon, Sep 26, 2011 at 03:15:21PM -0000, pburba@apache.org wrote:
> > Author: pburba
> > Date: Mon Sep 26 15:15:21 2011
> > New Revision: 1175903
> > URL: http://svn.apache.org/viewvc?rev=1175903&view=rev
> > Log:
> > Rev svn_mergeinfo_merge API to use the two-pool paradigm.
> 
> > * subversion/libsvn_subr/mergeinfo.c
> > 
> >   (svn_mergeinfo_merge2): New.
> 
> Hi Paul,
> 
> I think this commit introduced a pool lifetime issue. See below.

Oh, nevermind. Looking closer at the implementation of svn_sort__hash(),
the key pointer in the temporary array has the same value as the key
pointer in the hash table. So this should be fine.