[PATCH] validate: check for spurious incoming filelog entries

[Kevin Bullock]

On 11 Dec 2012, at 2:33 PM, Sune Foldager wrote:

> On 2012-12-11 14:22, Kevin Bullock wrote:
>> On 11 Dec 2012, at 1:15 PM, Sune Foldager wrote:
>> 
>>> # HG changeset patch
>>> # User Sune Foldager <cryo at cyanite.org>
>>> # Date 1355253201 -3600
>>> # Node ID 25a36cb85e7acda9901505832a186dfb883c451f
>>> # Parent  5522a7951bd7e2b16831ba1736feb2e9145d7e58
>>> validate: check for spurious incoming filelog entries
>>> 
>>> Accepting those will lead to "mild corruption", correctly reported as
>>> an error by hg verify, but often not a problem in practice.
>>> 
>>> Enabled when server.validate is switched on.
>> 
>> Do you have a case where it _is_ a problem in practice?
> 
> Well, server.validate is supposed to protect against corruption, and this is corruption. It is, for instance, a problem when you try to access the filelog (using hg log <filename>) for the file with spurious entries, as the linkrevs will be completely arbitrary (and always wrong).  Basically everything that looks at the filelog is affected.
> 
>> I'm generally +1 but want to be clearer on the possible impact. It won't really affect anyone who's running `hg verify` on their repos regularly, but that's likely to be the minority of our users.
> 
> No client should ever produce these changesets, but a malicious one might. Or an error in a plugin or similar.
> 
> For instance, due to a plugin error, we had a situation at work where a changeset with this problem was pushed to our server, and now everyone has it (and it's 100's of changesets from the tip). We have some tools that inspect the various graphs, talking directly to the mercurial python modules. These fail for the changesets involved, since the invariants that normally hold, don't in this case.

Seems like a good idea to reject such pushes then.

> diff -r 5522a7951bd7 -r 25a36cb85e7a tests/test-push-validation.t
> --- a/tests/test-push-validation.t	Mon Dec 10 12:14:55 2012 -0800
> +++ b/tests/test-push-validation.t	Tue Dec 11 20:13:21 2012 +0100
> @@ -18,7 +18,48 @@
>   updating to branch default
>   2 files updated, 0 files merged, 0 files removed, 0 files unresolved
> 
> +Test spurious filelog entries:
> +
>   $ cd test-clone
> +  $ echo blah >> beta
> +  $ cp .hg/store/data/beta.i tmp1

I had to squint to tell that this line is actually saving the filelog for the sake of the *next* (already existing) test. A comment here would be useful.

Aside from that, LGTM.

pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
Kevin R. Bullock