[darcs-devel] [patch2147] eliminate Darcs.Repository.HashedIO

[Ben Franksen]

Ben Franksen <ben.franksen at online.de> added the comment:

>>   * simplify createPristineDirectoryTree by reading and writing trees
> 
> I think the new code no longer checks 'isMaliciousSubPath'. I'm not 100%
> certain if it's a problem but I think it might be the place that clone
> is relying on to catch bad paths copied from remote repos.

I should have added a slightly longer comment to explain why all this is
no longer necessary.

The point is that nowadays all the validation is done when building
AnchoredPath, more specifically when create a Name. Thus, at the point
where we operate on a Tree, we can be sure that the paths in that tree
are non-malicious.

I think it was my latest index refactors that eliminated the last place
where we created a Name in an unsafe manner i.e. w/o validation. The
Name type is now abstract and there are no longer any unsafe functions
exported to build one.

The error handling when we encounter a bad name could still be improved.
For instance, decodeWhiteName throws a CorruptPatch exception (from pure
code). Apart from the naming (the thing at fault could also be a bad
pristine tree) it would be better to return an Either here and leave it
to the caller to fail with a more useful error message, including the
name of the corrupt file. I think I will send one or two patches to
clean that up, but not as a follow-up as it is pretty much unrelated.

> The new version also now does nothing for the NoWorkingDir case. That
> sort of makes sense but the old version deliberately evaluated the file
> anyway and had a comment to that effect. I think that's because it's
> ultimately calling 'fetchFileUsingCache' which can actually write into a
> local cache as it works. But I haven't dug into the details enough to be
> sure about this.

This is actually a bug, but not related to validation. The patch that
introduced that comment line was

patch 973cd2af4f30567c9f5b2e4f2a963358387daec3
Author: Guillaume Hoffmann <guillaumh at gmail.com>
Date:   Tue Apr 22 20:24:36 CEST 2014
  * resolve issue2365: correctly copy pristine in no-working-dir clones
  This patch makes Darcs.Repository.HashedIO.copyHashed take
  a flag that tells whether a clean working copy should be created
  while pristine is copied.

We do have a regression test for issue2365 and it succeeds, but this is
only because the whatsnew command in the test now /fetches/ the pristine
files from the repo, since it uses the cache now, and the source repo is
part of the cache (in the general sense i.e. set of hashed file
locations, which is independent of whether the /global/ cache is used or
not).

The test is easily fixed by renaming the source directory before the
whatsnew (you need to run the tests with -c=n to make it fail). And the
bug is fixed by reading and evaluating the tree, for the latter we can
use darcsAddMissingHashes.

Thanks for catching this! Will fix.

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch2147>
__________________________________