[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r924201 - in /subversion/trunk/subversion: libsvn_client/mergeinfo.c libsvn_subr/mergeinfo.c tests/libsvn_subr/mergeinfo-test.c

From: Paul Burba <ptburba_at_gmail.com>
Date: Fri, 19 Mar 2010 11:02:21 -0400

On Thu, Mar 18, 2010 at 8:44 PM, Julian Foad <julian.foad_at_wandisco.com> wrote:
> On Thu, 2010-03-18 at 17:34 -0400, Greg Stein wrote:
>> On Wed, Mar 17, 2010 at 05:37,  <julianfoad_at_apache.org> wrote:
>> >...
>> > +++ subversion/trunk/subversion/libsvn_subr/mergeinfo.c Wed Mar 17 09:37:11 2010
>> >...
>> > @@ -763,20 +765,31 @@ svn_rangelist_merge(apr_array_header_t *
>> >   return SVN_NO_ERROR;
>> >  }
>> >
>> > +/* Return TRUE iff the forward revision ranges FIRST and SECOND overlap and
>> > + * (if CONSIDER_INHERITANCE is TRUE) have the same inheritability. */
>> >  static svn_boolean_t
>> >  range_intersect(const svn_merge_range_t *first, const svn_merge_range_t *second,
>> >                 svn_boolean_t consider_inheritance)
>> >  {
>> > +  SVN_ERR_ASSERT_NO_RETURN(IS_VALID_FORWARD_RANGE(first));
>> > +  SVN_ERR_ASSERT_NO_RETURN(IS_VALID_FORWARD_RANGE(second));
>>
>> If somebody manually tweaks an svn:mergeinfo property to include 0,
>> then is our library going to dump core?
>>
>> (I imagine props are checked much earlier, but a new core dump is
>> always something to question :-P )
>
> Good question.  Let's see where the mergeinfo prop values are parsed
> into svn_merge_range_t's.
>
> Paul, do you think I should add a check like the following to
> svn_mergeinfo_parse()?

Hi Julian,

Yes, I think we should do this. However, your patch won't work if
there is only a single revision or single range, because your two
checks are inside a "if (rangelist->nelts > 1)" block:

C:\SVN\src-trunk-2\Debug\subversion\tests\cmdline\svn-test-work\working_copies\merge_tests-142>svn
ps svn:mergeinfo /A:0-4,7 A_COPY
..\..\..\subversion\svn\propset-cmd.c:194: (apr_err=200020)
..\..\..\subversion\svn\util.c:960: (apr_err=200020)
..\..\..\subversion\libsvn_client\prop_commands.c:408: (apr_err=200020)
..\..\..\subversion\libsvn_wc\props.c:2119: (apr_err=200020)
..\..\..\subversion\libsvn_wc\props.c:2119: (apr_err=200020)
..\..\..\subversion\libsvn_wc\props.c:2330: (apr_err=200020)
..\..\..\subversion\libsvn_subr\mergeinfo.c:690: (apr_err=200020)
..\..\..\subversion\libsvn_subr\mergeinfo.c:621: (apr_err=200020)
svn: Bad range in mergeinfo: '0-4'
     ^^^^
     GOOD!

>svn ps svn:mergeinfo /A:0-4 A_COPY
property 'svn:mergeinfo' set on 'A_COPY'

>svn pl -v A_COPY
Properties on 'A_COPY':
  svn:mergeinfo
    /A:0-4
    ^^^^^^
    UH-OH!

Instead I would put a single check inside parse_rangelist:

[[[
Index: subversion/libsvn_subr/mergeinfo.c
===================================================================
--- subversion/libsvn_subr/mergeinfo.c (revision 925213)
+++ subversion/libsvn_subr/mergeinfo.c (working copy)
@@ -503,6 +503,11 @@
       mrange->end = firstrev;
       mrange->inheritable = TRUE;

+ if (! IS_VALID_FORWARD_RANGE(mrange))
+ return svn_error_createf(SVN_ERR_MERGEINFO_PARSE_ERROR, NULL,
+ _("Bad range in mergeinfo: '%s'"),
+ range_to_string(mrange, pool));
+
       if (*curr == '-')
         {
           svn_revnum_t secondrev;
]]]

Paul

> [[[
> Index: subversion/libsvn_subr/mergeinfo.c
> ===================================================================
> --- subversion/libsvn_subr/mergeinfo.c  (revision 924839)
> +++ subversion/libsvn_subr/mergeinfo.c  (working copy)
> @@ -611,10 +611,19 @@ parse_revision_line(const char **input,
>       qsort(rangelist->elts, rangelist->nelts, rangelist->elt_size,
>             svn_sort_compare_ranges);
>       lastrange = APR_ARRAY_IDX(rangelist, 0, svn_merge_range_t *);
> +      if (! IS_VALID_FORWARD_RANGE(lastrange))
> +        return svn_error_createf(SVN_ERR_MERGEINFO_PARSE_ERROR, NULL,
> +                                 _("Bad range in mergeinfo: '%s'"),
> +                                 range_to_string(lastrange, pool));
>
>       for (i = 1; i < rangelist->nelts; i++)
>         {
>           range = APR_ARRAY_IDX(rangelist, i, svn_merge_range_t *);
> +          if (! IS_VALID_FORWARD_RANGE(range))
> +            return svn_error_createf(SVN_ERR_MERGEINFO_PARSE_ERROR, NULL,
> +                                     _("Bad range in mergeinfo: '%s'"),
> +                                     range_to_string(range, pool));
> +
>           if (lastrange->start <= range->end
>               && range->start <= lastrange->end)
>             {
> ]]]
>
> - Julian
Received on 2010-03-19 16:02:50 CET

This is an archived mail posted to the Subversion Dev mailing list.