From d089ebaad5315325d67db30176df1bbd7754fda9 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 28 Jan 2008 22:44:27 -0800 Subject: setup: sanitize absolute and funny paths in get_pathspec() The prefix_path() function called from get_pathspec() is responsible for translating list of user-supplied pathspecs to list of pathspecs that is relative to the root of the work tree. When working inside a subdirectory, the user-supplied pathspecs are taken to be relative to the current subdirectory. Among special path components in pathspecs, we used to accept and interpret only "." ("the directory", meaning a no-op) and ".." ("up one level") at the beginning. Everything else was passed through as-is. For example, if you are in Documentation/ directory of the project, you can name Documentation/howto/maintain-git.txt as: howto/maintain-git.txt ../Documentation/howto/maitain-git.txt ../././Documentation/howto/maitain-git.txt but not as: howto/./maintain-git.txt $(pwd)/howto/maintain-git.txt This patch updates prefix_path() in several ways: - If the pathspec is not absolute, prefix (i.e. the current subdirectory relative to the root of the work tree, with terminating slash, if not empty) and the pathspec is concatenated first and used in the next step. Otherwise, that absolute pathspec is used in the next step. - Then special path components "." (no-op) and ".." (up one level) are interpreted to simplify the path. It is an error to have too many ".." to cause the intermediate result to step outside of the input to this step. - If the original pathspec was not absolute, the result from the previous step is the resulting "sanitized" pathspec. Otherwise, the result from the previous step is still absolute, and it is an error if it does not begin with the directory that corresponds to the root of the work tree. The directory is stripped away from the result and is returned. - In any case, the resulting pathspec in the array get_pathspec() returns omit the ones that caused errors. With this patch, the last two examples also behave as expected. Signed-off-by: Junio C Hamano --- builtin-ls-files.c | 11 +++- builtin-mv.c | 4 +- setup.c | 158 +++++++++++++++++++++++++++++++++++++++-------------- t/t7010-setup.sh | 117 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 245 insertions(+), 45 deletions(-) create mode 100755 t/t7010-setup.sh diff --git a/builtin-ls-files.c b/builtin-ls-files.c index 0f0ab2da16..3801cf4cec 100644 --- a/builtin-ls-files.c +++ b/builtin-ls-files.c @@ -572,8 +572,17 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix) pathspec = get_pathspec(prefix, argv + i); /* Verify that the pathspec matches the prefix */ - if (pathspec) + if (pathspec) { + if (argc != i) { + int cnt; + for (cnt = 0; pathspec[cnt]; cnt++) + ; + if (cnt != (argc - i)) + exit(1); /* error message already given */ + } prefix = verify_pathspec(prefix); + } else if (argc != i) + exit(1); /* error message already given */ /* Treat unmatching pathspec elements as errors */ if (pathspec && error_unmatch) { diff --git a/builtin-mv.c b/builtin-mv.c index 990e21355d..94f6dd2aad 100644 --- a/builtin-mv.c +++ b/builtin-mv.c @@ -164,7 +164,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) } dst = add_slash(dst); - dst_len = strlen(dst) - 1; + dst_len = strlen(dst); for (j = 0; j < last - first; j++) { const char *path = @@ -172,7 +172,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix) source[argc + j] = path; destination[argc + j] = prefix_path(dst, dst_len, - path + length); + path + length + 1); modes[argc + j] = INDEX; } argc += last - first; diff --git a/setup.c b/setup.c index adede16a4d..23c9a11be5 100644 --- a/setup.c +++ b/setup.c @@ -4,51 +4,118 @@ static int inside_git_dir = -1; static int inside_work_tree = -1; -const char *prefix_path(const char *prefix, int len, const char *path) +static int sanitary_path_copy(char *dst, const char *src) { - const char *orig = path; + char *dst0 = dst; + + if (*src == '/') { + *dst++ = '/'; + while (*src == '/') + src++; + } + for (;;) { - char c; - if (*path != '.') - break; - c = path[1]; - /* "." */ - if (!c) { - path++; - break; + char c = *src; + + /* + * A path component that begins with . could be + * special: + * (1) "." and ends -- ignore and terminate. + * (2) "./" -- ignore them, eat slash and continue. + * (3) ".." and ends -- strip one and terminate. + * (4) "../" -- strip one, eat slash and continue. + */ + if (c == '.') { + switch (src[1]) { + case '\0': + /* (1) */ + src++; + break; + case '/': + /* (2) */ + src += 2; + while (*src == '/') + src++; + continue; + case '.': + switch (src[2]) { + case '\0': + /* (3) */ + src += 2; + goto up_one; + case '/': + /* (4) */ + src += 3; + while (*src == '/') + src++; + goto up_one; + } + } } - /* "./" */ + + /* copy up to the next '/', and eat all '/' */ + while ((c = *src++) != '\0' && c != '/') + *dst++ = c; if (c == '/') { - path += 2; - continue; - } - if (c != '.') + *dst++ = c; + while (c == '/') + c = *src++; + src--; + } else if (!c) break; - c = path[2]; - if (!c) - path += 2; - else if (c == '/') - path += 3; - else - break; - /* ".." and "../" */ - /* Remove last component of the prefix */ - do { - if (!len) - die("'%s' is outside repository", orig); - len--; - } while (len && prefix[len-1] != '/'); continue; + + up_one: + /* + * dst0..dst is prefix portion, and dst[-1] is '/'; + * go up one level. + */ + dst -= 2; /* go past trailing '/' if any */ + if (dst < dst0) + return -1; + while (1) { + if (dst <= dst0) + break; + c = *dst--; + if (c == '/') { + dst += 2; + break; + } + } } - if (len) { - int speclen = strlen(path); - char *n = xmalloc(speclen + len + 1); + *dst = '\0'; + return 0; +} - memcpy(n, prefix, len); - memcpy(n + len, path, speclen+1); - path = n; +const char *prefix_path(const char *prefix, int len, const char *path) +{ + const char *orig = path; + char *sanitized = xmalloc(len + strlen(path) + 1); + if (*orig == '/') + strcpy(sanitized, path); + else { + if (len) + memcpy(sanitized, prefix, len); + strcpy(sanitized + len, path); } - return path; + if (sanitary_path_copy(sanitized, sanitized)) + goto error_out; + if (*orig == '/') { + const char *work_tree = get_git_work_tree(); + size_t len = strlen(work_tree); + size_t total = strlen(sanitized) + 1; + if (strncmp(sanitized, work_tree, len) || + (sanitized[len] != '\0' && sanitized[len] != '/')) { + error_out: + error("'%s' is outside repository", orig); + free(sanitized); + return NULL; + } + if (sanitized[len] == '/') + len++; + memmove(sanitized, sanitized + len, total - len); + } + return sanitized; } /* @@ -114,7 +181,7 @@ void verify_non_filename(const char *prefix, const char *arg) const char **get_pathspec(const char *prefix, const char **pathspec) { const char *entry = *pathspec; - const char **p; + const char **src, **dst; int prefixlen; if (!prefix && !entry) @@ -128,12 +195,19 @@ const char **get_pathspec(const char *prefix, const char **pathspec) } /* Otherwise we have to re-write the entries.. */ - p = pathspec; + src = pathspec; + dst = pathspec; prefixlen = prefix ? strlen(prefix) : 0; - do { - *p = prefix_path(prefix, prefixlen, entry); - } while ((entry = *++p) != NULL); - return (const char **) pathspec; + while (*src) { + const char *p = prefix_path(prefix, prefixlen, *src); + if (p) + *(dst++) = p; + src++; + } + *dst = NULL; + if (!*pathspec) + return NULL; + return pathspec; } /* diff --git a/t/t7010-setup.sh b/t/t7010-setup.sh new file mode 100755 index 0000000000..da20ba514a --- /dev/null +++ b/t/t7010-setup.sh @@ -0,0 +1,117 @@ +#!/bin/sh + +test_description='setup taking and sanitizing funny paths' + +. ./test-lib.sh + +test_expect_success setup ' + + mkdir -p a/b/c a/e && + D=$(pwd) && + >a/b/c/d && + >a/e/f + +' + +test_expect_success 'git add (absolute)' ' + + git add "$D/a/b/c/d" && + git ls-files >current && + echo a/b/c/d >expect && + diff -u expect current + +' + + +test_expect_success 'git add (funny relative)' ' + + rm -f .git/index && + ( + cd a/b && + git add "../e/./f" + ) && + git ls-files >current && + echo a/e/f >expect && + diff -u expect current + +' + +test_expect_success 'git rm (absolute)' ' + + rm -f .git/index && + git add a && + git rm -f --cached "$D/a/b/c/d" && + git ls-files >current && + echo a/e/f >expect && + diff -u expect current + +' + +test_expect_success 'git rm (funny relative)' ' + + rm -f .git/index && + git add a && + ( + cd a/b && + git rm -f --cached "../e/./f" + ) && + git ls-files >current && + echo a/b/c/d >expect && + diff -u expect current + +' + +test_expect_success 'git ls-files (absolute)' ' + + rm -f .git/index && + git add a && + git ls-files "$D/a/e/../b" >current && + echo a/b/c/d >expect && + diff -u expect current + +' + +test_expect_success 'git ls-files (relative #1)' ' + + rm -f .git/index && + git add a && + ( + cd a/b && + git ls-files "../b/c" + ) >current && + echo c/d >expect && + diff -u expect current + +' + +test_expect_success 'git ls-files (relative #2)' ' + + rm -f .git/index && + git add a && + ( + cd a/b && + git ls-files --full-name "../e/f" + ) >current && + echo a/e/f >expect && + diff -u expect current + +' + +test_expect_success 'git ls-files (relative #3)' ' + + rm -f .git/index && + git add a && + ( + cd a/b && + if git ls-files "../e/f" + then + echo Gaah, should have failed + exit 1 + else + : happy + fi + ) + +' + +test_done -- cgit v1.2.3 From 097971f5f564dbd832eea774ae0cdcfa03ba35ac Mon Sep 17 00:00:00 2001 From: Robin Rosenberg Date: Fri, 1 Feb 2008 05:07:04 +0100 Subject: Make blame accept absolute paths Blame did not always use prefix_path. Signed-off-by: Robin Rosenberg Signed-off-by: Junio C Hamano --- builtin-blame.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/builtin-blame.c b/builtin-blame.c index 9b4c02e87f..dea640d17b 100644 --- a/builtin-blame.c +++ b/builtin-blame.c @@ -1894,9 +1894,7 @@ static unsigned parse_score(const char *arg) static const char *add_prefix(const char *prefix, const char *path) { - if (!prefix || !prefix[0]) - return path; - return prefix_path(prefix, strlen(prefix), path); + return prefix_path(prefix, prefix ? strlen(prefix) : 0, path); } /* -- cgit v1.2.3 From 1abf0950638d4f3279d059a1365da9c253d5718a Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 1 Feb 2008 03:13:10 -0800 Subject: git-add: adjust to the get_pathspec() changes. We would need to notice and fail if command line had a nonsense pathspec. Earlier get_pathspec() returned all the inputs including bad ones, but the new one issues warnings and removes offending ones from its return value, so the callers need to be adjusted to notice it. Additional test scripts were initially from Robin Rosenberg, further fixed. Signed-off-by: Junio C Hamano --- builtin-add.c | 12 ++++++++++++ t/t7010-setup.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/builtin-add.c b/builtin-add.c index 4a91e3eb11..820110e085 100644 --- a/builtin-add.c +++ b/builtin-add.c @@ -228,6 +228,18 @@ int cmd_add(int argc, const char **argv, const char *prefix) goto finish; } + if (*argv) { + /* Was there an invalid path? */ + if (pathspec) { + int num; + for (num = 0; pathspec[num]; num++) + ; /* just counting */ + if (argc != num) + exit(1); /* error message already given */ + } else + exit(1); /* error message already given */ + } + fill_directory(&dir, pathspec, ignored_too); if (show_only) { diff --git a/t/t7010-setup.sh b/t/t7010-setup.sh index da20ba514a..e809e0e2c9 100755 --- a/t/t7010-setup.sh +++ b/t/t7010-setup.sh @@ -114,4 +114,51 @@ test_expect_success 'git ls-files (relative #3)' ' ' +test_expect_success 'commit using absolute path names' ' + git commit -m "foo" && + echo aa >>a/b/c/d && + git commit -m "aa" "$(pwd)/a/b/c/d" +' + +test_expect_success 'log using absolute path names' ' + echo bb >>a/b/c/d && + git commit -m "bb" $(pwd)/a/b/c/d && + + git log a/b/c/d >f1.txt && + git log "$(pwd)/a/b/c/d" >f2.txt && + diff -u f1.txt f2.txt +' + +test_expect_success 'blame using absolute path names' ' + git blame a/b/c/d >f1.txt && + git blame "$(pwd)/a/b/c/d" >f2.txt && + diff -u f1.txt f2.txt +' + +test_expect_success 'setup deeper work tree' ' + test_create_repo tester +' + +test_expect_success 'add a directory outside the work tree' '( + cd tester && + d1="$(cd .. ; pwd)" && + git add "$d1" +)' + +test_expect_success 'add a file outside the work tree, nasty case 1' '( + cd tester && + f="$(pwd)x" && + echo "$f" && + touch "$f" && + git add "$f" +)' + +test_expect_success 'add a file outside the work tree, nasty case 2' '( + cd tester && + f="$(pwd | sed "s/.$//")x" && + echo "$f" && + touch "$f" && + git add "$f" +)' + test_done -- cgit v1.2.3 From 744dacd3f5045240a304e687f3ef7135398e7865 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 3 Feb 2008 23:59:17 -0800 Subject: builtin-mv: minimum fix to avoid losing files An incorrect command "git mv subdir /outer/space" threw the subdirectory to outside of the repository and then noticed that /outer/space/subdir/ would be outside of the repository. The error checking is backwards. This fixes the issue by being careful about use of the return value of get_pathspec(). Since the implementation already has handcrafted loop to munge each path on the command line, we use prefix_path() instead. Signed-off-by: Junio C Hamano --- builtin-mv.c | 6 +++++- t/t7001-mv.sh | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/builtin-mv.c b/builtin-mv.c index 94f6dd2aad..68aa2a68bb 100644 --- a/builtin-mv.c +++ b/builtin-mv.c @@ -19,6 +19,7 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec, int count, int base_name) { int i; + int len = prefix ? strlen(prefix) : 0; const char **result = xmalloc((count + 1) * sizeof(const char *)); memcpy(result, pathspec, count * sizeof(const char *)); result[count] = NULL; @@ -32,8 +33,11 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec, if (last_slash) result[i] = last_slash + 1; } + result[i] = prefix_path(prefix, len, result[i]); + if (!result[i]) + exit(1); /* error already given */ } - return get_pathspec(prefix, result); + return result; } static void show_list(const char *label, struct path_list *list) diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index b1243b4163..fa382c58da 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -118,4 +118,42 @@ test_expect_success "Sergey Vlasov's test case" ' git mv ab a ' +test_expect_success 'absolute pathname' '( + + rm -fr mine && + mkdir mine && + cd mine && + test_create_repo one && + cd one && + mkdir sub && + >sub/file && + git add sub/file && + + git mv sub "$(pwd)/in" && + ! test -d sub && + test -d in && + git ls-files --error-unmatch in/file + + +)' + +test_expect_success 'absolute pathname outside should fail' '( + + rm -fr mine && + mkdir mine && + cd mine && + out=$(pwd) && + test_create_repo one && + cd one && + mkdir sub && + >sub/file && + git add sub/file && + + ! git mv sub "$out/out" && + test -d sub && + ! test -d ../in && + git ls-files --error-unmatch sub/file + +)' + test_done -- cgit v1.2.3