Re: [PATCH] apply: refuse touching a file beyond symlink

From: Jeff King <peff@peff.net>
Date: Jan 30, 2015, 20:07 UTC
Message-ID: <20150130200731.GC30738@peff.net>
In-Reply-To: <xmqq61bocao1.fsf@gitster.dls.corp.google.com>

On Fri, Jan 30, 2015 at 11:48:14AM -0800, Junio C Hamano wrote:

Show 33 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> >> +	if (!patch->is_delete && path_is_beyond_symlink(patch->new_name))
> >> +		return error(_("affected file '%s' is beyond a symbolic link"),
> >> +			     patch->new_name);
> >
> > Why does this not kick in when deleting a file? If it is not OK to
> > add across a symlink, why is it OK to delete?
> 
> Hmph, adding
> 
> 	if (patch->is_delete &&	path_is_beyond_symlink(patch->old_name))
> 		return error(_("deleted file '%s' is beyond a symlink"),
> 				patch->old_name);
> 
> seems to break t4114.11, which wants to apply this patch to a tree
> that does not have a symbolic link but a directory at 'foo/'.
> 
> diff --git a/foo b/foo
> new file mode 120000
> index 0000000..ba0e162
> --- /dev/null
> +++ b/foo
> @@ -0,0 +1 @@
> +bar
> \ No newline at end of file
> diff --git a/foo/baz b/foo/baz
> deleted file mode 100644
> index 682c76b..0000000
> --- a/foo/baz
> +++ /dev/null
> @@ -1 +0,0 @@
> -if only I knew

Hrm. That only works in the current code because we apply the deletion in the directory (and then clean up the now-empty directory) first. So I think you would need to check the paths progressively as you apply them, since those other parts of the diff "haven't happened yet".

If we take deletion as one phase and addition as another, I think you could get away with:

diff --git a/builtin/apply.c b/builtin/apply.c
index f5491cd..12c9d8e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3549,7 +3549,7 @@ static int check_to_create(const char *new_name, int ok_if_exists)
 	return 0;
 }
 
-static int path_is_beyond_symlink(const char *name_)
+static int path_is_beyond_symlink(const char *name_, int check_table)
 {
 	struct strbuf name = STRBUF_INIT;
 
@@ -3562,7 +3562,8 @@ static int path_is_beyond_symlink(const char *name_)
 		if (!name.len)
 			break;
 		name.buf[name.len] = '\0';
-		previous = in_fn_table(name.buf);
+
+		previous = check_table ? in_fn_table(name.buf) : NULL;
 		if (previous) {
 			if (!was_deleted(previous) &&
 			    !to_be_deleted(previous) &&
@@ -3676,9 +3677,12 @@ static int check_patch(struct patch *patch)
 		}
 	}
 
-	if (!patch->is_delete && path_is_beyond_symlink(patch->new_name))
+	if (!patch->is_delete && path_is_beyond_symlink(patch->new_name, 1))
 		return error(_("affected file '%s' is beyond a symbolic link"),
 			     patch->new_name);
+	if (patch->is_delete && path_is_beyond_symlink(patch->old_name, 0))
+		return error(_("affected file '%s' is beyond a symbolic link"),
+			     patch->old_name);
 
 	if (apply_data(patch, &st, ce) < 0)
 		return error(_("%s: patch does not apply"), name);


but I suspect we could construct a case that depends more closely on the
order of application. E.g., a patch that does:

  1. add foo as a symlink
  2. add file foo/bar

is definitely wrong. But is:

  1. add file foo/bar
  2. add foo as a symlink

does not technically fall afoul of the symlink rules. It is a _bogus_
patch, of course, because the second part will get a D/F conflict. I am
not sure if there are any legitimate patches that could run into this
ordering problem, but even without it, it smells a bit funny to complain
for the wrong reason.

Looking at the code, though, it seems like we should be doing these
checks progressively as we add entries to the fn_table. So that is doing
the right thing. It is only the deletion re-ordering that trips us up.
Can we reorder all deletions before all additions before calling
check_patch on each?

-Peff