[PATCH v2 0/4] "git apply" safety

[PATCH v2 0/4] "git apply" safety

[Junio C Hamano]

"git apply" have been fairly careless about letting the input follow
symbolic links, especially when used without the --index/--cached
options (which was more or less deliberate to mimic what "patch"
used to do).  When the input tells it to modify a/b/c, and lstat(2)
said that there is "a/b/c" that matches the preimage in the input,
we happily overwrote it, even when a/b is a symbolic link that
pointed somewhere, even outside the working tree.

This series tightens things a bit for safety.

 (1) By default, we reject patches to ".git/file", "../some/where",
     "./this/././that", etc., i.e. the names you cannot add to the
     index.  Those who use "git apply" (without --index/--cached) as
     a replacement for GNU patch can use --unsafe-paths option to
     override this safety.  This is what patch 1/4 does.

 (2) We do not allow a patch to depend on a location beyond a
     symbolic link (this includes "a patch to remove a path beyond a
     symbolic link").  This is patch 2/4 and 3/4.

 (3) We do not allow a patch to create result on a location beyond a
     symbolic link.  This is patch 4/4.

There is no knob to override the latter two points, as this is not a
safety but is a correctness issue.  Because Git keeps track of and
can express changes to symbolic links, a patch that expects a file
"a/b/c" to be tracked (either the patch adds it, or it modifies an
existing file tehre) implicitly expects that there is no symbolic
link "a/b", so attempting to apply such a patch to a tree with a
symbolic link at "a/b", even when the link points at some directory,
must detect that the target tree does not match what the patch's
preimage expects and fail.

The previous attempt begins at around here:

  http://thread.gmane.org/gmane.linux.kernel/1874498/focus=1878385

Junio C Hamano (4):
  apply: reject input that touches outside $cwd
  apply: do not read from the filesystem under --index
  apply: do not read from beyond a symbolic link
  apply: do not touch a file beyond a symbolic link

 Documentation/git-apply.txt     |  14 +++-
 builtin/apply.c                 | 139 +++++++++++++++++++++++++++++++++++++++-
 t/t4122-apply-symlink-inside.sh |  89 +++++++++++++++++++++++++
 t/t4139-apply-escape.sh         | 137 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 377 insertions(+), 2 deletions(-)
 create mode 100755 t/t4139-apply-escape.sh

-- 
2.3.0-rc2-164-g799cdce

[PATCH v2 1/4] 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 (or a mischief); Git itself does not create
such a patch, unless the user bends backwards and specifies a
non-standard 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 `--unsafe-paths`
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>
---
 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..a6e83a9 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] [--unsafe-paths] [<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`.
 
+--unsafe-paths::
+	By default, a patch that affects outside the working area is
+	rejected as a mistake (or a mischief); 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 `--unsafe-paths`
+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 ef32e4f..c751ddf 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 unsafe_paths;
 static const char *fake_ancestor;
 static int line_termination = '\n';
 static unsigned int p_context = UINT_MAX;
@@ -3483,6 +3484,23 @@ static int check_to_create(const char *new_name, int ok_if_exists)
 	return 0;
 }
 
+static void die_on_unsafe_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.
@@ -3570,6 +3588,9 @@ static int check_patch(struct patch *patch)
 		}
 	}
 
+	if (!unsafe_paths)
+		die_on_unsafe_path(patch);
+
 	if (apply_data(patch, &st, ce) < 0)
 		return error(_("%s: patch does not apply"), name);
 	patch->rejected = 0;
@@ -4379,6 +4400,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, "unsafe-paths", &unsafe_paths,
+			N_("accept a patch to touch outside the working area")),
 		OPT_BOOL(0, "apply", &force_apply,
 			N_("also apply the patch (use with --stat/--summary/--check)")),
 		OPT_BOOL('3', "3way", &threeway,
@@ -4451,6 +4474,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)
 			die(_("--cached outside a repository"));
 		check_index = 1;
 	}
+	if (check_index)
+		unsafe_paths = 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..5e67179
--- /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 --unsafe-paths' '
+	mkpatch_add ../foo >patch &&
+	git apply --unsafe-paths 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 --unsafe-paths (index)' '
+	mkpatch_add ../foo >patch &&
+	test_must_fail git apply --index --unsafe-paths 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 --unsafe-paths' '
+	mkpatch_del ../foo >patch &&
+	git apply --unsafe-paths 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_failure '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_failure '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_failure '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-164-g799cdce

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

[Jeff King]

On Mon, Feb 02, 2015 at 03:27:27PM -0800, Junio C Hamano wrote:

> By default, a patch that affects outside the working area is
> rejected as a mistake (or a mischief); Git itself does not create
> such a patch, unless the user bends backwards and specifies a
> non-standard 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 `--unsafe-paths`
> 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.

I notice that this includes the symlink-crossing tests, marked as
failures. Reading the series, I know what is going to happen later, but
do you want to leave a note like:

  Note that we also add tests for leaving the working directory by
  crossing symlink boundaries, which is not addressed in this patch.
  That is a separate issue caused following symlinks, which will come
  later.

or something to help later readers of "git log"?

> +--unsafe-paths::
> +	By default, a patch that affects outside the working area is
> +	rejected as a mistake (or a mischief); Git itself never
> +	creates such a patch unless the user bends backwards and
> +	specifies nonstandard prefix to "git diff" and friends.

Minor wordsmithing: the usual idiom is "bend over backwards", and you
probably want s/specifies/& a/.

> ++
> +When `git apply` is used without either `--index` or `--cached`
> +option as a "better GNU patch", the user can pass `--unsafe-paths`
> +option to override this safety check.

Similarly, probably every instance of "foo option" would read better as
"the foo option".

> This cannot be used to escape
> +outside the working tree when using `--index` or `--cached` to apply
> +the patch to the index.

I had trouble figuring out what this meant. Would it be simpler to just
say:

  This option has no effect when `--index` or `--cached` is in use.

Or is there some other subtlety that you are trying to convey that I am
missing?

-Peff

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

[Jeff King]

On Mon, Feb 02, 2015 at 03:27:27PM -0800, Junio C Hamano wrote:

> +test_expect_failure 'symlink escape via ..' '
> +	{
> +		mkpatch_symlink tmp .. &&
> +		mkpatch_add tmp/foo ../foo
> +	} >patch &&
> +	test_must_fail git apply patch &&
> +	test_path_is_missing ../foo
> +'

By the way, does this patch (and the other symlink-escape ones) need to
be marked with the SYMLINKS prereq? For a pure-index application, it
should work anywhere, but I have a feeling that this "git apply patch"
may try to write the symlink to the filesystem, fail, and report failure
for the wrong reason.  I don't have a SYMLINK-challenged filesystem to
test on, though.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Feb 02, 2015 at 03:27:27PM -0800, Junio C Hamano wrote:
>
>> +test_expect_failure 'symlink escape via ..' '
>> +	{
>> +		mkpatch_symlink tmp .. &&
>> +		mkpatch_add tmp/foo ../foo
>> +	} >patch &&
>> +	test_must_fail git apply patch &&
>> +	test_path_is_missing ../foo
>> +'
>
> By the way, does this patch (and the other symlink-escape ones) need to
> be marked with the SYMLINKS prereq? For a pure-index application, it
> should work anywhere, but I have a feeling that this "git apply patch"
> may try to write the symlink to the filesystem, fail, and report failure
> for the wrong reason.  I don't have a SYMLINK-challenged filesystem to
> test on, though.

We check the links to be created by the patch itself in-core before
going to the filesystem, and the symbolic links you are creating
using mkpatch_symlink should be caught before we invoke symlink(2),
I think.

In other words, this series attempts to stick to the "verify
everything in-core before deciding that it is OK to touch the
working tree or the index".

A few new tests in t4122 do try to see that the command is not
fooled by existihng symbolic links on the filesystem and they need
to be marked with SYMLINKS prerequisite.

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

[Jeff King]

On Tue, Feb 03, 2015 at 12:23:28PM -0800, Junio C Hamano wrote:

> > By the way, does this patch (and the other symlink-escape ones) need to
> > be marked with the SYMLINKS prereq? For a pure-index application, it
> > should work anywhere, but I have a feeling that this "git apply patch"
> > may try to write the symlink to the filesystem, fail, and report failure
> > for the wrong reason.  I don't have a SYMLINK-challenged filesystem to
> > test on, though.
> 
> We check the links to be created by the patch itself in-core before
> going to the filesystem, and the symbolic links you are creating
> using mkpatch_symlink should be caught before we invoke symlink(2),
> I think.
> 
> In other words, this series attempts to stick to the "verify
> everything in-core before deciding that it is OK to touch the
> working tree or the index".

Right, I do not think these tests will _fail_ when the filesystem does
not support symlinks. But nor are they actually testing anything
interesting. They would pass on such a system even without your patch,
as we would fail to apply even the symlink creation part of the patch.

I can live with leaving them unmarked, though. It gets the code
exercised on more systems, which gives a slightly higher chance of
catching some other unexpected breakage.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Right, I do not think these tests will _fail_ when the filesystem does
> not support symlinks. But nor are they actually testing anything
> interesting. They would pass on such a system even without your patch,
> as we would fail to apply even the symlink creation part of the patch.

I thought we write out the contents of the symbolic link as a
regular file on such a filesystem, and as long as we do not expect
"test -h expected-to-be-symlink-we-just-created" to succeed we would
be fine.

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

[Jeff King]

On Tue, Feb 03, 2015 at 01:23:15PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Right, I do not think these tests will _fail_ when the filesystem does
> > not support symlinks. But nor are they actually testing anything
> > interesting. They would pass on such a system even without your patch,
> > as we would fail to apply even the symlink creation part of the patch.
> 
> I thought we write out the contents of the symbolic link as a
> regular file on such a filesystem, and as long as we do not expect
> "test -h expected-to-be-symlink-we-just-created" to succeed we would
> be fine.

But wouldn't we still fail writing "foo/bar" at that point if "foo" is a
regular file (again, we should never do this, but that is the point of
the test).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But wouldn't we still fail writing "foo/bar" at that point if "foo" is a
> regular file (again, we should never do this, but that is the point of
> the test).

The point of the test is not to create foo, whether it is a symlink
or an emulating regular file, in the first place.

In this piece:

>> +test_expect_failure 'symlink escape via ..' '
>> +	{
>> +		mkpatch_symlink tmp .. &&

This is a patch to create "tmp" that points at ".."

>> +		mkpatch_add tmp/foo ../foo

And this is a patch to create "tmp/foo", and make sure ../foo does
not exist in the filesystem to prepare for the test.

>> +	} >patch &&
>> +	test_must_fail git apply patch &&

And this patch, if the rejection logic were to be broken in the
future, might create "tmp" and then try to follow it to affect
"../foo".

>> +	test_path_is_missing ../foo

So if this test makes sure if "tmp" is missing, then it would be
alright, no?  The "follow the symlink to affect ../foo" may not
happen on a filesystem without symlinks, but verifying that "tmp"
is missing will assure us that the patch application is atomic,
i.e. if we see "tmp" on the filesystem after seeing "git apply"
failed, that is a sign that "git apply" failed to be atomic.

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

[Jeff King]

On Tue, Feb 03, 2015 at 01:40:12PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > But wouldn't we still fail writing "foo/bar" at that point if "foo" is a
> > regular file (again, we should never do this, but that is the point of
> > the test).
> 
> The point of the test is not to create foo, whether it is a symlink
> or an emulating regular file, in the first place.

I thought the point was not to create "../bar", when "foo" points to
"..". I agree that the way you have implemented it is that we would
never even write "foo", and the test checks for that, but to me that is
the least interesting bit of what is being tested. Crossing a symlink
boundary and escaping from the tree are interesting, and the atomicity
is a side note. We could also realize that treating "foo" as a file
would fail and cancel the whole operation atomically, too.

But I think we are getting into contrasting our mental models, which is
probably not productive. I am OK with leaving it without the SYMLINKS
flag, as it should pass on systems that do not handle symlinks. And if
it does not, then that will be a good cross-check that our analysis was
sane. :)

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Feb 03, 2015 at 01:40:12PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > But wouldn't we still fail writing "foo/bar" at that point if "foo" is a
>> > regular file (again, we should never do this, but that is the point of
>> > the test).
>> 
>> The point of the test is not to create foo, whether it is a symlink
>> or an emulating regular file, in the first place.
>
> I thought the point was not to create "../bar", when "foo" points to
> "..".

That is not what the new test you added was doing, though.  You are
calling "tmp" in that test "foo" and "../foo" in the test is called
"../bar" in the message I am responding to, so that is confusing,
but in these tests you added, I do not see anything that prepares a
symbolic link ON the filesystem and wait for "git apply" to get
fooled.

> I agree that the way you have implemented it is that we would
> never even write "foo", and the test checks for that, but to me that is
> the least interesting bit of what is being tested.

They are both interesting.  When rejecting an input, "git apply"
must be atomic.  When checking an input, "git apply" should notice
and reject a patch that tries to follow a symbolic link.

Taken together:

 (1) If a patchset tries to create a symbolic link and then tries to
     follow it, the latter principle should make "git apply" to
     reject the patchset, and the former should make sure there is
     no external effect.

 (2) If a patchset tries to affect a path, and the target of the
     patch application has a symlink that may divert the operation
     to the original path the patchset wants to affect, the latter
     principle should make "git apply" to reject the patchset, too.

And my observation is that the new tests that are not protected by
SYMLINKS prerequisite (i.e. all of them) in your new test 4139, are
all the former kind.  As "git aplly" must be atomic, rejection must
be decided without touching the filesystem at all.  Hence there is
no need for the filesystem to even support symbolic links.

But some bozo may try to break "git apply" in the future and try to
incrementally apply the patch in a patchset, and at that point, the
existing "test_must_fail git apply" may pass not because we
correctly decide not to follow symbolic links but because his broken
version of "git apply" would try to create a symbolic link (which we
would turn into a regular file) and then the filesystem would fail
to follow that symbolic link mimic, and as the result the test may
still pass.

In order to prevent that from happening, we may want to make sure
that

 - "test_must_fail git apply"

 - There is no "foo" (or "tmp"); the input to 'git apply' is the
   only thing that could create, as you do not create symlinks as
   traps before running 'git apply', so this will catch the 'make it
   incremental and then break the do-not-follow logic'.

 - There is no "../bar" (or "../foo").

The second one is missing from your tests, I think, and it would be
a very good addition, even on a platform with SYMLINKS prerequisite
satisfied.  The future change might be trying to make it incremental
and impelent do-not-follow logic by observing what is in the
filesystem, and we do want to catch such a broken implementation.




	

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

[Torsten Bögershausen]

If I am allowed to to some load thinking:

The commit msh header says:
  reject input that touches outside $cwd

The commit message says:
   By default, a patch that affects outside the working area

And the new command line option is this:
   --unsafe-paths
(Which may be a good choice to pretend people from using it without
   having read the documentaion)

(And isn't working area the same as "work space" ?)

I have the slight feeling that there may be more unsafe-path situations coming
up in the future..

Will
--allow-outside-ws
or
--patch-outside-ws
be better ?

[PATCH v2 2/4] apply: do not read from the filesystem under --index

[Junio C Hamano]

We currently read the preimage to apply a patch from the index only
when the --cached option is given.  Do so also when the command is
running under the --index option.  With --index, the index entry and
the working tree file for a path that is involved in a patch must be
identical, so this should not affect the result, but by reading from
the index, we will get the protection to avoid reading an unintended
path beyond a symbolic link automatically.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index c751ddf..05eaf54 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3136,7 +3136,7 @@ static int load_patch_target(struct strbuf *buf,
 			     const char *name,
 			     unsigned expected_mode)
 {
-	if (cached) {
+	if (cached || check_index) {
 		if (read_file_or_gitlink(ce, buf))
 			return error(_("read of %s failed"), name);
 	} else if (name) {
-- 
2.3.0-rc2-164-g799cdce

[PATCH v2 3/4] apply: do not read from beyond a symbolic link

[Junio C Hamano]

We should reject a patch, whether it renames/copies dir/file to
elsewhere with or without modificiation, or updates dir/file in
place, if "dir/" part is actually a symbolic link to elsewhere,
by making sure that the code to read the preimage does not read
from a path that is beyond a symbolic link.

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

diff --git a/builtin/apply.c b/builtin/apply.c
index 05eaf54..60d821c 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3145,6 +3145,8 @@ static int load_patch_target(struct strbuf *buf,
 				return read_file_or_gitlink(ce, buf);
 			else
 				return SUBMODULE_PATCH_WITHOUT_INDEX;
+		} else if (has_symlink_leading_path(name, strlen(name))) {
+			return error(_("reading from '%s' beyond a symbolic link"), name);
 		} else {
 			if (read_old_data(st, name, buf))
 				return error(_("read of %s failed"), name);
diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 70b3a06..035c080 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -52,4 +52,23 @@ test_expect_success 'check result' '
 
 '
 
+test_expect_success SYMLINKS 'do not read from beyond symbolic link' '
+	git reset --hard &&
+	mkdir -p arch/x86_64/dir &&
+	>arch/x86_64/dir/file &&
+	git add arch/x86_64/dir/file &&
+	echo line >arch/x86_64/dir/file &&
+	git diff >patch &&
+	git reset --hard &&
+
+	mkdir arch/i386/dir &&
+	>arch/i386/dir/file &&
+	ln -s ../i386/dir arch/x86_64/dir &&
+
+	test_must_fail git apply patch &&
+	test_must_fail git apply --cached patch &&
+	test_must_fail git apply --index patch
+
+'
+
 test_done
-- 
2.3.0-rc2-164-g799cdce

Re: [PATCH v2 3/4] apply: do not read from beyond a symbolic link

[Stefan Beller]

On Mon, Feb 2, 2015 at 3:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> +       test_must_fail git apply --index patch
> +
> +'

Is the empty line between the last test_must_fail and the closing `'`
intentional?

Re: [PATCH v2 3/4] apply: do not read from beyond a symbolic link

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Mon, Feb 2, 2015 at 3:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> +       test_must_fail git apply --index patch
>> +
>> +'
>
> Is the empty line between the last test_must_fail and the closing `'`
> intentional?

I think I just mimicked the previous existing one, but I can go with
the version without.

Re: [PATCH v2 3/4] apply: do not read from beyond a symbolic link

[Stefan Beller]

On Tue, Feb 3, 2015 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Mon, Feb 2, 2015 at 3:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> +       test_must_fail git apply --index patch
>>> +
>>> +'
>>
>> Is the empty line between the last test_must_fail and the closing `'`
>> intentional?
>
> I think I just mimicked the previous existing one, but I can go with
> the version without.

It was really a honest question. I've seen and written and sent tests
without such an ending
empty line and it seemed to be ok.

Maybe my email was more of "Note to self: this is also a valid style
used in the test suite
at some places"

Re: [PATCH v2 3/4] apply: do not read from beyond a symbolic link

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Feb 3, 2015 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> On Mon, Feb 2, 2015 at 3:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> +       test_must_fail git apply --index patch
>>>> +
>>>> +'
>>>
>>> Is the empty line between the last test_must_fail and the closing `'`
>>> intentional?
>>
>> I think I just mimicked the previous existing one, but I can go with
>> the version without.
>
> It was really a honest question. I've seen and written and sent
> tests without such an ending empty line and it seemed to be ok.

It was a honest answer, too.

I see existing new-style tests [*1*] that have and/or lack the empty
line at the end of the command, and I do not have particular
preference either way.  Having these variations did not bother me
too much, at least until now.  We might want to make them consistent,
and the time to be careful is when we add new tests, so it was very
valid for you to bring it up against this patch.  But I do not know
if we as the Git developer community have preference either way, and
I myself don't either, so...


[Footnote]

*1* The really ancient style we want to clean-up goes like

        cmd for setup > expect
        
        test_expect_success \
                'title of the test' \
                'command && more command &&
                 yet more command > actual &&
                 test_cmp expect actual'

        cmd for cleanup

    and we want them to read more like

        test_expect_success 'title of the test' '
                test_when_finished "cmd for cleanup" &&
                cmd for setup >expect &&
                command &&
                more command &&
                yet more command >actual &&
                test_cmp expect actual
        '

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

[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 created 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 the
resulting tree that the patch would create by inspecting all the
patches in the input and then the target of patch application
(either the index or the working tree).

This way, we catch a mischief or a mistake to add a symbolic link
path/to/dir and a file path/to/dir/file at the same time, while
allowing a valid patch that removes a symbolic link path/to/dir and
then adds a file path/to/dir/file.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c                 | 109 ++++++++++++++++++++++++++++++++++++++++
 t/t4122-apply-symlink-inside.sh |  70 ++++++++++++++++++++++++++
 t/t4139-apply-escape.sh         |   6 +--
 3 files changed, 182 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 60d821c..a760d97 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3486,6 +3486,103 @@ static int check_to_create(const char *new_name, int ok_if_exists)
 	return 0;
 }
 
+/*
+ * We need to keep track of how symlinks in the preimage are
+ * manipulated by the patches.  A patch to add a/b/c where a/b
+ * is a symlink should not be allowed to affect the directory
+ * the symlink points at, but if the same patch removes a/b,
+ * it is perfectly fine, as the patch removes a/b to make room
+ * to create a directory a/b so that a/b/c can be created.
+ */
+static struct string_list symlink_changes;
+#define SYMLINK_GOES_AWAY 01
+#define SYMLINK_IN_RESULT 02
+
+static uintptr_t register_symlink_changes(const char *path, uintptr_t what)
+{
+	struct string_list_item *ent;
+
+	ent = string_list_lookup(&symlink_changes, path);
+	if (!ent) {
+		ent = string_list_insert(&symlink_changes, path);
+		ent->util = (void *)0;
+	}
+	ent->util = (void *)(what | ((uintptr_t)ent->util));
+	return (uintptr_t)ent->util;
+}
+
+static uintptr_t check_symlink_changes(const char *path)
+{
+	struct string_list_item *ent;
+
+	ent = string_list_lookup(&symlink_changes, path);
+	if (!ent)
+		return 0;
+	return (uintptr_t)ent->util;
+}
+
+static void prepare_symlink_changes(struct patch *patch)
+{
+	for ( ; patch; patch = patch->next) {
+		if ((patch->old_name && S_ISLNK(patch->old_mode)) &&
+		    (patch->is_rename || patch->is_delete))
+			/* the symlink at patch->old_name is removed */
+			register_symlink_changes(patch->old_name, SYMLINK_GOES_AWAY);
+
+		if (patch->new_name && S_ISLNK(patch->new_mode))
+			/* the symlink at patch->new_name is created or remains */
+			register_symlink_changes(patch->new_name, SYMLINK_IN_RESULT);
+	}
+}
+
+static int path_is_beyond_symlink_1(struct strbuf *name)
+{
+	do {
+		unsigned int change;
+
+		while (--name->len && name->buf[name->len] != '/')
+			; /* scan backwards */
+		if (!name->len)
+			break;
+		name->buf[name->len] = '\0';
+		change = check_symlink_changes(name->buf);
+		if (change & SYMLINK_IN_RESULT)
+			return 1;
+		if (change & SYMLINK_GOES_AWAY)
+			/*
+			 * This cannot be "return 0", because we may
+			 * see a new one created at a higher level.
+			 */
+			continue;
+
+		/* otherwise, check the preimage */
+		if (check_index) {
+			int pos = cache_name_pos(name->buf, name->len);
+			if (0 <= pos &&
+			    S_ISLNK(active_cache[pos]->ce_mode))
+				return 1;
+		} else {
+			struct stat st;
+			if (!lstat(name->buf, &st) && S_ISLNK(st.st_mode))
+				return 1;
+		}
+	} while (1);
+	return 0;
+}
+
+static int path_is_beyond_symlink(const char *name_)
+{
+	int ret;
+	struct strbuf name = STRBUF_INIT;
+
+	assert(*name_ != '\0');
+	strbuf_addstr(&name, name_);
+	ret = path_is_beyond_symlink_1(&name);
+	strbuf_release(&name);
+
+	return ret;
+}
+
 static void die_on_unsafe_path(struct patch *patch)
 {
 	const char *old_name = NULL;
@@ -3593,6 +3690,17 @@ static int check_patch(struct patch *patch)
 	if (!unsafe_paths)
 		die_on_unsafe_path(patch);
 
+	/*
+	 * 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);
+
 	if (apply_data(patch, &st, ce) < 0)
 		return error(_("%s: patch does not apply"), name);
 	patch->rejected = 0;
@@ -3603,6 +3711,7 @@ static int check_patch_list(struct patch *patch)
 {
 	int err = 0;
 
+	prepare_symlink_changes(patch);
 	prepare_fn_table(patch);
 	while (patch) {
 		if (apply_verbosely)
diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 035c080..942c5cb 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -71,4 +71,74 @@ test_expect_success SYMLINKS 'do not read from beyond symbolic link' '
 
 '
 
+test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '
+
+	rm -rf arch/i386/dir arch/x86_64/dir &&
+	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 diff -R HEAD >del_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 confusing symbolic link
+	test_must_fail git apply patch 2>error-wt &&
+	test_i18ngrep "beyond a symbolic link" error-wt &&
+	test_path_is_missing arch/x86_64/dir &&
+	test_path_is_missing arch/i386/dir/file &&
+
+	test_must_fail git apply --index patch 2>error-ix &&
+	test_i18ngrep "beyond a symbolic link" error-ix &&
+	test_path_is_missing arch/x86_64/dir &&
+	test_path_is_missing 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-add &&
+	test_i18ngrep "beyond a symbolic link" error-wt-add &&
+	test_path_is_missing arch/i386/dir/file &&
+
+	>arch/i386/dir/file &&
+	test_must_fail git apply del_file.patch 2>error-wt-del &&
+	test_i18ngrep "beyond a symbolic link" error-wt-del &&
+	test_path_is_file arch/i386/dir/file &&
+	rm arch/i386/dir/file &&
+
+	test_must_fail git apply --index add_file.patch 2>error-ix-add &&
+	test_i18ngrep "beyond a symbolic link" error-ix-add &&
+	test_path_is_missing 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
diff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh
index 5e67179..25917cc 100755
--- a/t/t4139-apply-escape.sh
+++ b/t/t4139-apply-escape.sh
@@ -98,7 +98,7 @@ test_expect_success 'cannot del file containing .. (index)' '
 	test_path_is_file ../foo
 '
 
-test_expect_failure 'symlink escape via ..' '
+test_expect_success 'symlink escape via ..' '
 	{
 		mkpatch_symlink tmp .. &&
 		mkpatch_add tmp/foo ../foo
@@ -107,7 +107,7 @@ test_expect_failure 'symlink escape via ..' '
 	test_path_is_missing ../foo
 '
 
-test_expect_failure 'symlink escape via .. (index)' '
+test_expect_success 'symlink escape via .. (index)' '
 	{
 		mkpatch_symlink tmp .. &&
 		mkpatch_add tmp/foo ../foo
@@ -116,7 +116,7 @@ test_expect_failure 'symlink escape via .. (index)' '
 	test_path_is_missing ../foo
 '
 
-test_expect_failure 'symlink escape via absolute path' '
+test_expect_success 'symlink escape via absolute path' '
 	{
 		mkpatch_symlink tmp "$(pwd)" &&
 		mkpatch_add tmp/foo ../foo
-- 
2.3.0-rc2-164-g799cdce

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

[Jeff King]

On Mon, Feb 02, 2015 at 03:27:30PM -0800, Junio C Hamano wrote:

> +static struct string_list symlink_changes;

I notice we don't duplicate strings for this list. Are the paths we
register here always stable? I think they should be, as they are part of
the "struct patch".

> +#define SYMLINK_GOES_AWAY 01
> +#define SYMLINK_IN_RESULT 02
> +
> +static uintptr_t register_symlink_changes(const char *path, uintptr_t what)
> +{
> +	struct string_list_item *ent;
> +
> +	ent = string_list_lookup(&symlink_changes, path);
> +	if (!ent) {
> +		ent = string_list_insert(&symlink_changes, path);
> +		ent->util = (void *)0;
> +	}
> +	ent->util = (void *)(what | ((uintptr_t)ent->util));
> +	return (uintptr_t)ent->util;
> +}

I was surprised to see this as a bit-field and not a "current state" as
we walk through the set of patches to apply. 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, but we'll reject. I suspect this is making things
much simpler for you, because now we don't have to worry about order of
application that we were discussing the other day. If that is the
reason, then I think patches like the above are an acceptable casualty.
It seems rather far-fetched in the first place for a real patch (as
opposed to a mischievous one).

> +	/*
> +	 * 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 (and I am not clear on how 3/4 handles deleting from a patch
on the far side of a symlink we have just created).

It does seem to work, though. I'm just not sure how. :)

Here's the test addition I came up with, because it didn't look like we
were covering this case. 

diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 942c5cb..fbba8dd 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -89,6 +89,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '
 	rm -fr arch/x86_64/dir &&
 
 	cat add_symlink.patch add_file.patch >patch &&
+	cat add_symlink.patch del_file.patch >tricky_del &&
 
 	mkdir arch/i386/dir
 '
@@ -112,6 +113,20 @@ test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '
 	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
+
+	>arch/i386/dir/file &&
+	git add arch/i386/dir/file &&
+	test_must_fail git apply tricky_del &&
+	test_path_is_file arch/i386/dir/file &&
+
+	test_must_fail git apply --index tricky_del &&
+	test_path_is_file arch/i386/dir/file &&
+	test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+	git ls-files --error-unmatch arch/i386/dir &&
+
+	test_must_fail git apply --cached tricky_del &&
+	test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+	git ls-files --error-unmatch arch/i386/dir
 '
 
 test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '
@@ -125,6 +140,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '
 	test_i18ngrep "beyond a symbolic link" error-wt-add &&
 	test_path_is_missing arch/i386/dir/file &&
 
+	mkdir arch/i386/dir &&
 	>arch/i386/dir/file &&
 	test_must_fail git apply del_file.patch 2>error-wt-del &&
 	test_i18ngrep "beyond a symbolic link" error-wt-del &&

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Feb 02, 2015 at 03:27:30PM -0800, Junio C Hamano wrote:
>
>> +static struct string_list symlink_changes;
>
> I notice we don't duplicate strings for this list. Are the paths we
> register here always stable? I think they should be, as they are part of
> the "struct patch".

Yeah, and I also forgot to free this string-list itself.  Needs
fixing.

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

"create B by copying A" and "modify A in-place" can appear in the
same patchset in any order, and the new file B will have the
contents from the original A, not the result of modifying A
in-place, which is what will be in the resulting A.  That is how
"git diff" expresses renames and copies, and that is why rearranging
the patchset using "git diff -Oorderfile" is safe.

> but we'll reject. I suspect this is making things
> much simpler for you, because now we don't have to worry about order of
> application that we were discussing the other day.

It is not "now this new decision made things simpler".  "git diff"
output and "git apply" application have been designed to work that
way from day one.  At least from day one of rename/copy feature.

We probably should start thinking about ripping out the fn_table[]
crud.  It fundamentally cannot correctly work on an input that
concatenates more than one "git diff" outputs that have renames
and/or copies of the same file, and it never will, and that is not
due to a bug in the implementation.

The reason why the incremental application is a fundamentally flawed
concept in the context of "git apply" is because you cannot tell the
boundary between the original "git diff" outputs.

Imagine that you have three versions, A, B and C, and the original
two "git diff -M A B" and "git diff -M B C" output said this:

    (A -> B)
    edit X in place and add two lines at the beginning
    create Z by copying X

    (B -> C)
    create Y by renaming X and add a line at the end

If you take "git diff -M A C", it should say:

    (A -> C)
    edit X in place and add two lines at the beginning
    create Y by copying X and add two lines at the beginning
        and a line at the end
    create Z by copying X

Now, if you concatenate two "git diff" outputs and feed it to "git
apply", you would want it to express a patchset that goes from A to
C, but think if you can really get such a semantics.

    edit X in place and add two lines at the beginning
    create Z by copying X
    create Y by renaming X and add a line at the end

You fundamentally cannot tell that Z needs to be a copy of X before
the change to X (which is what the usual "git apply" does), but Y
needs to start from a copy of X after the change to X.  There is no
clue to tell "git apply" that there is a boundary between the first
two operations and the third one.  It is impossible for the
concatenated patch to express the same thing as "(A -> C)" patch
does, without inventng some "I am now switching to a new basis"
marker in the "git apply" input stream.

>> +	/*
>> +	 * 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.

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Here's the test addition I came up with, because it didn't look like we
> were covering this case. 

Thanks.

> diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
> index 942c5cb..fbba8dd 100755
> --- a/t/t4122-apply-symlink-inside.sh
> +++ b/t/t4122-apply-symlink-inside.sh
> @@ -89,6 +89,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '
>  	rm -fr arch/x86_64/dir &&
>  
>  	cat add_symlink.patch add_file.patch >patch &&
> +	cat add_symlink.patch del_file.patch >tricky_del &&

This new patch

 (1) creates a symlink arch/x86_64/dir pointing at ../i386/dir
 (2) deletes arch/x86_64/dir/file

It can be a valid patch to be applied to a tree where arch/x86_64/dir/file
is in the index (either as a regular file, a symlink, or even a submodule)
and nothing else is in arch/x86_64/dir directory.


> @@ -112,6 +113,20 @@ test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '
>  	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
> +
> +	>arch/i386/dir/file &&
> +	git add arch/i386/dir/file &&

At this point, the target of the patch application has:

	arch/i386/boot/Makefile
	arch/i386/dir/file
	arch/x86_64/boot/Makefile

all of which are regular files.  The index and the working tree
match.

> +	test_must_fail git apply tricky_del &&

The reason why this does not apply has nothing to do with the topic
of this series, I think.  It wants to delete arch/x86_64/dir/file,
which does not exist in the target, and the patch is rejected.

It is a good test to make sure that we do not "incrementally" apply
and get fooled by arch/x86_64/dir that will become a symbolic link,
making arch/x86_64/dir/file to appear as arch/i386/dir/file that
does exist in the preimage.

> +	test_path_is_file arch/i386/dir/file &&

When we reject the entire patch, we do so without touching the
outside world, of course ;-), which is good.

> +	test_must_fail git apply --index tricky_del &&
> +	test_path_is_file arch/i386/dir/file &&
> +	test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
> +	git ls-files --error-unmatch arch/i386/dir &&
> +
> +	test_must_fail git apply --cached tricky_del &&
> +	test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
> +	git ls-files --error-unmatch arch/i386/dir
>  '

In both of the above, "git apply" rejects its input for the same
reason.  The file it wants to remove does not exist in the target.

>  test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '
> @@ -125,6 +140,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link (existing)' '
>  	test_i18ngrep "beyond a symbolic link" error-wt-add &&
>  	test_path_is_missing arch/i386/dir/file &&
>  
> +	mkdir arch/i386/dir &&

Thanks for spotting this one ;-)

>  	>arch/i386/dir/file &&
>  	test_must_fail git apply del_file.patch 2>error-wt-del &&

del_file.patch wants to remove arch/x86_64/dir/file, and arch/x86_64/dir
is a symbolic link to ../i386/dir in the target at this point, so it
is trying to delete beyond the symbolic link, which gets rejected by
this series.  Good.

>  	test_i18ngrep "beyond a symbolic link" error-wt-del &&

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

[Eric Sunshine]

On Tue, Feb 3, 2015 at 4:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>> diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
>> index 942c5cb..fbba8dd 100755
>> --- a/t/t4122-apply-symlink-inside.sh
>> +++ b/t/t4122-apply-symlink-inside.sh
>> @@ -112,6 +113,20 @@ test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '
>>       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

Broken &&-chain.

>> +
>> +     >arch/i386/dir/file &&
>> +     git add arch/i386/dir/file &&
>
> At this point, the target of the patch application has:
>
>         arch/i386/boot/Makefile
>         arch/i386/dir/file
>         arch/x86_64/boot/Makefile
>
> all of which are regular files.  The index and the working tree
> match.

[PATCH v3 0/4] "git apply" safety

[Junio C Hamano]

The design and implementation haven't changed.

This round primarily is to update documentation and comments with
rephrases and grammofixes and tighten some tests.

The previous round begins here:

  http://thread.gmane.org/gmane.comp.version-control.git/263291

Junio C Hamano (4):
  apply: reject input that touches outside the working area
  apply: do not read from the filesystem under --index
  apply: do not read from beyond a symbolic link
  apply: do not touch a file beyond a symbolic link

 Documentation/git-apply.txt     |  15 ++++-
 builtin/apply.c                 | 141 +++++++++++++++++++++++++++++++++++++++-
 t/t4122-apply-symlink-inside.sh | 106 ++++++++++++++++++++++++++++++
 t/t4139-apply-escape.sh         | 137 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 397 insertions(+), 2 deletions(-)
 create mode 100755 t/t4139-apply-escape.sh

-- 
2.3.0-rc2-168-g106c876

[PATCH v3 1/4] apply: reject input that touches outside the working area

[Junio C Hamano]

By default, a patch that affects outside the working area
(either a Git controlled working tree, or the current working
directory when "git apply" is used as a replacement of GNU
patch) rejected as a mistake (or a mischief); Git itself does
not create such a patch, unless the user bends over backwards
and specifies a non-standard prefix to "git diff" and friends.

When `git apply` is used without either the `--index` or the
`--cached` option as a "better GNU patch", the user can pass
the `--unsafe-paths` option to override this safety check. This
option has no effect when `--index` or `--cached` is in use.

The new test was stolen from Jeff King with slight enhancements.
Note that a few new tests for touching outside the working area by
following a symbolic link are still expected to fail at this step,
but will be fixed in later steps.

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

 * Documentation rephrased with grammofixes courtesy of Peff.

 Documentation/git-apply.txt |  15 ++++-
 builtin/apply.c             |  26 +++++++++
 t/t4139-apply-escape.sh     | 137 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 177 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..a2cdc4a 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] [--unsafe-paths] [<patch>...]
 
 DESCRIPTION
 -----------
@@ -229,6 +229,19 @@ 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`.
 
+--unsafe-paths::
+	By default, a patch that affects outside the working area
+	(either a Git controlled working tree, or the current working
+	directory when "git apply" is used as a replacement of GNU
+	patch) rejected as a mistake (or a mischief); Git itself does
+	not create such a patch, unless the user bends over backwards
+	and specifies a nonstandard prefix to "git diff" and friends.
++
+When `git apply` is used without either the `--index` or the `--cached`
+option as a "better GNU patch", the user can pass the `--unsafe-paths`
+option to override this safety check.  This option has no effect when
+`--index` or `--cached` is in use.
+
 Configuration
 -------------
 
diff --git a/builtin/apply.c b/builtin/apply.c
index ef32e4f..c751ddf 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 unsafe_paths;
 static const char *fake_ancestor;
 static int line_termination = '\n';
 static unsigned int p_context = UINT_MAX;
@@ -3483,6 +3484,23 @@ static int check_to_create(const char *new_name, int ok_if_exists)
 	return 0;
 }
 
+static void die_on_unsafe_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.
@@ -3570,6 +3588,9 @@ static int check_patch(struct patch *patch)
 		}
 	}
 
+	if (!unsafe_paths)
+		die_on_unsafe_path(patch);
+
 	if (apply_data(patch, &st, ce) < 0)
 		return error(_("%s: patch does not apply"), name);
 	patch->rejected = 0;
@@ -4379,6 +4400,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, "unsafe-paths", &unsafe_paths,
+			N_("accept a patch to touch outside the working area")),
 		OPT_BOOL(0, "apply", &force_apply,
 			N_("also apply the patch (use with --stat/--summary/--check)")),
 		OPT_BOOL('3', "3way", &threeway,
@@ -4451,6 +4474,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)
 			die(_("--cached outside a repository"));
 		check_index = 1;
 	}
+	if (check_index)
+		unsafe_paths = 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..5e67179
--- /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 --unsafe-paths' '
+	mkpatch_add ../foo >patch &&
+	git apply --unsafe-paths 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 --unsafe-paths (index)' '
+	mkpatch_add ../foo >patch &&
+	test_must_fail git apply --index --unsafe-paths 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 --unsafe-paths' '
+	mkpatch_del ../foo >patch &&
+	git apply --unsafe-paths 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_failure '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_failure '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_failure '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-168-g106c876

[PATCH v3 2/4] apply: do not read from the filesystem under --index

[Junio C Hamano]

We currently read the preimage to apply a patch from the index only
when the --cached option is given.  Do so also when the command is
running under the --index option.  With --index, the index entry and
the working tree file for a path that is involved in a patch must be
identical, so this should not affect the result, but by reading from
the index, we will get the protection to avoid reading an unintended
path beyond a symbolic link automatically.

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

 * Same as v2

 builtin/apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index c751ddf..05eaf54 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3136,7 +3136,7 @@ static int load_patch_target(struct strbuf *buf,
 			     const char *name,
 			     unsigned expected_mode)
 {
-	if (cached) {
+	if (cached || check_index) {
 		if (read_file_or_gitlink(ce, buf))
 			return error(_("read of %s failed"), name);
 	} else if (name) {
-- 
2.3.0-rc2-168-g106c876

[PATCH v3 3/4] apply: do not read from beyond a symbolic link

[Junio C Hamano]

We should reject a patch, whether it renames/copies dir/file to
elsewhere with or without modificiation, or updates dir/file in
place, if "dir/" part is actually a symbolic link to elsewhere,
by making sure that the code to read the preimage does not read
from a path that is beyond a symbolic link.

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

 * Same as v2

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

diff --git a/builtin/apply.c b/builtin/apply.c
index 05eaf54..60d821c 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3145,6 +3145,8 @@ static int load_patch_target(struct strbuf *buf,
 				return read_file_or_gitlink(ce, buf);
 			else
 				return SUBMODULE_PATCH_WITHOUT_INDEX;
+		} else if (has_symlink_leading_path(name, strlen(name))) {
+			return error(_("reading from '%s' beyond a symbolic link"), name);
 		} else {
 			if (read_old_data(st, name, buf))
 				return error(_("read of %s failed"), name);
diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 70b3a06..035c080 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -52,4 +52,23 @@ test_expect_success 'check result' '
 
 '
 
+test_expect_success SYMLINKS 'do not read from beyond symbolic link' '
+	git reset --hard &&
+	mkdir -p arch/x86_64/dir &&
+	>arch/x86_64/dir/file &&
+	git add arch/x86_64/dir/file &&
+	echo line >arch/x86_64/dir/file &&
+	git diff >patch &&
+	git reset --hard &&
+
+	mkdir arch/i386/dir &&
+	>arch/i386/dir/file &&
+	ln -s ../i386/dir arch/x86_64/dir &&
+
+	test_must_fail git apply patch &&
+	test_must_fail git apply --cached patch &&
+	test_must_fail git apply --index patch
+
+'
+
 test_done
-- 
2.3.0-rc2-168-g106c876

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

[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 created 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 the
resulting tree that the patch would create by inspecting all the
patches in the input and then the target of patch application
(either the index or the working tree).

This way, we catch a mischief or a mistake to add a symbolic link
path/to/dir and a file path/to/dir/file at the same time, while
allowing a valid patch that removes a symbolic link path/to/dir and
then adds a file path/to/dir/file.

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

 * In code comment to check_patch() rephrased to clarify why we
   don't have to check a is_delete patch there while we have to
   check others.

   An additional test case stolen from Peff.

 builtin/apply.c                 | 111 ++++++++++++++++++++++++++++++++++++++++
 t/t4122-apply-symlink-inside.sh |  87 +++++++++++++++++++++++++++++++
 t/t4139-apply-escape.sh         |   6 +--
 3 files changed, 201 insertions(+), 3 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 60d821c..28975e6 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3486,6 +3486,103 @@ static int check_to_create(const char *new_name, int ok_if_exists)
 	return 0;
 }
 
+/*
+ * We need to keep track of how symlinks in the preimage are
+ * manipulated by the patches.  A patch to add a/b/c where a/b
+ * is a symlink should not be allowed to affect the directory
+ * the symlink points at, but if the same patch removes a/b,
+ * it is perfectly fine, as the patch removes a/b to make room
+ * to create a directory a/b so that a/b/c can be created.
+ */
+static struct string_list symlink_changes;
+#define SYMLINK_GOES_AWAY 01
+#define SYMLINK_IN_RESULT 02
+
+static uintptr_t register_symlink_changes(const char *path, uintptr_t what)
+{
+	struct string_list_item *ent;
+
+	ent = string_list_lookup(&symlink_changes, path);
+	if (!ent) {
+		ent = string_list_insert(&symlink_changes, path);
+		ent->util = (void *)0;
+	}
+	ent->util = (void *)(what | ((uintptr_t)ent->util));
+	return (uintptr_t)ent->util;
+}
+
+static uintptr_t check_symlink_changes(const char *path)
+{
+	struct string_list_item *ent;
+
+	ent = string_list_lookup(&symlink_changes, path);
+	if (!ent)
+		return 0;
+	return (uintptr_t)ent->util;
+}
+
+static void prepare_symlink_changes(struct patch *patch)
+{
+	for ( ; patch; patch = patch->next) {
+		if ((patch->old_name && S_ISLNK(patch->old_mode)) &&
+		    (patch->is_rename || patch->is_delete))
+			/* the symlink at patch->old_name is removed */
+			register_symlink_changes(patch->old_name, SYMLINK_GOES_AWAY);
+
+		if (patch->new_name && S_ISLNK(patch->new_mode))
+			/* the symlink at patch->new_name is created or remains */
+			register_symlink_changes(patch->new_name, SYMLINK_IN_RESULT);
+	}
+}
+
+static int path_is_beyond_symlink_1(struct strbuf *name)
+{
+	do {
+		unsigned int change;
+
+		while (--name->len && name->buf[name->len] != '/')
+			; /* scan backwards */
+		if (!name->len)
+			break;
+		name->buf[name->len] = '\0';
+		change = check_symlink_changes(name->buf);
+		if (change & SYMLINK_IN_RESULT)
+			return 1;
+		if (change & SYMLINK_GOES_AWAY)
+			/*
+			 * This cannot be "return 0", because we may
+			 * see a new one created at a higher level.
+			 */
+			continue;
+
+		/* otherwise, check the preimage */
+		if (check_index) {
+			int pos = cache_name_pos(name->buf, name->len);
+			if (0 <= pos &&
+			    S_ISLNK(active_cache[pos]->ce_mode))
+				return 1;
+		} else {
+			struct stat st;
+			if (!lstat(name->buf, &st) && S_ISLNK(st.st_mode))
+				return 1;
+		}
+	} while (1);
+	return 0;
+}
+
+static int path_is_beyond_symlink(const char *name_)
+{
+	int ret;
+	struct strbuf name = STRBUF_INIT;
+
+	assert(*name_ != '\0');
+	strbuf_addstr(&name, name_);
+	ret = path_is_beyond_symlink_1(&name);
+	strbuf_release(&name);
+
+	return ret;
+}
+
 static void die_on_unsafe_path(struct patch *patch)
 {
 	const char *old_name = NULL;
@@ -3593,6 +3690,19 @@ static int check_patch(struct patch *patch)
 	if (!unsafe_paths)
 		die_on_unsafe_path(patch);
 
+	/*
+	 * 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() so we do not
+	 * have to worry about a patch marked with "is_delete" bit
+	 * here.  We however need to make sure that the patch result
+	 * is not deposited to a path that is beyond a symbolic link
+	 * here.
+	 */
+	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;
@@ -3603,6 +3713,7 @@ static int check_patch_list(struct patch *patch)
 {
 	int err = 0;
 
+	prepare_symlink_changes(patch);
 	prepare_fn_table(patch);
 	while (patch) {
 		if (apply_verbosely)
diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 035c080..1779c0a 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -71,4 +71,91 @@ test_expect_success SYMLINKS 'do not read from beyond symbolic link' '
 
 '
 
+test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '
+
+	rm -rf arch/i386/dir arch/x86_64/dir &&
+	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 diff -R HEAD >del_file.patch &&
+	git reset --hard &&
+	rm -fr arch/x86_64/dir &&
+
+	cat add_symlink.patch add_file.patch >patch &&
+	cat add_symlink.patch del_file.patch >tricky_del &&
+
+	mkdir arch/i386/dir
+'
+
+test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '
+
+	# same input creates a confusing symbolic link
+	test_must_fail git apply patch 2>error-wt &&
+	test_i18ngrep "beyond a symbolic link" error-wt &&
+	test_path_is_missing arch/x86_64/dir &&
+	test_path_is_missing arch/i386/dir/file &&
+
+	test_must_fail git apply --index patch 2>error-ix &&
+	test_i18ngrep "beyond a symbolic link" error-ix &&
+	test_path_is_missing arch/x86_64/dir &&
+	test_path_is_missing 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 &&
+
+	>arch/i386/dir/file &&
+	git add arch/i386/dir/file &&
+
+	test_must_fail git apply tricky_del &&
+	test_path_is_file arch/i386/dir/file &&
+
+	test_must_fail git apply --index tricky_del &&
+	test_path_is_file arch/i386/dir/file &&
+	test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+	git ls-files --error-unmatch arch/i386/dir &&
+
+	test_must_fail git apply --cached tricky_del &&
+	test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+	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-add &&
+	test_i18ngrep "beyond a symbolic link" error-wt-add &&
+	test_path_is_missing arch/i386/dir/file &&
+
+	mkdir arch/i386/dir &&
+	>arch/i386/dir/file &&
+	test_must_fail git apply del_file.patch 2>error-wt-del &&
+	test_i18ngrep "beyond a symbolic link" error-wt-del &&
+	test_path_is_file arch/i386/dir/file &&
+	rm arch/i386/dir/file &&
+
+	test_must_fail git apply --index add_file.patch 2>error-ix-add &&
+	test_i18ngrep "beyond a symbolic link" error-ix-add &&
+	test_path_is_missing 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
diff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh
index 5e67179..25917cc 100755
--- a/t/t4139-apply-escape.sh
+++ b/t/t4139-apply-escape.sh
@@ -98,7 +98,7 @@ test_expect_success 'cannot del file containing .. (index)' '
 	test_path_is_file ../foo
 '
 
-test_expect_failure 'symlink escape via ..' '
+test_expect_success 'symlink escape via ..' '
 	{
 		mkpatch_symlink tmp .. &&
 		mkpatch_add tmp/foo ../foo
@@ -107,7 +107,7 @@ test_expect_failure 'symlink escape via ..' '
 	test_path_is_missing ../foo
 '
 
-test_expect_failure 'symlink escape via .. (index)' '
+test_expect_success 'symlink escape via .. (index)' '
 	{
 		mkpatch_symlink tmp .. &&
 		mkpatch_add tmp/foo ../foo
@@ -116,7 +116,7 @@ test_expect_failure 'symlink escape via .. (index)' '
 	test_path_is_missing ../foo
 '
 
-test_expect_failure 'symlink escape via absolute path' '
+test_expect_success 'symlink escape via absolute path' '
 	{
 		mkpatch_symlink tmp "$(pwd)" &&
 		mkpatch_add tmp/foo ../foo
-- 
2.3.0-rc2-168-g106c876

[PATCH v4 0/4] "git apply" safety

[Junio C Hamano]

Git tracks symbolic links; e.g. you can remove files that have been
tracked in a directory "dir/file*" and then creates a symbolic link
at "dir" to point elsewhere, express such a change as a patchset and
then apply it to the original tree.  Consequently, applying a patch
to update dir/file, when you have "dir" as a symbolic link pointing
somewhere, must fail, just like a patch whose preimage does not
match what you have in tree you are trying to apply the patch to
gets rejected.  Also, we fundamentally do not like to touch a path
that contains ".git" as a path component.

This round uses cache_file_exists() in the last patch to cope with
case insensitive filesystems better.

The previous round begins here:

  http://thread.gmane.org/gmane.comp.version-control.git/263341

Junio C Hamano (4):
  apply: reject input that touches outside the working area
  apply: do not read from the filesystem under --index
  apply: do not read from beyond a symbolic link
  apply: do not touch a file beyond a symbolic link

 Documentation/git-apply.txt     |  12 +++-
 builtin/apply.c                 | 142 +++++++++++++++++++++++++++++++++++++++-
 t/t4122-apply-symlink-inside.sh | 106 ++++++++++++++++++++++++++++++
 t/t4139-apply-escape.sh         | 141 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 399 insertions(+), 2 deletions(-)
 create mode 100755 t/t4139-apply-escape.sh

-- 
2.3.0-185-g073f588

[PATCH v4 1/4] apply: reject input that touches outside the working area

[Junio C Hamano]

By default, a patch that affects outside the working area (either a
Git controlled working tree, or the current working directory when
"git apply" is used as a replacement of GNU patch) is rejected as a
mistake (or a mischief).  Git itself does not create such a patch,
unless the user bends over backwards and specifies a non-standard
prefix to "git diff" and friends.

When `git apply` is used as a "better GNU patch", the user can pass
the `--unsafe-paths` option to override this safety check. This
option has no effect when `--index` or `--cached` is in use.

The new test was stolen from Jeff King with slight enhancements.
Note that a few new tests for touching outside the working area by
following a symbolic link are still expected to fail at this step,
but will be fixed in later steps.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-apply.txt |  12 +++-
 builtin/apply.c             |  26 ++++++++
 t/t4139-apply-escape.sh     | 141 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 178 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..9489664 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] [--unsafe-paths] [<patch>...]
 
 DESCRIPTION
 -----------
@@ -229,6 +229,16 @@ 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`.
 
+--unsafe-paths::
+	By default, a patch that affects outside the working area
+	(either a Git controlled working tree, or the current working
+	directory when "git apply" is used as a replacement of GNU
+	patch) is rejected as a mistake (or a mischief).
++
+When `git apply` is used as a "better GNU patch", the user can pass
+the `--unsafe-paths` option to override this safety check.  This option
+has no effect when `--index` or `--cached` is in use.
+
 Configuration
 -------------
 
diff --git a/builtin/apply.c b/builtin/apply.c
index ef32e4f..8561236 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 unsafe_paths;
 static const char *fake_ancestor;
 static int line_termination = '\n';
 static unsigned int p_context = UINT_MAX;
@@ -3483,6 +3484,23 @@ static int check_to_create(const char *new_name, int ok_if_exists)
 	return 0;
 }
 
+static void die_on_unsafe_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.
@@ -3570,6 +3588,9 @@ static int check_patch(struct patch *patch)
 		}
 	}
 
+	if (!unsafe_paths)
+		die_on_unsafe_path(patch);
+
 	if (apply_data(patch, &st, ce) < 0)
 		return error(_("%s: patch does not apply"), name);
 	patch->rejected = 0;
@@ -4379,6 +4400,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, "unsafe-paths", &unsafe_paths,
+			N_("accept a patch that touches outside the working area")),
 		OPT_BOOL(0, "apply", &force_apply,
 			N_("also apply the patch (use with --stat/--summary/--check)")),
 		OPT_BOOL('3', "3way", &threeway,
@@ -4451,6 +4474,9 @@ int cmd_apply(int argc, const char **argv, const char *prefix_)
 			die(_("--cached outside a repository"));
 		check_index = 1;
 	}
+	if (check_index)
+		unsafe_paths = 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..02b97b3
--- /dev/null
+++ b/t/t4139-apply-escape.sh
@@ -0,0 +1,141 @@
+#!/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 create file containing ..' '
+	mkpatch_add ../foo >patch &&
+	test_must_fail git apply patch &&
+	test_path_is_missing ../foo
+'
+
+test_expect_success 'can create file containing .. with --unsafe-paths' '
+	mkpatch_add ../foo >patch &&
+	git apply --unsafe-paths patch &&
+	test_path_is_file ../foo
+'
+
+test_expect_success  'cannot create file containing .. (index)' '
+	mkpatch_add ../foo >patch &&
+	test_must_fail git apply --index patch &&
+	test_path_is_missing ../foo
+'
+
+test_expect_success  'cannot create file containing .. with --unsafe-paths (index)' '
+	mkpatch_add ../foo >patch &&
+	test_must_fail git apply --index --unsafe-paths patch &&
+	test_path_is_missing ../foo
+'
+
+test_expect_success 'cannot delete file containing ..' '
+	mkpatch_del ../foo >patch &&
+	test_must_fail git apply patch &&
+	test_path_is_file ../foo
+'
+
+test_expect_success 'can delete file containing .. with --unsafe-paths' '
+	mkpatch_del ../foo >patch &&
+	git apply --unsafe-paths patch &&
+	test_path_is_missing ../foo
+'
+
+test_expect_success 'cannot delete file containing .. (index)' '
+	mkpatch_del ../foo >patch &&
+	test_must_fail git apply --index patch &&
+	test_path_is_file ../foo
+'
+
+test_expect_failure SYMLINKS 'symlink escape via ..' '
+	{
+		mkpatch_symlink tmp .. &&
+		mkpatch_add tmp/foo ../foo
+	} >patch &&
+	test_must_fail git apply patch &&
+	test_path_is_missing tmp &&
+	test_path_is_missing ../foo
+'
+
+test_expect_failure SYMLINKS 'symlink escape via .. (index)' '
+	{
+		mkpatch_symlink tmp .. &&
+		mkpatch_add tmp/foo ../foo
+	} >patch &&
+	test_must_fail git apply --index patch &&
+	test_path_is_missing tmp &&
+	test_path_is_missing ../foo
+'
+
+test_expect_failure SYMLINKS '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 tmp &&
+	test_path_is_missing ../foo
+'
+
+test_expect_failure SYMLINKS '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 tmp &&
+	test_path_is_missing ../foo
+'
+
+test_done
-- 
2.3.0-185-g073f588

[PATCH v4 2/4] apply: do not read from the filesystem under --index

[Junio C Hamano]

We currently read the preimage to apply a patch from the index only
when the --cached option is given.  Do so also when the command is
running under the --index option.  With --index, the index entry and
the working tree file for a path that is involved in a patch must be
identical, so this should not affect the result, but by reading from
the index, we will get the protection to avoid reading an unintended
path beyond a symbolic link automatically.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 8561236..21e45a0 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3136,7 +3136,7 @@ static int load_patch_target(struct strbuf *buf,
 			     const char *name,
 			     unsigned expected_mode)
 {
-	if (cached) {
+	if (cached || check_index) {
 		if (read_file_or_gitlink(ce, buf))
 			return error(_("read of %s failed"), name);
 	} else if (name) {
-- 
2.3.0-185-g073f588

[PATCH v4 3/4] apply: do not read from beyond a symbolic link

[Junio C Hamano]

We should reject a patch, whether it renames/copies dir/file to
elsewhere with or without modificiation, or updates dir/file in
place, if "dir/" part is actually a symbolic link to elsewhere,
by making sure that the code to read the preimage does not read
from a path that is beyond a symbolic link.

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

diff --git a/builtin/apply.c b/builtin/apply.c
index 21e45a0..422e4ce 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3145,6 +3145,8 @@ static int load_patch_target(struct strbuf *buf,
 				return read_file_or_gitlink(ce, buf);
 			else
 				return SUBMODULE_PATCH_WITHOUT_INDEX;
+		} else if (has_symlink_leading_path(name, strlen(name))) {
+			return error(_("reading from '%s' beyond a symbolic link"), name);
 		} else {
 			if (read_old_data(st, name, buf))
 				return error(_("read of %s failed"), name);
diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 70b3a06..035c080 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -52,4 +52,23 @@ test_expect_success 'check result' '
 
 '
 
+test_expect_success SYMLINKS 'do not read from beyond symbolic link' '
+	git reset --hard &&
+	mkdir -p arch/x86_64/dir &&
+	>arch/x86_64/dir/file &&
+	git add arch/x86_64/dir/file &&
+	echo line >arch/x86_64/dir/file &&
+	git diff >patch &&
+	git reset --hard &&
+
+	mkdir arch/i386/dir &&
+	>arch/i386/dir/file &&
+	ln -s ../i386/dir arch/x86_64/dir &&
+
+	test_must_fail git apply patch &&
+	test_must_fail git apply --cached patch &&
+	test_must_fail git apply --index patch
+
+'
+
 test_done
-- 
2.3.0-185-g073f588

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

[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 created 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 the
resulting tree that the patch would create by inspecting all the
patches in the input and then the target of patch application
(either the index or the working tree).

This way, we catch a mischief or a mistake to add a symbolic link
path/to/dir and a file path/to/dir/file at the same time, while
allowing a valid patch that removes a symbolic link path/to/dir and
then adds a file path/to/dir/file.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c                 | 112 ++++++++++++++++++++++++++++++++++++++++
 t/t4122-apply-symlink-inside.sh |  87 +++++++++++++++++++++++++++++++
 t/t4139-apply-escape.sh         |   8 +--
 3 files changed, 203 insertions(+), 4 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 422e4ce..c2c6fda 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3486,6 +3486,104 @@ static int check_to_create(const char *new_name, int ok_if_exists)
 	return 0;
 }
 
+/*
+ * We need to keep track of how symlinks in the preimage are
+ * manipulated by the patches.  A patch to add a/b/c where a/b
+ * is a symlink should not be allowed to affect the directory
+ * the symlink points at, but if the same patch removes a/b,
+ * it is perfectly fine, as the patch removes a/b to make room
+ * to create a directory a/b so that a/b/c can be created.
+ */
+static struct string_list symlink_changes;
+#define SYMLINK_GOES_AWAY 01
+#define SYMLINK_IN_RESULT 02
+
+static uintptr_t register_symlink_changes(const char *path, uintptr_t what)
+{
+	struct string_list_item *ent;
+
+	ent = string_list_lookup(&symlink_changes, path);
+	if (!ent) {
+		ent = string_list_insert(&symlink_changes, path);
+		ent->util = (void *)0;
+	}
+	ent->util = (void *)(what | ((uintptr_t)ent->util));
+	return (uintptr_t)ent->util;
+}
+
+static uintptr_t check_symlink_changes(const char *path)
+{
+	struct string_list_item *ent;
+
+	ent = string_list_lookup(&symlink_changes, path);
+	if (!ent)
+		return 0;
+	return (uintptr_t)ent->util;
+}
+
+static void prepare_symlink_changes(struct patch *patch)
+{
+	for ( ; patch; patch = patch->next) {
+		if ((patch->old_name && S_ISLNK(patch->old_mode)) &&
+		    (patch->is_rename || patch->is_delete))
+			/* the symlink at patch->old_name is removed */
+			register_symlink_changes(patch->old_name, SYMLINK_GOES_AWAY);
+
+		if (patch->new_name && S_ISLNK(patch->new_mode))
+			/* the symlink at patch->new_name is created or remains */
+			register_symlink_changes(patch->new_name, SYMLINK_IN_RESULT);
+	}
+}
+
+static int path_is_beyond_symlink_1(struct strbuf *name)
+{
+	do {
+		unsigned int change;
+
+		while (--name->len && name->buf[name->len] != '/')
+			; /* scan backwards */
+		if (!name->len)
+			break;
+		name->buf[name->len] = '\0';
+		change = check_symlink_changes(name->buf);
+		if (change & SYMLINK_IN_RESULT)
+			return 1;
+		if (change & SYMLINK_GOES_AWAY)
+			/*
+			 * This cannot be "return 0", because we may
+			 * see a new one created at a higher level.
+			 */
+			continue;
+
+		/* otherwise, check the preimage */
+		if (check_index) {
+			struct cache_entry *ce;
+
+			ce = cache_file_exists(name->buf, name->len, ignore_case);
+			if (ce && S_ISLNK(ce->ce_mode))
+				return 1;
+		} else {
+			struct stat st;
+			if (!lstat(name->buf, &st) && S_ISLNK(st.st_mode))
+				return 1;
+		}
+	} while (1);
+	return 0;
+}
+
+static int path_is_beyond_symlink(const char *name_)
+{
+	int ret;
+	struct strbuf name = STRBUF_INIT;
+
+	assert(*name_ != '\0');
+	strbuf_addstr(&name, name_);
+	ret = path_is_beyond_symlink_1(&name);
+	strbuf_release(&name);
+
+	return ret;
+}
+
 static void die_on_unsafe_path(struct patch *patch)
 {
 	const char *old_name = NULL;
@@ -3593,6 +3691,19 @@ static int check_patch(struct patch *patch)
 	if (!unsafe_paths)
 		die_on_unsafe_path(patch);
 
+	/*
+	 * 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() so we do not
+	 * have to worry about a patch marked with "is_delete" bit
+	 * here.  We however need to make sure that the patch result
+	 * is not deposited to a path that is beyond a symbolic link
+	 * here.
+	 */
+	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;
@@ -3603,6 +3714,7 @@ static int check_patch_list(struct patch *patch)
 {
 	int err = 0;
 
+	prepare_symlink_changes(patch);
 	prepare_fn_table(patch);
 	while (patch) {
 		if (apply_verbosely)
diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh
index 035c080..1779c0a 100755
--- a/t/t4122-apply-symlink-inside.sh
+++ b/t/t4122-apply-symlink-inside.sh
@@ -71,4 +71,91 @@ test_expect_success SYMLINKS 'do not read from beyond symbolic link' '
 
 '
 
+test_expect_success SYMLINKS 'do not follow symbolic link (setup)' '
+
+	rm -rf arch/i386/dir arch/x86_64/dir &&
+	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 diff -R HEAD >del_file.patch &&
+	git reset --hard &&
+	rm -fr arch/x86_64/dir &&
+
+	cat add_symlink.patch add_file.patch >patch &&
+	cat add_symlink.patch del_file.patch >tricky_del &&
+
+	mkdir arch/i386/dir
+'
+
+test_expect_success SYMLINKS 'do not follow symbolic link (same input)' '
+
+	# same input creates a confusing symbolic link
+	test_must_fail git apply patch 2>error-wt &&
+	test_i18ngrep "beyond a symbolic link" error-wt &&
+	test_path_is_missing arch/x86_64/dir &&
+	test_path_is_missing arch/i386/dir/file &&
+
+	test_must_fail git apply --index patch 2>error-ix &&
+	test_i18ngrep "beyond a symbolic link" error-ix &&
+	test_path_is_missing arch/x86_64/dir &&
+	test_path_is_missing 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 &&
+
+	>arch/i386/dir/file &&
+	git add arch/i386/dir/file &&
+
+	test_must_fail git apply tricky_del &&
+	test_path_is_file arch/i386/dir/file &&
+
+	test_must_fail git apply --index tricky_del &&
+	test_path_is_file arch/i386/dir/file &&
+	test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+	git ls-files --error-unmatch arch/i386/dir &&
+
+	test_must_fail git apply --cached tricky_del &&
+	test_must_fail git ls-files --error-unmatch arch/x86_64/dir &&
+	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-add &&
+	test_i18ngrep "beyond a symbolic link" error-wt-add &&
+	test_path_is_missing arch/i386/dir/file &&
+
+	mkdir arch/i386/dir &&
+	>arch/i386/dir/file &&
+	test_must_fail git apply del_file.patch 2>error-wt-del &&
+	test_i18ngrep "beyond a symbolic link" error-wt-del &&
+	test_path_is_file arch/i386/dir/file &&
+	rm arch/i386/dir/file &&
+
+	test_must_fail git apply --index add_file.patch 2>error-ix-add &&
+	test_i18ngrep "beyond a symbolic link" error-ix-add &&
+	test_path_is_missing 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
diff --git a/t/t4139-apply-escape.sh b/t/t4139-apply-escape.sh
index 02b97b3..45b5660 100755
--- a/t/t4139-apply-escape.sh
+++ b/t/t4139-apply-escape.sh
@@ -98,7 +98,7 @@ test_expect_success 'cannot delete file containing .. (index)' '
 	test_path_is_file ../foo
 '
 
-test_expect_failure SYMLINKS 'symlink escape via ..' '
+test_expect_success SYMLINKS 'symlink escape via ..' '
 	{
 		mkpatch_symlink tmp .. &&
 		mkpatch_add tmp/foo ../foo
@@ -108,7 +108,7 @@ test_expect_failure SYMLINKS 'symlink escape via ..' '
 	test_path_is_missing ../foo
 '
 
-test_expect_failure SYMLINKS 'symlink escape via .. (index)' '
+test_expect_success SYMLINKS 'symlink escape via .. (index)' '
 	{
 		mkpatch_symlink tmp .. &&
 		mkpatch_add tmp/foo ../foo
@@ -118,7 +118,7 @@ test_expect_failure SYMLINKS 'symlink escape via .. (index)' '
 	test_path_is_missing ../foo
 '
 
-test_expect_failure SYMLINKS 'symlink escape via absolute path' '
+test_expect_success SYMLINKS 'symlink escape via absolute path' '
 	{
 		mkpatch_symlink tmp "$(pwd)" &&
 		mkpatch_add tmp/foo ../foo
@@ -128,7 +128,7 @@ test_expect_failure SYMLINKS 'symlink escape via absolute path' '
 	test_path_is_missing ../foo
 '
 
-test_expect_failure SYMLINKS 'symlink escape via absolute path (index)' '
+test_expect_success SYMLINKS 'symlink escape via absolute path (index)' '
 	{
 		mkpatch_symlink tmp "$(pwd)" &&
 		mkpatch_add tmp/foo ../foo
-- 
2.3.0-185-g073f588