You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ju...@apache.org on 2010/03/17 10:37:11 UTC
svn commit: r924201 - in /subversion/trunk/subversion:
libsvn_client/mergeinfo.c libsvn_subr/mergeinfo.c
tests/libsvn_subr/mergeinfo-test.c
Author: julianfoad
Date: Wed Mar 17 09:37:11 2010
New Revision: 924201
URL: http://svn.apache.org/viewvc?rev=924201&view=rev
Log:
Tighten merge-range validation to not allow "change number r0" aka "revision
range -1:Y".
* subversion/libsvn_client/mergeinfo.c
(filter_log_entry_with_rangelist): Ignore revision 0 because that doesn't
represent a mergeable change.
* subversion/libsvn_subr/mergeinfo.c
(IS_VALID_FORWARD_RANGE): New macro.
(get_type_of_intersection): Use IS_VALID_FORWARD_RANGE() for tighter
validation of arguments than before: it previously accepted "change 0".
(range_intersect, range_contains): Validate arguments. Add doc strings.
* subversion/tests/libsvn_subr/mergeinfo-test.c
(randomly_fill_rev_array, rev_array_to_rangelist): Expand doc strings.
(test_rangelist_remove_randomly, test_rangelist_intersect_randomly): Don't
ever include change number r0 in a merge range.
Modified:
subversion/trunk/subversion/libsvn_client/mergeinfo.c
subversion/trunk/subversion/libsvn_subr/mergeinfo.c
subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c
Modified: subversion/trunk/subversion/libsvn_client/mergeinfo.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/mergeinfo.c?rev=924201&r1=924200&r2=924201&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/mergeinfo.c (original)
+++ subversion/trunk/subversion/libsvn_client/mergeinfo.c Wed Mar 17 09:37:11 2010
@@ -1296,7 +1296,12 @@ struct filter_log_entry_baton_t
};
/* Implements the svn_log_entry_receiver_t interface. BATON is a
- `struct filter_log_entry_baton_t *' */
+ `struct filter_log_entry_baton_t *'.
+
+ Call the wrapped log receiver BATON->log_receiver (with
+ BATON->log_receiver_baton), only if the log entry falls within the
+ ranges in BATON->rangelist.
+ */
static svn_error_t *
filter_log_entry_with_rangelist(void *baton,
svn_log_entry_t *log_entry,
@@ -1308,6 +1313,10 @@ filter_log_entry_with_rangelist(void *ba
if (fleb->ctx->cancel_func)
SVN_ERR(fleb->ctx->cancel_func(fleb->ctx->cancel_baton));
+ /* Ignore r0 because there can be no "change 0" in a merge range. */
+ if (log_entry->revision == 0)
+ return SVN_NO_ERROR;
+
this_rangelist = svn_rangelist__initialize(log_entry->revision - 1,
log_entry->revision,
TRUE, pool);
@@ -1436,7 +1445,7 @@ filter_log_entry_with_rangelist(void *ba
}
}
- /* Call the wrapped log reveiver which this function is filtering for. */
+ /* Call the wrapped log receiver which this function is filtering for. */
return fleb->log_receiver(fleb->log_receiver_baton, log_entry, pool);
}
Modified: subversion/trunk/subversion/libsvn_subr/mergeinfo.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/mergeinfo.c?rev=924201&r1=924200&r2=924201&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/mergeinfo.c (original)
+++ subversion/trunk/subversion/libsvn_subr/mergeinfo.c Wed Mar 17 09:37:11 2010
@@ -115,6 +115,16 @@ parse_pathname(const char **input,
return SVN_NO_ERROR;
}
+/* Return TRUE iff (svn_merge_range_t *) RANGE describes a valid, forward
+ * revision range.
+ *
+ * Note: The smallest valid value of RANGE->start is 0 because it is an
+ * exclusive endpoint, being one less than the revision number of the first
+ * change described by the range, and the oldest possible change is "r1" as
+ * there cannot be a change "r0". */
+#define IS_VALID_FORWARD_RANGE(range) \
+ (SVN_IS_VALID_REVNUM((range)->start) && ((range)->start < (range)->end))
+
/* Ways in which two svn_merge_range_t can intersect or adjoin, if at all. */
typedef enum
{
@@ -144,16 +154,8 @@ get_type_of_intersection(const svn_merge
{
SVN_ERR_ASSERT(r1);
SVN_ERR_ASSERT(r2);
-
- /* Why not use SVN_IS_VALID_REVNUM here? Because revision 0
- is described START = -1, END = 0. See svn_merge_range_t. */
- SVN_ERR_ASSERT(r1->start >= -1);
- SVN_ERR_ASSERT(r2->start >= -1);
-
- SVN_ERR_ASSERT(SVN_IS_VALID_REVNUM(r1->end));
- SVN_ERR_ASSERT(SVN_IS_VALID_REVNUM(r2->end));
- SVN_ERR_ASSERT(r1->start < r1->end);
- SVN_ERR_ASSERT(r2->start < r2->end);
+ SVN_ERR_ASSERT(IS_VALID_FORWARD_RANGE(r1));
+ SVN_ERR_ASSERT(IS_VALID_FORWARD_RANGE(r2));
if (!(r1->start <= r2->end && r2->start <= r1->end))
*intersection_type = svn__no_intersection;
@@ -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));
+
return (first->start + 1 <= second->end)
&& (second->start + 1 <= first->end)
&& (!consider_inheritance
|| (!(first->inheritable) == !(second->inheritable)));
}
+/* Return TRUE iff the forward revision range FIRST wholly contains the
+ * forward revision range SECOND and (if CONSIDER_INHERITANCE is TRUE) has
+ * the same inheritability. */
static svn_boolean_t
range_contains(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));
+
return (first->start <= second->start) && (second->end <= first->end)
&& (!consider_inheritance
|| (!(first->inheritable) == !(second->inheritable)));
Modified: subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c?rev=924201&r1=924200&r2=924201&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_subr/mergeinfo-test.c Wed Mar 17 09:37:11 2010
@@ -927,7 +927,8 @@ test_remove_rangelist(apr_pool_t *pool)
/* Random number seed. */
static apr_uint32_t random_rev_array_seed;
-/* Fill 3/4 of the array with 1s. */
+/* Set a random 3/4-ish of the elements of array REVS[RANDOM_REV_ARRAY_LENGTH]
+ * to TRUE and the rest to FALSE. */
static void
randomly_fill_rev_array(svn_boolean_t *revs)
{
@@ -939,6 +940,8 @@ randomly_fill_rev_array(svn_boolean_t *r
}
}
+/* Set *RANGELIST to a rangelist representing the revisions that are marked
+ * with TRUE in the array REVS[RANDOM_REV_ARRAY_LENGTH]. */
static svn_error_t *
rev_array_to_rangelist(apr_array_header_t **rangelist,
svn_boolean_t *revs,
@@ -993,6 +996,9 @@ test_rangelist_remove_randomly(apr_pool_
randomly_fill_rev_array(first_revs);
randomly_fill_rev_array(second_revs);
+ /* There is no change numbered "r0" */
+ first_revs[0] = FALSE;
+ second_revs[0] = FALSE;
for (j = 0; j < RANDOM_REV_ARRAY_LENGTH; j++)
expected_revs[j] = second_revs[j] && !first_revs[j];
@@ -1048,6 +1054,9 @@ test_rangelist_intersect_randomly(apr_po
randomly_fill_rev_array(first_revs);
randomly_fill_rev_array(second_revs);
+ /* There is no change numbered "r0" */
+ first_revs[0] = FALSE;
+ second_revs[0] = FALSE;
for (j = 0; j < RANDOM_REV_ARRAY_LENGTH; j++)
expected_revs[j] = second_revs[j] && first_revs[j];
Re: svn commit: r924201 - in /subversion/trunk/subversion:
libsvn_client/mergeinfo.c libsvn_subr/mergeinfo.c
tests/libsvn_subr/mergeinfo-test.c
Posted by Julian Foad <ju...@wandisco.com>.
Paul Burba wrote:
> On Thu, Mar 18, 2010 at 8:44 PM, Julian Foad <ju...@wandisco.com> wrote:
> > On Thu, 2010-03-18 at 17:34 -0400, Greg Stein wrote:
> >> 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:
Oops, I saw that at first and then forgot about it.
[...]
> Instead I would put a single check inside parse_rangelist:
OK, done (but explicitly testing for zero, similar to the other explicit
tests in this function) in r925290.
Thanks.
- Julian
Re: svn commit: r924201 - in /subversion/trunk/subversion:
libsvn_client/mergeinfo.c libsvn_subr/mergeinfo.c tests/libsvn_subr/mergeinfo-test.c
Posted by Paul Burba <pt...@gmail.com>.
On Thu, Mar 18, 2010 at 8:44 PM, Julian Foad <ju...@wandisco.com> wrote:
> On Thu, 2010-03-18 at 17:34 -0400, Greg Stein wrote:
>> On Wed, Mar 17, 2010 at 05:37, <ju...@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
Re: svn commit: r924201 - in /subversion/trunk/subversion:
libsvn_client/mergeinfo.c libsvn_subr/mergeinfo.c
tests/libsvn_subr/mergeinfo-test.c
Posted by Julian Foad <ju...@wandisco.com>.
On Thu, 2010-03-18 at 17:34 -0400, Greg Stein wrote:
> On Wed, Mar 17, 2010 at 05:37, <ju...@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()?
[[[
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
Re: svn commit: r924201 - in /subversion/trunk/subversion:
libsvn_client/mergeinfo.c libsvn_subr/mergeinfo.c tests/libsvn_subr/mergeinfo-test.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Mar 17, 2010 at 05:37, <ju...@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 )
>...
Cheers,
-g