Re: [PATCH 2/2] add: refuse to add paths beyond repository boundaries
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:56:45
Junio C Hamano [off-list ref] writes:
Ramkumar Ramachandra [off-list ref] writes:quoted
Currently, git add has the logic for refusing to add gitlinks using treat_path(), which in turn calls check_path_for_gitlink(). However, this only checks for an in-index submodule (or gitlink cache_entry). A path inside a git repository in the worktree still adds fine, and this is a bug. The logic for denying it is very similar to denying adding paths beyond symbolic links: die_if_path_beyond_symlink(). Follow its example and write a die_if_path_beyond_gitrepo() to fix this bug. Signed-off-by: Ramkumar Ramachandra <redacted> ---quoted
@@ -166,6 +166,7 @@ static const char **validate_pathspec(const char **argv, const char *prefix) const char **p; for (p = pathspec; *p; p++) { die_if_path_beyond_symlink(*p, prefix); + die_if_path_beyond_gitrepo(*p, prefix); } }diff --git a/cache.h b/cache.h index e1e8ce8..987d7f3 100644 --- a/cache.h +++ b/cache.h@@ -962,6 +962,8 @@ struct cache_def { extern int has_symlink_leading_path(const char *name, int len); +extern int has_gitrepo_leading_path(const char *name, int len);I looked at the output from "grep has_symlink_leading_path" and also for "die_if_path_beyond"; all of these places are checking "I have this multi-level path; I want to know if the path does not (should not) be part of the current project", I think. Certainly the one in the "update-index" is about the same operation as "git add" you are patching. Isn't it a better approach to _rename_ the existing function not to single out "symlink"-ness of the path first ? A symlink in the middle of such a multi-level path that leads to a place outside the project is _not_ the only way to step out of our project boundary. A directory in the middle of a multi-level path that is the top-level of the working tree of a foreign project is another way to step out of our project boundary. Perhaps die_if_path_outside_our_project() path_outside_our_project() And then update the implementation of path_outside_our_project(), which only took "symlink in the middle" into account so far, and teach it that such a "top-level of the working tree of a foreign project" is also stepping out of our project? That way, you do not have to settle on fixing the bug only in "git add" and keep the bug in "git update-index", I think. I think the hit in builtin/apply.c deals with the same "beyond symlink is outside our project" check and can be updated like so. I didn't look at the ones in diff-lib.c and dir.c so you may want to double check on what they use it for.
The first step (renaming and adjusting comments) would look like this. builtin/add.c | 6 +++--- builtin/apply.c | 8 ++++++-- builtin/check-ignore.c | 2 +- builtin/update-index.c | 4 ++-- cache.h | 4 ++-- diff-lib.c | 2 +- dir.c | 2 +- pathspec.c | 6 +++--- pathspec.h | 2 +- preload-index.c | 2 +- symlinks.c | 10 +++++----- t/t0008-ignores.sh | 2 +- 12 files changed, 27 insertions(+), 23 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index ab1c9e8..7cb80ef 100644
--- a/builtin/add.c
+++ b/builtin/add.c@@ -155,8 +155,8 @@ static void refresh(int verbose, const char **pathspec) /* * Normalizes argv relative to prefix, via get_pathspec(), and then - * runs die_if_path_beyond_symlink() on each path in the normalized - * list. + * runs die_if_path_outside_our_project() on each path in the + * normalized list. */ static const char **validate_pathspec(const char **argv, const char *prefix) {
@@ -165,7 +165,7 @@ static const char **validate_pathspec(const char **argv, const char *prefix) if (pathspec) { const char **p; for (p = pathspec; *p; p++) { - die_if_path_beyond_symlink(*p, prefix); + die_if_path_outside_our_project(*p, prefix); } }
diff --git a/builtin/apply.c b/builtin/apply.c
index 5b882d0..d0b408e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c@@ -3469,10 +3469,14 @@ static int check_to_create(const char *new_name, int ok_if_exists) * A leading component of new_name might be a symlink * that is going to be removed with this patch, but * still pointing at somewhere that has the path. - * In such a case, path "new_name" does not exist as + * Or it could be the top-level of a working tree of + * a different project that is embedded in our working + * tree. + * + * In such cases, path "new_name" does not exist as * far as git is concerned. */ - if (has_symlink_leading_path(new_name, strlen(new_name))) + if (path_outside_our_project(new_name, strlen(new_name))) return 0; return EXISTS_IN_WORKTREE;
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 0240f99..bce378d 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c@@ -88,7 +88,7 @@ static int check_ignore(const char *prefix, const char **pathspec) full_path = prefix_path(prefix, prefix ? strlen(prefix) : 0, path); full_path = check_path_for_gitlink(full_path); - die_if_path_beyond_symlink(full_path, prefix); + die_if_path_outside_our_project(full_path, prefix); if (!seen[i]) { exclude = last_exclude_matching_path(&check, full_path, -1, &dtype);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 5c7762e..7c47fa2 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c@@ -186,8 +186,8 @@ static int process_path(const char *path) struct cache_entry *ce; len = strlen(path); - if (has_symlink_leading_path(path, len)) - return error("'%s' is beyond a symbolic link", path); + if (path_outside_our_project(path, len)) + return error("'%s' is outside our working tree", path); pos = cache_name_pos(path, len); ce = pos < 0 ? NULL : active_cache[pos];
diff --git a/cache.h b/cache.h
index e1e8ce8..f6359b5 100644
--- a/cache.h
+++ b/cache.h@@ -960,8 +960,8 @@ struct cache_def { int prefix_len_stat_func; }; -extern int has_symlink_leading_path(const char *name, int len); -extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int); +extern int path_outside_our_project(const char *name, int len); +extern int threaded_path_outside_our_project(struct cache_def *, const char *, int); extern int check_leading_path(const char *name, int len); extern int has_dirs_only_path(const char *name, int len, int prefix_len); extern void schedule_dir_for_removal(const char *name, int len);
diff --git a/diff-lib.c b/diff-lib.c
index f35de0f..8aff906 100644
--- a/diff-lib.c
+++ b/diff-lib.c@@ -32,7 +32,7 @@ static int check_removed(const struct cache_entry *ce, struct stat *st) return -1; return 1; } - if (has_symlink_leading_path(ce->name, ce_namelen(ce))) + if (path_outside_our_project(ce->name, ce_namelen(ce))) return 1; if (S_ISDIR(st->st_mode)) { unsigned char sub[20];
diff --git a/dir.c b/dir.c
index 91cfd99..b90b57b 100644
--- a/dir.c
+++ b/dir.c@@ -1479,7 +1479,7 @@ int read_directory(struct dir_struct *dir, const char *path, int len, const char { struct path_simplify *simplify; - if (has_symlink_leading_path(path, len)) + if (path_outside_our_project(path, len)) return dir->nr; simplify = create_simplify(pathspec);
diff --git a/pathspec.c b/pathspec.c
index 284f397..336149f 100644
--- a/pathspec.c
+++ b/pathspec.c@@ -92,10 +92,10 @@ const char *check_path_for_gitlink(const char *path) * Dies if the given path refers to a file inside a symlinked * directory in the index. */ -void die_if_path_beyond_symlink(const char *path, const char *prefix) +void die_if_path_outside_our_project(const char *path, const char *prefix) { - if (has_symlink_leading_path(path, strlen(path))) { + if (path_outside_our_project(path, strlen(path))) { int len = prefix ? strlen(prefix) : 0; - die(_("'%s' is beyond a symbolic link"), path + len); + die(_("'%s' is outside the working tree"), path + len); } }
diff --git a/pathspec.h b/pathspec.h
index db0184a..ef816a8 100644
--- a/pathspec.h
+++ b/pathspec.h@@ -4,6 +4,6 @@ extern char *find_pathspecs_matching_against_index(const char **pathspec); extern void add_pathspec_matches_against_index(const char **pathspec, char *seen, int specs); extern const char *check_path_for_gitlink(const char *path); -extern void die_if_path_beyond_symlink(const char *path, const char *prefix); +extern void die_if_path_outside_our_project(const char *path, const char *prefix); #endif /* PATHSPEC_H */
diff --git a/preload-index.c b/preload-index.c
index 49cb08d..b3e57d4 100644
--- a/preload-index.c
+++ b/preload-index.c@@ -55,7 +55,7 @@ static void *preload_thread(void *_data) continue; if (!ce_path_match(ce, &pathspec)) continue; - if (threaded_has_symlink_leading_path(&cache, ce->name, ce_namelen(ce))) + if (threaded_path_outside_our_project(&cache, ce->name, ce_namelen(ce))) continue; if (lstat(ce->name, &st)) continue;
diff --git a/symlinks.c b/symlinks.c
index c2b41a8..baed93f 100644
--- a/symlinks.c
+++ b/symlinks.c@@ -196,19 +196,19 @@ static int lstat_cache(struct cache_def *cache, const char *name, int len, #define USE_ONLY_LSTAT 0 /* - * Return non-zero if path 'name' has a leading symlink component + * Return non-zero if path 'name' points outside the working tree */ -int threaded_has_symlink_leading_path(struct cache_def *cache, const char *name, int len) +int threaded_path_outside_our_project(struct cache_def *cache, const char *name, int len) { return lstat_cache(cache, name, len, FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) & FL_SYMLINK; } /* - * Return non-zero if path 'name' has a leading symlink component + * Return non-zero if path 'name' points outside the working tree */ -int has_symlink_leading_path(const char *name, int len) +int path_outside_our_project(const char *name, int len) { - return threaded_has_symlink_leading_path(&default_cache, name, len); + return threaded_path_outside_our_project(&default_cache, name, len); } /*
diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index 9c1bde1..3881e7d 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh@@ -397,7 +397,7 @@ test_expect_success_multi SYMLINKS 'symlink' '' ' test_expect_success_multi SYMLINKS 'beyond a symlink' '' ' test_check_ignore "a/symlink/foo" 128 && - test_stderr "fatal: '\''a/symlink/foo'\'' is beyond a symbolic link" + test_stderr "fatal: '\''a/symlink/foo'\'' is outside the working tree" ' test_expect_success_multi SYMLINKS 'beyond a symlink from subdirectory' '' '