patch-2.7.3 no longer applies relative symbolic link patches

patch-2.7.3 no longer applies relative symbolic link patches

[Josh Boyer]

Hi,

I went to do the Fedora 3.19-rc6 build this morning and it failed in
our buildsystem with:

+ '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'
+ case "$patch" in
+ unxz
+ patch -p1 -F1 -s
symbolic link target '../../../../../include/dt-bindings' is invalid
error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)

That is coming from the hunk in patch-3.19-rc6.xz that creates the
symbolic link from arch/arm64/boot/dts/include/dt-bindings to
include/dt-bindings.  Oddly enough, patch-3.19-rc5.xz contains the
same hunk and it built fine last week.

Digging in, it seems that upstream patch has decided that relative
symlinks are forbidden now as part of a fix for CVE-2015-1196.  You
can find the relevant bugs here:

https://bugzilla.redhat.com/show_bug.cgi?id=1185928
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=775901#13

Aside from locally modifying patch-3.19-rc6.xz, I'm not sure what else
to do.  I thought I would send a heads up since anyone that is using
patch-2.7.3 is probably going to run into this issue.

josh

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Josh Boyer]

[Adding Junio's correct email address.  Sigh.]

On Mon, Jan 26, 2015 at 11:29 AM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
> Hi,
>
> I went to do the Fedora 3.19-rc6 build this morning and it failed in
> our buildsystem with:
>
> + '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'
> + case "$patch" in
> + unxz
> + patch -p1 -F1 -s
> symbolic link target '../../../../../include/dt-bindings' is invalid
> error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)
>
> That is coming from the hunk in patch-3.19-rc6.xz that creates the
> symbolic link from arch/arm64/boot/dts/include/dt-bindings to
> include/dt-bindings.  Oddly enough, patch-3.19-rc5.xz contains the
> same hunk and it built fine last week.
>
> Digging in, it seems that upstream patch has decided that relative
> symlinks are forbidden now as part of a fix for CVE-2015-1196.  You
> can find the relevant bugs here:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1185928
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=775901#13
>
> Aside from locally modifying patch-3.19-rc6.xz, I'm not sure what else
> to do.  I thought I would send a heads up since anyone that is using
> patch-2.7.3 is probably going to run into this issue.
>
> josh

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Linus Torvalds]

On Mon, Jan 26, 2015 at 8:32 AM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
>
> I went to do the Fedora 3.19-rc6 build this morning and it failed in
> our buildsystem with:
>
> + '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'
> + case "$patch" in
> + unxz
> + patch -p1 -F1 -s
> symbolic link target '../../../../../include/dt-bindings' is invalid
> error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)

Ugh. I don't see anything we can do about this on the git side, and I
do kind of understand why 'patch' would be worried about '..' files.
In a perfect world, patch would parse the filename and see that it
stays within the directory structure of the project, but that is a
rather harder thing to do than just say "no dot-dot files".

The short-term fix is likely to just use "git apply" instead of "patch".

The long-term fix? I dunno. I don't see us not using symlinks, and a
quick check says that every *single* symlink we have in the kernel
source tree is one that points to a different directory using ".."
format. And while I could imagine that "patch" ends up counting the
dot-dot entries and checking that it's all inside the same tree it is
patching, I could also easily see patch *not* doing that. So using
"git apply" _might_ end up being the long-term fix too.

I suspect that if "patch" cannot apply even old-style kernel patches
due to the symlinks we have in the tree, and people end up having to
use "git apply" for them, I might end up starting to just use
rename-patches (ie using "git diff -M") for the kernel.

I've considered that for a while already, because "patch" _does_ kind
of understand them these days, although I think it gets the
cross-rename case wrong because it fundamentally works on a
file-by-file basis. But if "patch" just ends up not working at all,
the argument for trying to maintain backwards compatibility gets
really weak.

                                   Linus

Re: patch-2.7.3 no longer applies relative symbolic link patches

[David Kastrup]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Mon, Jan 26, 2015 at 8:32 AM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
>>
>> I went to do the Fedora 3.19-rc6 build this morning and it failed in
>> our buildsystem with:
>>
>> + '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'
>> + case "$patch" in
>> + unxz
>> + patch -p1 -F1 -s
>> symbolic link target '../../../../../include/dt-bindings' is invalid
>> error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)
>
> Ugh. I don't see anything we can do about this on the git side, and I
> do kind of understand why 'patch' would be worried about '..' files.
> In a perfect world, patch would parse the filename and see that it
> stays within the directory structure of the project, but that is a
> rather harder thing to do than just say "no dot-dot files".
>
> The short-term fix is likely to just use "git apply" instead of "patch".
>
> The long-term fix? I dunno. I don't see us not using symlinks, and a
> quick check says that every *single* symlink we have in the kernel
> source tree is one that points to a different directory using ".."
> format. And while I could imagine that "patch" ends up counting the
> dot-dot entries and checking that it's all inside the same tree it is
> patching, I could also easily see patch *not* doing that.

I consider it rather hard and error-prone and/or an attack vector to
choose a course of action for ../ in connection with the -p option.

-- 
David Kastrup

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Josh Boyer]

On Mon, Jan 26, 2015 at 3:44 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Jan 26, 2015 at 8:32 AM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
>>
>> I went to do the Fedora 3.19-rc6 build this morning and it failed in
>> our buildsystem with:
>>
>> + '[' '!' -f /builddir/build/SOURCES/patch-3.19-rc6.xz ']'
>> + case "$patch" in
>> + unxz
>> + patch -p1 -F1 -s
>> symbolic link target '../../../../../include/dt-bindings' is invalid
>> error: Bad exit status from /var/tmp/rpm-tmp.mWE3ZL (%prep)
>
> Ugh. I don't see anything we can do about this on the git side, and I
> do kind of understand why 'patch' would be worried about '..' files.
> In a perfect world, patch would parse the filename and see that it
> stays within the directory structure of the project, but that is a
> rather harder thing to do than just say "no dot-dot files".
>
> The short-term fix is likely to just use "git apply" instead of "patch".

Well, that's one fix anyway.  I just removed the hunk from the local
copy of patch-3.19-rc6.xz and added the symlink manually.  See why
below.

> The long-term fix? I dunno. I don't see us not using symlinks, and a
> quick check says that every *single* symlink we have in the kernel
> source tree is one that points to a different directory using ".."
> format. And while I could imagine that "patch" ends up counting the
> dot-dot entries and checking that it's all inside the same tree it is
> patching, I could also easily see patch *not* doing that. So using
> "git apply" _might_ end up being the long-term fix too.

It could, but from a distro perspective that requires either doing
'untar linux-3.N.tar.xz; cd linux-3.N; git add .; git apply
patch-3.N+1-rcX' , or just using a git tree to begin with, which then
makes all of this unnecessary anyway.  Creating a git repo from a
tarball for each build is kind of silly.  Some might say not just
using a git tree to build from to begin with in 2015 is also kind of
silly.

Or did I miss a way that git-apply can take a git patch and apply it
to a tree that isn't a git repo?

> I suspect that if "patch" cannot apply even old-style kernel patches
> due to the symlinks we have in the tree, and people end up having to
> use "git apply" for them, I might end up starting to just use
> rename-patches (ie using "git diff -M") for the kernel.

I'm kind of wondering why we'd generate patches at all if you have to
apply them to a git repo, but maybe people like doing things the
old-fashioned way just for the hell of it.

> I've considered that for a while already, because "patch" _does_ kind
> of understand them these days, although I think it gets the
> cross-rename case wrong because it fundamentally works on a
> file-by-file basis. But if "patch" just ends up not working at all,
> the argument for trying to maintain backwards compatibility gets
> really weak.

Yeah.  I mostly wanted to give people a heads up on the issue.  I'm
sure it's going to impact more than just the kernel.  I think for us
it's mostly limited to the -rcX patches, because once the tarball for
the final release is out the symlink should be created by tar just
fine.

josh

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Linus Torvalds]

On Mon, Jan 26, 2015 at 1:07 PM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
>
> Or did I miss a way that git-apply can take a git patch and apply it
> to a tree that isn't a git repo?

Exactly. "git apply" works as a straight "patch" replacement outside
of a git repository. It doesn't actually need a git tree to work.

(Of course, "git apply" is _not_ a "patch" replacement in the general
sense. It only applies context diffs - preferentially git style ones -
 so no old-style patches etc need apply. And it's not
replacement-compatible in a syntax sense either, in that while many of
the options are the same, not all are etc etc).

                               Linus

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Junio C Hamano]

On Mon, Jan 26, 2015 at 1:30 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Jan 26, 2015 at 1:07 PM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
>>
>> Or did I miss a way that git-apply can take a git patch and apply it
>> to a tree that isn't a git repo?
>
> Exactly. "git apply" works as a straight "patch" replacement outside
> of a git repository. It doesn't actually need a git tree to work.

What is your take on CVE-2015-1196, which brought this /regression/ to
GNU patch?
If "git apply" get /fixed/ for that same CVE, would that /break/ your fix?

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Linus Torvalds]

On Mon, Jan 26, 2015 at 1:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> What is your take on CVE-2015-1196, which brought this /regression/ to
> GNU patch?
> If "git apply" get /fixed/ for that same CVE, would that /break/ your fix?

I _think_ we allow arbitrary symlinks to be created, but then we
should be careful about actually _following_ them.

At least I _thought_ we were already quite careful not to do that,
even if it's been a long time since I looked at the code. So even if
we create a symlink to outside the repository, it normally shouldn't
matter. We have that whole "lstat_cache()" thing that exists exactly
to make it efficient to do pathname lookups while at the same time
being aware of symlinks in the middle.

Of course, our lstat cache is racy if somebody else modifies the tree
concurrently and changes things, but that's a non-issue, because if
somebody can just directly create random symlinks in the middle of the
tree, I don't think we care about any symlinks _git_ might be creating
concurrently ;)

But it is entirely possible that "git apply" - especially when used
outside of a real git directory - ends up doing that. And it's not
like we necessarily always use the whole "lstat-cache" mechanism to
begin with, so the fact that we have the infrastructure to be careful
in no way means that we necessarily always _are_ careful...

                                 Linus

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Andreas Gruenbacher]

On Mon, 26 Jan 2015 13:50:10 -0800, Linus Torvalds wrote:

> On Mon, Jan 26, 2015 at 1:35 PM, Junio C Hamano <gitster@pobox.com>
> wrote:
>>
>> What is your take on CVE-2015-1196, which brought this /regression/ to
>> GNU patch?
>> If "git apply" get /fixed/ for that same CVE, would that /break/ your
>> fix?
> 
> I _think_ we allow arbitrary symlinks to be created, but then we should
> be careful about actually _following_ them.

I would prefer to allow arbitrary symlinks even in GNU patch, but patch 
still must not be allowed to leave the working directory. The only way to 
achieve that I can think of is to implement path traversal in user space, 
which is not so easy to do correctly and efficiently.

I think file system modifications from "outside" are not much of a 
concern.

Andreas

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Andreas Gruenbacher]

On Tue, 27 Jan 2015 15:47:04 +0000, Andreas Gruenbacher wrote:
> On Mon, 26 Jan 2015 13:50:10 -0800, Linus Torvalds wrote:
>> I _think_ we allow arbitrary symlinks to be created, but then we should
>> be careful about actually _following_ them.
> 
> I would prefer to allow arbitrary symlinks even in GNU patch, but patch
> still must not be allowed to leave the working directory. The only way
> to achieve that I can think of is to implement path traversal in user
> space, which is not so easy to do correctly and efficiently.

This should be working in patch-2.7.4 now.

Andreas

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Josh Boyer]

On Mon, Jan 26, 2015 at 4:30 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Jan 26, 2015 at 1:07 PM, Josh Boyer <jwboyer@fedoraproject.org> wrote:
>>
>> Or did I miss a way that git-apply can take a git patch and apply it
>> to a tree that isn't a git repo?
>
> Exactly. "git apply" works as a straight "patch" replacement outside
> of a git repository. It doesn't actually need a git tree to work.

Ah.  I had somehow missed that entirely.  Good to know for future reference.

> (Of course, "git apply" is _not_ a "patch" replacement in the general
> sense. It only applies context diffs - preferentially git style ones -
>  so no old-style patches etc need apply. And it's not
> replacement-compatible in a syntax sense either, in that while many of
> the options are the same, not all are etc etc).

Sure.  Though for the Fedora kernel builds, we tend to use git
formatted patches only anyway.  I might play around with this and see
how it works as the normal way to apply things.

josh

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Ugh. I don't see anything we can do about this on the git side, and I
> do kind of understand why 'patch' would be worried about '..' files.
> In a perfect world, patch would parse the filename and see that it
> stays within the directory structure of the project, but that is a
> rather harder thing to do than just say "no dot-dot files".

It is unclear to me why "limit to the current directory and below"
is such a big deal in the first place.

If the user wants to apply a patch that touches ../etc/shadow, is
the tool in the place to complain?"

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> Ugh. I don't see anything we can do about this on the git side, and I
>> do kind of understand why 'patch' would be worried about '..' files.
>> In a perfect world, patch would parse the filename and see that it
>> stays within the directory structure of the project, but that is a
>> rather harder thing to do than just say "no dot-dot files".
>
> It is unclear to me why "limit to the current directory and below"
> is such a big deal in the first place.
>
> If the user wants to apply a patch that touches ../etc/shadow, is
> the tool in the place to complain?"

Let me take this part back.

I think "git apply" should behave closely to "git apply --index"
(which is used by "git am" unless there is a very good reason not to
(and "'git apply --index' behaves differently from GNU patch, and we
should match what the latter does" is not a very good reason).  When
the index guards the working tree, we do not follow any symlink,
whether the destination is inside the current directory or not.

I however do not think the current "git apply" notices that it will
overwrite a path beyond a symlink---we may need to fix that if that
is the case.  I'll see what I can find (but I'll be doing 2.3-rc2
today so it may be later this week).

Thanks.

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> If the user wants to apply a patch that touches ../etc/shadow, is
>> the tool in the place to complain?"
>
> Let me take this part back.
>
> I think "git apply" should behave closely to "git apply --index"
> (which is used by "git am" unless there is a very good reason not to
> (and "'git apply --index' behaves differently from GNU patch, and we
> should match what the latter does" is not a very good reason).  When
> the index guards the working tree, we do not follow any symlink,
> whether the destination is inside the current directory or not.
>
> I however do not think the current "git apply" notices that it will
> overwrite a path beyond a symlink---we may need to fix that if that
> is the case.  I'll see what I can find (but I'll be doing 2.3-rc2
> today so it may be later this week).

Yikes.  It turns out that the index is what protects us from going
outside the working tree.  "apply --index" (hence "am") is immune
against the CVE-2015-1196, but that is not because we do not follow
symbolic links.

Also the solution is not just a simple has_symlink_leading_path().
Here is tonight's snapshot of what I've found out (not tested beyond
passing the test suite including the new test added by the patch).

-- >8 --
Subject: [PATCH] apply: refuse touching a file beyond symlink

Because Git tracks symbolic links as symbolic links, a path that has
a symbolic link in its leading part (e.g. path/to/dir being a
symbolic link to somewhere else, be it inside or outside the working
tree) can never appear in a patch that validly apply, unless the
same patch first removes the symbolic link.

Detect and reject such a patch.  Things to note:

 - Unfortunately, we cannot reuse the has_symlink_leading_path()
   from dir.c, as that is only about the working tree, but "git
   apply" can be told to apply the patch only to the index.

 - We cannot directly use has_symlink_leading_path() even when we
   are applying to the working tree, as an early patch of a valid
   input may remove a symbolic link path/to/dir and then a later
   patch of the input may create a path path/to/dir/file.  The
   leading symbolic link check must be done on the interim result we
   compute in core (i.e. after the first patch, there is no
   path/to/dir symbolic link and it is perfectly valid to create
   path/to/dir/file).  Similarly, when an input creates a symbolic
   link path/to/dir and then creates a file path/to/dir/file, we
   need to flag it as an error without actually creating path/to/dir
   symbolic link in the filesystem.

 - Instead, for any patch in the input that leaves a path (i.e. a
   non deletion) in the result, we check all leading paths against
   interim result and then either the index or the working tree.
   The interim result of applying patch is already kept track of
   by fn_table logic for us.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c                 | 44 +++++++++++++++++++++++++++++++++++++++++
 t/t4122-apply-symlink-inside.sh | 37 ++++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/builtin/apply.c b/builtin/apply.c
index ef32e4f..da088c5 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3483,6 +3483,46 @@ static int check_to_create(const char *new_name, int ok_if_exists)
 	return 0;
 }
 
+static int path_is_beyond_symlink(const char *name_)
+{
+	struct strbuf name = STRBUF_INIT;
+
+	strbuf_addstr(&name, name_);
+	do {
+		struct patch *previous;
+
+		while (--name.len && name.buf[name.len] != '/')
+			; /* scan backwards */
+		if (!name.len)
+			break;
+		name.buf[name.len] = '\0';
+		previous = in_fn_table(name.buf);
+		if (previous) {
+			if (!was_deleted(previous) &&
+			    !to_be_deleted(previous) &&
+			    previous->new_mode &&
+			    S_ISLNK(previous->new_mode))
+				goto symlink_found;
+		} else if (check_index) {
+			int pos = cache_name_pos(name.buf, name.len);
+			if (0 <= pos &&
+			    S_ISLNK(active_cache[pos]->ce_mode))
+				goto symlink_found;
+		} else {
+			struct stat st;
+			if (!lstat(name.buf, &st) && S_ISLNK(st.st_mode))
+				goto symlink_found;
+		}
+	} while (1);
+
+	strbuf_release(&name);
+	return 0;
+symlink_found:
+	strbuf_release(&name);
+	return -1;
+
+}
+
 /*
  * Check and apply the patch in-core; leave the result in patch->result
  * for the caller to write it out to the final destination.
@@ -3570,6 +3610,10 @@ static int check_patch(struct patch *patch)
 		}
 	}
 
+	if (!patch->is_delete && path_is_beyond_symlink(patch->new_name))
+		return error(_("affected file '%s' is beyond a symbolic link"),
+			     patch->new_name);
+
 	if (apply_data(patch, &st, ce) < 0)
 		return error(_("%s: patch does not apply"), name);
 	patch->rejected = 0;
diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 70b3a06..8b11bc6 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -52,4 +52,41 @@ test_expect_success 'check result' '
 
 '
 
+test_expect_success 'do not follow symbolic link' '
+
+	git reset --hard &&
+	test_ln_s_add ../i386/dir arch/x86_64/dir &&
+	git diff HEAD >add_symlink.patch &&
+	git reset --hard &&
+
+	mkdir arch/x86_64/dir &&
+	>arch/x86_64/dir/file &&
+	git add arch/x86_64/dir/file &&
+	git diff HEAD >add_file.patch &&
+	git reset --hard &&
+	rm -fr arch/x86_64/dir &&
+
+	cat add_symlink.patch add_file.patch >patch &&
+
+	mkdir arch/i386/dir &&
+
+	test_must_fail git apply patch 2>error-wt &&
+	test_i18ngrep "beyond a symbolic link" error-wt &&
+	test ! -e arch/x86_64/dir &&
+	test ! -e arch/i386/dir/file &&
+
+	test_must_fail git apply --index patch 2>error-ix &&
+	test_i18ngrep "beyond a symbolic link" error-ix &&
+	test ! -e arch/x86_64/dir &&
+	test ! -e arch/i386/dir/file &&
+	test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+	test_must_fail git ls-files --error-unmatch arch/i386/dir &&
+
+	test_must_fail git apply --cached patch 2>error-ct &&
+	test_i18ngrep "beyond a symbolic link" error-ct &&
+	test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+	test_must_fail git ls-files --error-unmatch arch/i386/dir
+
+'
+
 test_done
-- 
2.3.0-rc2-149-gdd42ee9

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Subject: [PATCH] apply: refuse touching a file beyond symlink
>
> Because Git tracks symbolic links as symbolic links, a path that has
> a symbolic link in its leading part (e.g. path/to/dir being a
> symbolic link to somewhere else, be it inside or outside the working
> tree) can never appear in a patch that validly apply, unless the
> same patch first removes the symbolic link.

I should rephrase the above to make it more readable.

    ... its leading part (e.g. path/to/dir/file, where path/to/dir is a
    symbolic link to somewhere else, ...

is what I meant to say.

[PATCH] apply: refuse touching a file beyond symlink

[Junio C Hamano]

Because Git tracks symbolic links as symbolic links, a path that has
a symbolic link in its leading part (e.g. path/to/dir/file, where
path/to/dir is a symbolic link to somewhere else, be it inside or
outside the working tree) can never appear in a patch that validly
applies, unless the same patch first removes the symbolic link to
allow a directory to be there.

Detect and reject such a patch.  Things to note:

 - Unfortunately, we cannot reuse the has_symlink_leading_path()
   from dir.c, as that is only about the working tree, but "git
   apply" can be told to apply the patch only to the index or to
   both the index and to the working tree.

 - We cannot directly use has_symlink_leading_path() even when we
   are applying only to the working tree, as an early patch of a
   valid input may remove a symbolic link path/to/dir and then a
   later patch of the input may create a path path/to/dir/file, but
   "git apply" first checks the input without touching either the
   index or the working tree.  The leading symbolic link check must
   be done on the interim result we compute in-core (i.e. after the
   first patch, there is no path/to/dir symbolic link and it is
   perfectly valid to create path/to/dir/file).

   Similarly, when an input creates a symbolic link path/to/dir and
   then creates a file path/to/dir/file, we need to flag it as an
   error without actually creating path/to/dir symbolic link in the
   filesystem.

Instead, for any patch in the input that leaves a path (i.e. a non
deletion) in the result, we check all leading paths against interim
result and then either the index or the working tree.  The interim
results of applying patches are kept track of by fn_table logic for
us already, so use it to fiture out if existing a symbolic link will
cause problems, if a new symbolic link that will cause problems will
appear, etc.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * At least I convinced myself enough to say that I do not seem to
   be breaking things with this patch, after taking patches out of
   dozens of random pairs of commits from the Linux kernel history
   and applying them using this version ;-) No code change since
   last night's snapshot, but the test script is a bit more thorough
   in this version.

 builtin/apply.c                 | 44 +++++++++++++++++++++++++++++
 t/t4122-apply-symlink-inside.sh | 62 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+)

diff --git a/builtin/apply.c b/builtin/apply.c
index ef32e4f..dcb44fb 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3483,6 +3483,46 @@ static int check_to_create(const char *new_name, int ok_if_exists)
 	return 0;
 }
 
+static int path_is_beyond_symlink(const char *name_)
+{
+	struct strbuf name = STRBUF_INIT;
+
+	strbuf_addstr(&name, name_);
+	do {
+		struct patch *previous;
+
+		while (--name.len && name.buf[name.len] != '/')
+			; /* scan backwards */
+		if (!name.len)
+			break;
+		name.buf[name.len] = '\0';
+		previous = in_fn_table(name.buf);
+		if (previous) {
+			if (!was_deleted(previous) &&
+			    !to_be_deleted(previous) &&
+			    previous->new_mode &&
+			    S_ISLNK(previous->new_mode))
+				goto symlink_found;
+		} else if (check_index) {
+			int pos = cache_name_pos(name.buf, name.len);
+			if (0 <= pos &&
+			    S_ISLNK(active_cache[pos]->ce_mode))
+				goto symlink_found;
+		} else {
+			struct stat st;
+			if (!lstat(name.buf, &st) && S_ISLNK(st.st_mode))
+				goto symlink_found;
+		}
+	} while (1);
+
+	strbuf_release(&name);
+	return 0;
+symlink_found:
+	strbuf_release(&name);
+	return 1;
+
+}
+
 /*
  * Check and apply the patch in-core; leave the result in patch->result
  * for the caller to write it out to the final destination.
@@ -3570,6 +3610,10 @@ static int check_patch(struct patch *patch)
 		}
 	}
 
+	if (!patch->is_delete && path_is_beyond_symlink(patch->new_name))
+		return error(_("affected file '%s' is beyond a symbolic link"),
+			     patch->new_name);
+
 	if (apply_data(patch, &st, ce) < 0)
 		return error(_("%s: patch does not apply"), name);
 	patch->rejected = 0;
diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 70b3a06..0a8de4a 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -52,4 +52,66 @@ test_expect_success 'check result' '
 
 '
 
+test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '
+
+	git reset --hard &&
+	ln -s ../i386/dir arch/x86_64/dir &&
+	git add arch/x86_64/dir &&
+	git diff HEAD >add_symlink.patch &&
+	git reset --hard &&
+
+	mkdir arch/x86_64/dir &&
+	>arch/x86_64/dir/file &&
+	git add arch/x86_64/dir/file &&
+	git diff HEAD >add_file.patch &&
+	git reset --hard &&
+	rm -fr arch/x86_64/dir &&
+
+	cat add_symlink.patch add_file.patch >patch &&
+
+	mkdir arch/i386/dir
+'
+
+test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '
+
+	# same input creates a confusihng symbolic link
+	test_must_fail git apply patch 2>error-wt &&
+	test_i18ngrep "beyond a symbolic link" error-wt &&
+	test ! -e arch/x86_64/dir &&
+	test ! -e arch/i386/dir/file &&
+
+	test_must_fail git apply --index patch 2>error-ix &&
+	test_i18ngrep "beyond a symbolic link" error-ix &&
+	test ! -e arch/x86_64/dir &&
+	test ! -e arch/i386/dir/file &&
+	test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+	test_must_fail git ls-files --error-unmatch arch/i386/dir &&
+
+	test_must_fail git apply --cached patch 2>error-ct &&
+	test_i18ngrep "beyond a symbolic link" error-ct &&
+	test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+	test_must_fail git ls-files --error-unmatch arch/i386/dir
+'
+
+test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '
+
+	# existing symbolic link
+	git reset --hard &&
+	ln -s ../i386/dir arch/x86_64/dir &&
+	git add arch/x86_64/dir &&
+
+	test_must_fail git apply add_file.patch 2>error-wt-file &&
+	test_i18ngrep "beyond a symbolic link" error-wt-file &&
+	test ! -e arch/i386/dir/file &&
+
+	test_must_fail git apply --index add_file.patch 2>error-ix-file &&
+	test_i18ngrep "beyond a symbolic link" error-ix-file &&
+	test ! -e arch/i386/dir/file &&
+	test_must_fail git ls-files --error-unmatch arch/i386/dir &&
+
+	test_must_fail git apply --cached add_file.patch 2>error-ct-file &&
+	test_i18ngrep "beyond a symbolic link" error-ct-file &&
+	test_must_fail git ls-files --error-unmatch arch/i386/dir
+'
+
 test_done
-- 
2.3.0-rc2-153-g9e53805

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

[Stefan Beller]

On Thu, Jan 29, 2015 at 12:45 PM, Junio C Hamano <gitster@pobox.com> wrote:

> +
> +test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '
> +
> +       # same input creates a confusihng symbolic link

s/confusihng/confusing/

[PATCH 2/1] apply: reject input that touches outside $cwd

[Junio C Hamano]

By default, a patch that affects outside the working area is
rejected as a mistake; Git itself never creates such a patch
unless the user bends backwards and specifies nonstandard
prefix to "git diff" and friends.

When `git apply` is used without either `--index` or `--cached`
option as a "better GNU patch", the user can pass `--allow-uplevel`
option to override this safety check.  This cannot be used to escape
outside the working tree when using `--index` or `--cached` to apply
the patch to the index.

The new test was stolen from Jeff King with slight enhancements.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Meant to apply on top of the previous one, but these two are
   about separate and orthogonal issues.

 Documentation/git-apply.txt |  14 ++++-
 builtin/apply.c             |  26 +++++++++
 t/t4139-apply-escape.sh     | 137 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 176 insertions(+), 1 deletion(-)
 create mode 100755 t/t4139-apply-escape.sh

diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index f605327..20c3a6f 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 	  [--ignore-space-change | --ignore-whitespace ]
 	  [--whitespace=(nowarn|warn|fix|error|error-all)]
 	  [--exclude=<path>] [--include=<path>] [--directory=<root>]
-	  [--verbose] [<patch>...]
+	  [--verbose] [--allow-uplevel] [<patch>...]
 
 DESCRIPTION
 -----------
@@ -229,6 +229,18 @@ For example, a patch that talks about updating `a/git-gui.sh` to `b/git-gui.sh`
 can be applied to the file in the working tree `modules/git-gui/git-gui.sh` by
 running `git apply --directory=modules/git-gui`.
 
+--allow-uplevel::
+	By default, a patch that affects outside the working area is
+	rejected as a mistake; Git itself never creates such a patch
+	unless the user bends backwards and specifies nonstandard
+	prefix to "git diff" and friends.
++
+When `git apply` is used without either `--index` or `--cached`
+option as a "better GNU patch", the user can pass `--allow-uplevel`
+option to override this safety check.  This cannot be used to escape
+outside the working tree when using `--index` or `--cached` to apply
+the patch to the index.
+
 Configuration
 -------------
 
diff --git a/builtin/apply.c b/builtin/apply.c
index dcb44fb..ce5a594 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -50,6 +50,7 @@ static int apply_verbosely;
 static int allow_overlap;
 static int no_add;
 static int threeway;
+static int allow_uplevel;
 static const char *fake_ancestor;
 static int line_termination = '\n';
 static unsigned int p_context = UINT_MAX;
@@ -3523,6 +3524,23 @@ symlink_found:
 
 }
 
+static void die_on_uplevel_path(struct patch *patch)
+{
+	const char *old_name = NULL;
+	const char *new_name = NULL;
+	if (patch->is_delete)
+		old_name = patch->old_name;
+	else if (!patch->is_new && !patch->is_copy)
+		old_name = patch->old_name;
+	if (!patch->is_delete)
+		new_name = patch->new_name;
+
+	if (old_name && !verify_path(old_name))
+		die(_("invalid path '%s'"), old_name);
+	if (new_name && !verify_path(new_name))
+		die(_("invalid path '%s'"), new_name);
+}
+
 /*
  * Check and apply the patch in-core; leave the result in patch->result
  * for the caller to write it out to the final destination.
@@ -3614,6 +3632,9 @@ static int check_patch(struct patch *patch)
 		return error(_("affected file '%s' is beyond a symbolic link"),
 			     patch->new_name);
 
+	if (!allow_uplevel)
+		die_on_uplevel_path(patch);
+
 	if (apply_data(patch, &st, ce) < 0)
 		return error(_("%s: patch does not apply"), name);
 	patch->rejected = 0;
@@ -4423,6 +4444,8 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)
 			N_("make sure the patch is applicable to the current index")),
 		OPT_BOOL(0, "cached", &cached,
 			N_("apply a patch without touching the working tree")),
+		OPT_BOOL(0, "allow-uplevel", &allow_uplevel,
+			N_("accept a patch to touch outside the current directory")),
 		OPT_BOOL(0, "apply", &force_apply,
 			N_("also apply the patch (use with --stat/--summary/--check)")),
 		OPT_BOOL('3', "3way", &threeway,
@@ -4495,6 +4518,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)
 			die(_("--cached outside a repository"));
 		check_index = 1;
 	}
+	if (check_index)
+		allow_uplevel = 0;
+
 	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];
 		int fd;
diff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh
new file mode 100755
index 0000000..39de838
--- /dev/null
+++ b/t/t4139-apply-escape.sh
@@ -0,0 +1,137 @@
+#!/bin/sh
+
+test_description='paths written by git-apply cannot escape the working tree'
+. ./test-lib.sh
+
+# tests will try to write to ../foo, and we do not
+# want them to escape the trash directory when they
+# fail
+test_expect_success 'bump git repo one level down' '
+	mkdir inside &&
+	mv .git inside/ &&
+	cd inside
+'
+
+# $1 = name of file
+# $2 = current path to file (if different)
+mkpatch_add() {
+	rm -f "${2:-$1}" &&
+	cat <<-EOF
+	diff --git a/$1 b/$1
+	new file mode 100644
+	index 0000000..53c74cd
+	--- /dev/null
+	+++ b/$1
+	@@ -0,0 +1 @@
+	+evil
+	EOF
+}
+
+mkpatch_del() {
+	echo evil >"${2:-$1}" &&
+	cat <<-EOF
+	diff --git a/$1 b/$1
+	deleted file mode 100644
+	index 53c74cd..0000000
+	--- a/$1
+	+++ /dev/null
+	@@ -1 +0,0 @@
+	-evil
+	EOF
+}
+
+# $1 = name of file
+# $2 = content of symlink
+mkpatch_symlink() {
+	rm -f "$1" &&
+	cat <<-EOF
+	diff --git a/$1 b/$1
+	new file mode 120000
+	index 0000000..$(printf "%s" "$2" | git hash-object --stdin)
+	--- /dev/null
+	+++ b/$1
+	@@ -0,0 +1 @@
+	+$2
+	\ No newline at end of file
+	EOF
+}
+
+test_expect_success 'cannot add file containing ..' '
+	mkpatch_add ../foo >patch &&
+	test_must_fail git apply patch &&
+	test_path_is_missing ../foo
+'
+
+test_expect_success 'can add file containing .. with --allow-uplevel' '
+	mkpatch_add ../foo >patch &&
+	git apply --allow-uplevel patch &&
+	test_path_is_file ../foo
+'
+
+test_expect_success  'cannot add file containing .. (index)' '
+	mkpatch_add ../foo >patch &&
+	test_must_fail git apply --index patch &&
+	test_path_is_missing ../foo
+'
+
+test_expect_success  'cannot add file containing .. with --allow-uplevel (index)' '
+	mkpatch_add ../foo >patch &&
+	test_must_fail git apply --index --allow-uplevel patch &&
+	test_path_is_missing ../foo
+'
+
+test_expect_success 'cannot del file containing ..' '
+	mkpatch_del ../foo >patch &&
+	test_must_fail git apply patch &&
+	test_path_is_file ../foo
+'
+
+test_expect_success 'can del file containing .. with --allow-uplevel' '
+	mkpatch_del ../foo >patch &&
+	git apply --allow-uplevel patch &&
+	test_path_is_missing ../foo
+'
+
+test_expect_success 'cannot del file containing .. (index)' '
+	mkpatch_del ../foo >patch &&
+	test_must_fail git apply --index patch &&
+	test_path_is_file ../foo
+'
+
+test_expect_success 'symlink escape via ..' '
+	{
+		mkpatch_symlink tmp .. &&
+		mkpatch_add tmp/foo ../foo
+	} >patch &&
+	test_must_fail git apply patch &&
+	test_path_is_missing ../foo
+'
+
+test_expect_success 'symlink escape via .. (index)' '
+	{
+		mkpatch_symlink tmp .. &&
+		mkpatch_add tmp/foo ../foo
+	} >patch &&
+	test_must_fail git apply --index patch &&
+	test_path_is_missing ../foo
+'
+
+test_expect_success 'symlink escape via absolute path' '
+	{
+		mkpatch_symlink tmp "$(pwd)" &&
+		mkpatch_add tmp/foo ../foo
+	} >patch &&
+	test_must_fail git apply patch &&
+	test_path_is_missing ../foo
+'
+
+test_expect_success 'symlink escape via absolute path (index)' '
+	{
+		mkpatch_symlink tmp "$(pwd)" &&
+		mkpatch_add tmp/foo ../foo
+	} >patch &&
+	test_must_fail git apply --index patch &&
+	test_path_is_missing ../foo
+'
+
+test_done
-- 
2.3.0-rc2-158-g17413e7

Re: [PATCH 2/1] apply: reject input that touches outside $cwd

[Jeff King]

On Thu, Jan 29, 2015 at 03:48:14PM -0800, Junio C Hamano wrote:

> By default, a patch that affects outside the working area is
> rejected as a mistake; Git itself never creates such a patch
> unless the user bends backwards and specifies nonstandard
> prefix to "git diff" and friends.
> 
> When `git apply` is used without either `--index` or `--cached`
> option as a "better GNU patch", the user can pass `--allow-uplevel`
> option to override this safety check.  This cannot be used to escape
> outside the working tree when using `--index` or `--cached` to apply
> the patch to the index.

It looks like your new --allow-uplevel goes to verify_path(). So this
isn't just about "..", but it will also protect against applying a patch
inside ".git". Which seems like a good thing to me, but I wonder if the
option name is a little misleading. It is really about applying the same
checks we do for index paths to the non-index mode of "git apply".

>  * Meant to apply on top of the previous one, but these two are
>    about separate and orthogonal issues.

I agree they are orthogonal in concept, though I doubt the symlink tests
here would pass without the previous one (since verify_path does not
know or care about crossing symlink boundaries).

-Peff

Re: [PATCH 2/1] apply: reject input that touches outside $cwd

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It looks like your new --allow-uplevel goes to verify_path(). So this
> isn't just about "..", but it will also protect against applying a patch
> inside ".git". Which seems like a good thing to me, but I wonder if the
> option name is a little misleading.

True; not just misleading but is incorrect, I would say.
Suggestions?

> I agree they are orthogonal in concept, though I doubt the symlink tests
> here would pass without the previous one...

It won't; "do not apply across symlinks" is unconditional, and the
new codepath introduced by this patch, which is conditional to the
user option, shouldn't have to worry about them.

Re: [PATCH 2/1] apply: reject input that touches outside $cwd

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > It looks like your new --allow-uplevel goes to verify_path(). So this
> > isn't just about "..", but it will also protect against applying a patch
> > inside ".git". Which seems like a good thing to me, but I wonder if the
> > option name is a little misleading.
> 
> True; not just misleading but is incorrect, I would say.
> Suggestions?

I think just "--verify-paths" (and "--no-verify-paths", since the former
would be the default) might be fine. That leaves the definition of
"verify" vague, but I think that's OK. It used to mean "no '..' and no
'.git'", and now it has been widened to include "no weird
filesystem-specific variants of .git".

If you wanted to avoid the negative being the commonly used option,
maybe "--unsafe-paths" (or "--allow-unsafe-paths" if you like verbs).

-Peff

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

[Christian Couder]

On Thu, Jan 29, 2015 at 9:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Instead, for any patch in the input that leaves a path (i.e. a non
> deletion) in the result, we check all leading paths against interim
> result and then either the index or the working tree.  The interim
> results of applying patches are kept track of by fn_table logic for
> us already, so use it to fiture out if existing a symbolic link will

s/fiture/figure/
s/existing a symbolic link/an existing symbolic link/

> cause problems, if a new symbolic link that will cause problems will
> appear, etc.

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

[Jeff King]

On Thu, Jan 29, 2015 at 12:45:22PM -0800, Junio C Hamano wrote:

> +static int path_is_beyond_symlink(const char *name_)
> +{
> +	struct strbuf name = STRBUF_INIT;
> +
> +	strbuf_addstr(&name, name_);
> +	do {
> +		struct patch *previous;
> +
> +		while (--name.len && name.buf[name.len] != '/')
> +			; /* scan backwards */
> +		if (!name.len)
> +			break;

I imagine it is impossible here for "name_" to be initially empty, but
it would make the backwards-scan loop go quite badly. Worth a comment or
an assert()?

> +		name.buf[name.len] = '\0';
> +		previous = in_fn_table(name.buf);
> +		if (previous) {
> +			if (!was_deleted(previous) &&
> +			    !to_be_deleted(previous) &&
> +			    previous->new_mode &&
> +			    S_ISLNK(previous->new_mode))
> +				goto symlink_found;
> +		} else if (check_index) {
> +			int pos = cache_name_pos(name.buf, name.len);
> +			if (0 <= pos &&
> +			    S_ISLNK(active_cache[pos]->ce_mode))
> +				goto symlink_found;
> +		} else {
> +			struct stat st;
> +			if (!lstat(name.buf, &st) && S_ISLNK(st.st_mode))
> +				goto symlink_found;
> +		}
> +	} while (1);
> +
> +	strbuf_release(&name);
> +	return 0;
> +symlink_found:
> +	strbuf_release(&name);
> +	return 1;

Style nit, but might this be easier to follow the logic without the
gotos, by putting the setup and cleanup in a wrapper function and
returning directly from the main logic?

  static int path_is_beyond_symlink(const char *name)
  {
	struct strbuf buf = STRBUF_INIT;
	int ret;

	strbuf_addstr(&buf, name);
	ret = path_is_beyond_symlink_1(name);
	strbuf_release(&buf);

	return ret;
  }

I can live with it either way, though.

> +	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? IOW, why should this test
fail:

diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 0a8de4a..f03b604 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -64,6 +64,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '
 	>arch/x86_64/dir/file &&
 	git add arch/x86_64/dir/file &&
 	git diff HEAD >add_file.patch &&
+	git diff -R HEAD >del_file.patch &&
 	git reset --hard &&
 	rm -fr arch/x86_64/dir &&
 
@@ -111,7 +112,11 @@ test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '
 
 	test_must_fail git apply --cached add_file.patch 2>error-ct-file &&
 	test_i18ngrep "beyond a symbolic link" error-ct-file &&
-	test_must_fail git ls-files --error-unmatch arch/i386/dir
+	test_must_fail git ls-files --error-unmatch arch/i386/dir &&
+
+	>arch/i386/dir/file &&
+	test_must_fail git apply del_file.patch &&
+	test_path_is_file arch/i386/dir/file
 '
 
 test_done

> +	test ! -e arch/x86_64/dir &&
> +	test ! -e arch/i386/dir/file &&

Minor nit: use test_path_is_missing here (and elsewhere in the added
tests).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Jan 29, 2015 at 12:45:22PM -0800, Junio C Hamano wrote:
>
>> +	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?

Half-written logic, forgotten to be revisited (i.e. "ok, anything
that is not delete we can check new_name, so do that first, later
we'd deal with deletion patch and I think the way to do so is by
checking old_name, but let's make sure this case works first").

Thanks for catching.

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

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Jan 29, 2015 at 12:45:22PM -0800, Junio C Hamano wrote:
> >
> >> +	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?
> 
> Half-written logic, forgotten to be revisited (i.e. "ok, anything
> that is not delete we can check new_name, so do that first, later
> we'd deal with deletion patch and I think the way to do so is by
> checking old_name, but let's make sure this case works first").

OK, I was worried I was missing something clever. :)

I agree that checking patch->old_name should work in that case.

-Peff

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

[Junio C Hamano]

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

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

[Jeff King]

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

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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".

Just to make sure that I am not hallucinating, I added this one:

diff --git a/t/t4114-apply-typechange.sh b/t/t4114-apply-typechange.sh
index ebadbc3..83ddf62 100755
--- a/t/t4114-apply-typechange.sh
+++ b/t/t4114-apply-typechange.sh
@@ -119,4 +119,12 @@ test_expect_success 'directory becomes symlink' '
 test_debug 'cat patch'
 
 
+test_expect_success 'directory becomes symlink' '
+	git checkout -f foo-becomes-a-directory &&
+	printf "%s\n" foo/baz foo >order &&
+	git diff-tree -Oorder -p HEAD foo-symlinked-to-bar >patch &&
+	git apply --index <patch
+	'
+test_debug 'cat patch'
+
 test_done

It is a copy of the original, only forcing the patches in the input
in the opposite order.

Having said that and also having read your two-phase internal
application change, I think that two-phase thing is probably a good
way to go (we may even want to ignore "previous_patch()" stuff, as
its "was_deleted()" and "tobe_deleted()" are all about "force the
application of a later patch to depend on the result of application
of an earlier patch").

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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


I am not sure how to fix this, without completely ripping out the
misguided "We should be able to concatenate outputs from multiple
invocations of 'git diff' into a single file and apply the result
with a single invocation of 'git apply'" change I grudgingly
accepted long time ago (7a07841c (git-apply: handle a patch that
touches the same path more than once better, 2008-06-27).

"git diff" output is designed each patch to apply independently to
the preimage to produce the postimage, and that allows patches to
two files can be swapped via -Oorderfile mechanism, and also "X was
created by copying from Y and Y is modified in place" will result in
X with the contents of Y in the preimage (i.e. before the in-place
modification of Y in the same patch) regardless of the order of X
and Y in the "git diff" output.  The above input used by t4114.11
expects to remove 'foo/baz' (leaving an empty directory foo as an
result but we do not track directories so it can be nuked to make
room if other patch in the same input wants to put something else,
either a regular file or a symbolic link, there) and create a blob
at 'foo', and such an input should apply regardless of the order of
patches in it.

The in_fn_table[] stuff broke that design completely.

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

[Jeff King]

On Fri, Jan 30, 2015 at 12:11:30PM -0800, Junio C Hamano wrote:

> I am not sure how to fix this, without completely ripping out the
> misguided "We should be able to concatenate outputs from multiple
> invocations of 'git diff' into a single file and apply the result
> with a single invocation of 'git apply'" change I grudgingly
> accepted long time ago (7a07841c (git-apply: handle a patch that
> touches the same path more than once better, 2008-06-27).
> 
> "git diff" output is designed each patch to apply independently to
> the preimage to produce the postimage, and that allows patches to
> two files can be swapped via -Oorderfile mechanism, and also "X was
> created by copying from Y and Y is modified in place" will result in
> X with the contents of Y in the preimage (i.e. before the in-place
> modification of Y in the same patch) regardless of the order of X
> and Y in the "git diff" output.  The above input used by t4114.11
> expects to remove 'foo/baz' (leaving an empty directory foo as an
> result but we do not track directories so it can be nuked to make
> room if other patch in the same input wants to put something else,
> either a regular file or a symbolic link, there) and create a blob
> at 'foo', and such an input should apply regardless of the order of
> patches in it.
> 
> The in_fn_table[] stuff broke that design completely.

I had the impression that we did not apply in any arbitrary order that
could work, but rather that we did deletions first followed by
additions. But I am fairly ignorant of the apply code.

If that assumption is correct, then I think we could just follow the
same phases that the actual application does. Here's a hacky version
below. Probably the check of phase versus is_delete needs to be better
(and ideally the logic would be factored out of write_one_result so they
always match).

diff --git a/builtin/apply.c b/builtin/apply.c
index f5491cd..85364b8 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3593,7 +3593,7 @@ symlink_found:
  * Check and apply the patch in-core; leave the result in patch->result
  * for the caller to write it out to the final destination.
  */
-static int check_patch(struct patch *patch)
+static int check_patch(struct patch *patch, int phase)
 {
 	struct stat st;
 	const char *old_name = patch->old_name;
@@ -3604,6 +3604,9 @@ static int check_patch(struct patch *patch)
 	int ok_if_exists;
 	int status;
 
+	if (!phase != patch->is_delete)
+		return 0;
+
 	patch->rejected = 1; /* we will drop this after we succeed */
 
 	status = check_preimage(patch, &ce, &st);
@@ -3679,6 +3682,9 @@ static int check_patch(struct patch *patch)
 	if (!patch->is_delete && path_is_beyond_symlink(patch->new_name))
 		return error(_("affected file '%s' is beyond a symbolic link"),
 			     patch->new_name);
+	if (patch->is_delete && path_is_beyond_symlink(patch->old_name))
+		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);
@@ -3686,7 +3692,7 @@ static int check_patch(struct patch *patch)
 	return 0;
 }
 
-static int check_patch_list(struct patch *patch)
+static int check_patch_list_1(struct patch *patch, int phase)
 {
 	int err = 0;
 
@@ -3695,12 +3701,22 @@ static int check_patch_list(struct patch *patch)
 		if (apply_verbosely)
 			say_patch_name(stderr,
 				       _("Checking patch %s..."), patch);
-		err |= check_patch(patch);
+		err |= check_patch(patch, phase);
 		patch = patch->next;
 	}
 	return err;
 }
 
+static int check_patch_list(struct patch *patch)
+{
+	int err = 0;
+	int phase;
+
+	for (phase = 0; phase < 2; phase++)
+		err |= check_patch_list_1(patch, phase);
+	return err;
+}
+
 /* This function tries to read the sha1 from the current index */
 static int get_current_sha1(const char *path, unsigned char *sha1)
 {

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I had the impression that we did not apply in any arbitrary order that
> could work, but rather that we did deletions first followed by
> additions. But I am fairly ignorant of the apply code.

No, you are thinking about the write-out of the finished result,
which may have to turn existing directory to a file or vice versa on
the filesystem, but that happens _after_ we decide what to turn into
what else, completely in-core.

And the decision to determine what the input _means_ should not
depend on the order of patches in the input.

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

[Jeff King]

On Fri, Jan 30, 2015 at 12:20:02PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I had the impression that we did not apply in any arbitrary order that
> > could work, but rather that we did deletions first followed by
> > additions. But I am fairly ignorant of the apply code.
> 
> No, you are thinking about the write-out of the finished result,
> which may have to turn existing directory to a file or vice versa on
> the filesystem, but that happens _after_ we decide what to turn into
> what else, completely in-core.
> 
> And the decision to determine what the input _means_ should not
> depend on the order of patches in the input.

Ah, OK. Yeah, doing it progressively can only be accurate if our
name-checks follow the same order as applying, because we are checking
against a particular state.

But could we instead pull this check to just before the write-out time?
That is, to let any horrible thing happen in-core, as long as what we
write out to the index and the filesystem is sane?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Ah, OK. Yeah, doing it progressively can only be accurate if our
> name-checks follow the same order as applying, because we are checking
> against a particular state.
>
> But could we instead pull this check to just before the write-out time?
> That is, to let any horrible thing happen in-core, as long as what we
> write out to the index and the filesystem is sane?

That would make me feel dirty.

I noticed one thing.  The PATH_TO_BE_DELETED/PATH_WAS_DELETED crud
kicks in -only- during the actual application phase, and all patches
that remove paths from the end result should have been appropriately
marked in fn_table[] by the call to prepare_fn_table() at the
beginning of check_patch_list() as PATH_TO_BE_DELETED.

But it was wrong to call previous_patch() in my fix.  The function
is the cause of evil I see in the "let's support concatenated patch,
making the later patch depend on the result of earlier ones" and
deliberately ignores PATH_TO_BE_DELETED patches.  We would need to
do the early part of previous_patch() without the filtering.

This is a preparatory step to clean-up the mess I have in mind.  It
does not mean to change the semantics (applied to the codebase with
or without the changes we have been discussing); it only makes it
always return the "previous" patch to the callers and makes them
responsible to see if the previous was to-be-deleted or was-deleted.

With that change, I think my symlink fix plus the "check the deleted
one with old_name, too" change has a better chance to do the moral
equivalent of your two-phase thing.  Essentially, "First see what
will be deleted in the input as a whole" has already been done by
the prepare_fn_table() thing.

 builtin/apply.c | 34 ++++++++++++----------------------
 1 file changed, 12 insertions(+), 22 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 41b7236..a064017 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3097,25 +3097,12 @@ static int checkout_target(struct cache_entry *ce, struct stat *st)
 	return 0;
 }
 
-static struct patch *previous_patch(struct patch *patch, int *gone)
+static struct patch *previous_patch(struct patch *patch)
 {
-	struct patch *previous;
-
-	*gone = 0;
 	if (patch->is_copy || patch->is_rename)
 		return NULL; /* "git" patches do not depend on the order */
 
-	previous = in_fn_table(patch->old_name);
-	if (!previous)
-		return NULL;
-
-	if (to_be_deleted(previous))
-		return NULL; /* the deletion hasn't happened yet */
-
-	if (was_deleted(previous))
-		*gone = 1;
-
-	return previous;
+	return in_fn_table(patch->old_name);
 }
 
 static int verify_index_match(const struct cache_entry *ce, struct stat *st)
@@ -3170,11 +3157,11 @@ static int load_preimage(struct image *image,
 	struct patch *previous;
 	int status;
 
-	previous = previous_patch(patch, &status);
-	if (status)
+	previous = previous_patch(patch);
+	if (was_deleted(previous))
 		return error(_("path %s has been renamed/deleted"),
 			     patch->old_name);
-	if (previous) {
+	if (previous && !to_be_deleted(previous)) {
 		/* We have a patched copy in memory; use that. */
 		strbuf_add(&buf, previous->result, previous->resultsize);
 	} else {
@@ -3384,18 +3371,18 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 {
 	const char *old_name = patch->old_name;
 	struct patch *previous = NULL;
-	int stat_ret = 0, status;
+	int stat_ret = 0;
 	unsigned st_mode = 0;
 
 	if (!old_name)
 		return 0;
 
 	assert(patch->is_new <= 0);
-	previous = previous_patch(patch, &status);
+	previous = previous_patch(patch);
 
-	if (status)
+	if (was_deleted(previous))
 		return error(_("path %s has been renamed/deleted"), old_name);
-	if (previous) {
+	if (previous && !to_be_deleted(previous)) {
 		st_mode = previous->new_mode;
 	} else if (!cached) {
 		stat_ret = lstat(old_name, st);
@@ -3403,6 +3390,9 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s
 			return error(_("%s: %s"), old_name, strerror(errno));
 	}
 
+	if (to_be_deleted(previous))
+		previous = NULL;
+
 	if (check_index && !previous) {
 		int pos = cache_name_pos(old_name, strlen(old_name));
 		if (pos < 0) {

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But could we instead pull this check to just before the write-out time?
> That is, to let any horrible thing happen in-core, as long as what we
> write out to the index and the filesystem is sane?

The check in-core is somewhat tricky, because we would need to (1)
catch a patch that creates a symlink and also a file as if that new
symlink is a directory and (2) allow a patch that removes a symlink
and also a file in a new directory at the location removed symlink
used to occupy.

For (1) we need to see if there is a patch in the entire input that
creates a symbolic link and reject the input.  For (2) we need to
see if there is a patch that removes the symbolic link.  (1) cannot
be caught with the approach based on fn_table[], which is inherently
meant to help incremental application, that is oblivious to a path
that will materialize after applying a later patch in the input.

Let me think about it a bit more.  The fix probably needs to abandon
depending on fn_table[] stuff, if we want to do in the "sanity check
the input and compute the final state all in-core" route.

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Andreas Gruenbacher]

On Mon, 26 Jan 2015 12:44:33 -0800, Linus Torvalds wrote:
> I've considered that for a while already, because "patch" _does_ kind of
> understand them these days, although I think it gets the cross-rename
> case wrong because it fundamentally works on a file-by-file basis.

Patch handles cross-renames correctly nowadays; if not, it's a bug.

That's not enough to solve the symlink problem unfortunately.

Andreas

Re: patch-2.7.3 no longer applies relative symbolic link patches

[Andreas Gruenbacher]

On Mon, 26 Jan 2015 12:44:33 -0800, Linus Torvalds wrote:
> I've considered that for a while already, because "patch" _does_ kind of
> understand them these days, although I think it gets the cross-rename
> case wrong because it fundamentally works on a file-by-file basis.

Patch handles cross-renames correctly nowadays; if not, it's a bug.

That's not enough to solve the symlink problem unfortunately.

Andreas