Re: [#4840] Merge assertion failure in svn_sort__array_insert

Now I have made some progress. The relevant commits are noted in the
issue tracker.

> Ill-defined canonical form for a rangelist.

Not yet tackled. It doesn't seem to be an immediate blocker.

> Rangelist "merge" is a set-union operation.

Not re-written yet. This is one of the next steps; see below.

> The function range_to_string(), bogus output of empty range

Fixed.

> Poor and inconsistent error handling in svn_sort__array_insert and
_delete.

Fixed. I changed all the "abort" code paths I could find in the area
(libsvn_subr/mergeinfo.c), to return catchable assertion errors.

> Our testing is clearly inadequate.

I have added thorough random-input testing for svn_rangelist_merge2().

It shows the bug of non-canonical output from canonical inputs (see below).

I investigated the next level up the call stack: svn_mergeinfo_merge2(),
and added a random-input crash test for it. It is a simple wrapper
around svn_rangelist_merge2() and given valid inputs there doesn't seem
to be any way it could invoke assertion failures (except for bugs in its
callees such as the "non-canonical output" bug just mentioned).

   * Other mergeinfo arithmetic functions still need random-input testing.

=== Next steps ===

   * Investigate further up the call stack in a similar way:
     combine_forked_mergeinfo_props()

   * Re-write svn_rangelist_merge2() as a neat little understandable
algorithm, thereby fixing the revealed bug (non-canonical output from
canonical inputs), now that it has decent regression tests.

(Does anybody else fancy taking on that last one? Sounds like a
potentially fun little algorithm rewrite task.)

- Julian
Received on 2019-12-30 18:28:13 CET