Re: [PATCH v2 4/4] apply: do not touch a file beyond a symbolic link

[Jeff King <peff@peff.net>]

On Mon, Feb 02, 2015 at 05:56:55PM -0800, Junio C Hamano wrote:

> > I think this means we'll be
> > overly cautious with a patch that does:
> >
> >   1. add foo as a symlink
> >
> >   2. remove foo
> >
> >   3. add foo/bar
> >
> > which is perfectly OK
> 
> No, such a patchset is broken.
> 
> A valid "git apply" input must *not* depend on the order of patches
> in it.  The consequence is that "an input to 'git apply' must not
> mention the fate of each path at most once."

Ah, right, I forgot we covered this already in the earlier discussion
(but thanks for elaborating; I think the reason I forgot is that I did
not really understand all of the implications).  If we do not have to
worry about that, then it's not a problem.

> >> +	/*
> >> +	 * An attempt to read from or delete a path that is beyond
> >> +	 * a symbolic link will be prevented by load_patch_target()
> >> +	 * that is called at the beginning of apply_data().  We need
> >> +	 * to make sure that the patch result is not deposited to
> >> +	 * a path that is beyond a symbolic link ourselves.
> >> +	 */
> >> +	if (!patch->is_delete && path_is_beyond_symlink(patch->new_name))
> >> +		return error(_("affected file '%s' is beyond a symbolic link"),
> >> +			     patch->new_name);
> >
> > Do we need to check the patch->is_delete case here (with patch->old_name)?
> 
> > I had a suspicion that the new patch 3/4 to check the reading-side might
> > help with that, but the comment here sounds like we do need to check
> > here, too
> 
> Hmm, the comment above was meant to tell you that we do not have to
> worry about the deletion case (because load_patch_target() will try
> to read the original to verify we are deleting what we expect to
> delete at the beginning of apply_data(), and it will notice that
> old_name is beyond a symbolic link), but we still need to check the
> non-deletion case.  Strictly speaking, modify-in-place case does not
> have to depend on this code (the same load_patch_target() check will
> catch it because it wants to check the preimage).
> 
> May need rephrasing to clarify but I thought it was clear enough.

Ah, OK. I totally misread it, thinking that load_patch_target was what
set up the symlink-table, and that was what you were referring to.  It
might be more clear after "...that is called at the beginning of
apply_data()" to add "...so we do not have to worry about that case
here".

-Peff