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

[Jeff King <peff@peff.net>]

On Mon, Feb 02, 2015 at 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 &&