Re: useful extra check in FS fold_change function

On Fri, Mar 19, 2010 at 10:26 AM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
> David Glasser wrote:
>> When dealing with some nasty cases in the almost-retired old Google
>> Code subversion backend, I found a kind of data corruption that the FS
>> code wasn't catching even though it caught relatively similar issues.
>> Specifically, find the fold_change function in both of the FS
>> implementations.  There's a check:
>>
>>       /* Sanity check: an add, replacement, or reset must be the first
>>          thing to follow a deletion. */
>>       if ((old_change->change_kind == svn_fs_path_change_delete)
>>           && (! ((change->kind == svn_fs_path_change_replace)
>>                  || (change->kind == svn_fs_path_change_reset)
>>                  || (change->kind == svn_fs_path_change_add))))
>>         return svn_error_create
>>           (SVN_ERR_FS_CORRUPT, NULL,
>>            _("Invalid change ordering: non-add change on deleted path"));
>>
>> It might also be nice to check the opposite condition: if change->kind
>> is add or replace, and old_change is not delete (or reset, I guess),
>> throw an error.  (We had accidentally recorded the sequence "add,
>> prop-mod, text-mod" out of order as "prop-mod, add, text-mod", which
>> was interpreted as an "R" below instead of an "A"; this suggested
>> check would have caught it earlier.)
>
> Shouldn't this be just: "if change->kind is add, and old_change is not
> delete, throw an error"?  It would be legit for someone to record the
> sequence "add, prop-mod, replace, text-mod", for example.
>
> Is the attached patch what you had in mind?  (Plus similar logic for FSFS,
> of course.)

Ah, yes, that's what I meant; that patch looks great, assuming it
works :) I would like to get a working Subversion development
environment back one of these days, once I re-derive how to work
around the Debian libtool issue again...

thanks,
dave

> --
> C. Michael Pilato <cmpilato_at_collab.net>
> CollabNet   <>   www.collab.net   <>   Distributed Development On Demand
>
> Index: subversion/libsvn_fs_base/bdb/changes-table.c
> ===================================================================
> --- subversion/libsvn_fs_base/bdb/changes-table.c       (revision 924936)
> +++ subversion/libsvn_fs_base/bdb/changes-table.c       (working copy)
> @@ -168,6 +168,15 @@
>           (SVN_ERR_FS_CORRUPT, NULL,
>            _("Invalid change ordering: non-add change on deleted path"));
>
> +      /* Sanity check: an add can't follow anything except
> +         a delete or reset.  */
> +      if ((change->kind == svn_fs_path_change_add)
> +          && (old_change->change_kind != svn_fs_path_change_delete)
> +          && (old_change->change_kind != svn_fs_path_change_reset))
> +        return svn_error_create
> +          (SVN_ERR_FS_CORRUPT, NULL,
> +           _("Invalid change ordering: add change on preexisting path"));
> +
>       /* Now, merge that change in. */
>       switch (change->kind)
>         {
>
>

-- 
glasser_at_davidglasser.net | langtonlabs.org | flickr.com/photos/glasser/