Re: [PATCH] apply: refuse touching a file beyond symlink
From: Jeff King <hidden>
Date: 2015-01-30 20:07:36
Also in:
lkml
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2015-01-30 · Re: [PATCH] apply: refuse touching a file beyond symlink · Junio C Hamano <hidden>
- 2015-01-30 · Re: [PATCH] apply: refuse touching a file beyond symlink · Junio C Hamano <hidden>
- 2015-01-30 · Re: [PATCH] apply: refuse touching a file beyond symlink · Jeff King <hidden>
- 2015-01-30 · Re: [PATCH] apply: refuse touching a file beyond symlink · Junio C Hamano <hidden>
- 2015-01-30 · Re: [PATCH] apply: refuse touching a file beyond symlink · Junio C Hamano <hidden>
On Fri, Jan 30, 2015 at 11:48:14AM -0800, Junio C Hamano wrote:
quoted hunk ↗ jump to hunk
Jeff King [off-list ref] writes:quoted
quoted
+ if (!patch->is_delete && path_is_beyond_symlink(patch->new_name)) + return error(_("affected file '%s' is beyond a symbolic link"), + patch->new_name);Why does this not kick in when deleting a file? If it is not OK to add across a symlink, why is it OK to delete?Hmph, adding if (patch->is_delete && path_is_beyond_symlink(patch->old_name)) return error(_("deleted file '%s' is beyond a symlink"), patch->old_name); seems to break t4114.11, which wants to apply this patch to a tree that does not have a symbolic link but a directory at 'foo/'.diff --git a/foo b/foo new file mode 120000 index 0000000..ba0e162 --- /dev/null +++ b/foo@@ -0,0 +1 @@ +bar\ No newline at end of filediff --git a/foo/baz b/foo/baz deleted file mode 100644 index 682c76b..0000000 --- a/foo/baz +++ /dev/null@@ -1 +0,0 @@ -if only I knew
Hrm. That only works in the current code because we apply the deletion in the directory (and then clean up the now-empty directory) first. So I think you would need to check the paths progressively as you apply them, since those other parts of the diff "haven't happened yet". If we take deletion as one phase and addition as another, I think you could get away with:
diff --git a/builtin/apply.c b/builtin/apply.c
index f5491cd..12c9d8e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c@@ -3549,7 +3549,7 @@ static int check_to_create(const char *new_name, int ok_if_exists) return 0; } -static int path_is_beyond_symlink(const char *name_) +static int path_is_beyond_symlink(const char *name_, int check_table) { struct strbuf name = STRBUF_INIT;
@@ -3562,7 +3562,8 @@ static int path_is_beyond_symlink(const char *name_) if (!name.len) break; name.buf[name.len] = '\0'; - previous = in_fn_table(name.buf); + + previous = check_table ? in_fn_table(name.buf) : NULL; if (previous) { if (!was_deleted(previous) && !to_be_deleted(previous) &&
@@ -3676,9 +3677,12 @@ static int check_patch(struct patch *patch) } } - if (!patch->is_delete && path_is_beyond_symlink(patch->new_name)) + if (!patch->is_delete && path_is_beyond_symlink(patch->new_name, 1)) return error(_("affected file '%s' is beyond a symbolic link"), patch->new_name); + if (patch->is_delete && path_is_beyond_symlink(patch->old_name, 0)) + return error(_("affected file '%s' is beyond a symbolic link"), + patch->old_name); if (apply_data(patch, &st, ce) < 0) return error(_("%s: patch does not apply"), name);
but I suspect we could construct a case that depends more closely on the order of application. E.g., a patch that does: 1. add foo as a symlink 2. add file foo/bar is definitely wrong. But is: 1. add file foo/bar 2. add foo as a symlink does not technically fall afoul of the symlink rules. It is a _bogus_ patch, of course, because the second part will get a D/F conflict. I am not sure if there are any legitimate patches that could run into this ordering problem, but even without it, it smells a bit funny to complain for the wrong reason. Looking at the code, though, it seems like we should be doing these checks progressively as we add entries to the fn_table. So that is doing the right thing. It is only the deletion re-ordering that trips us up. Can we reorder all deletions before all additions before calling check_patch on each? -Peff