From f02033f1d02a55b32815a4702389e4d492f38bcc Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 4 Feb 2014 15:25:15 +0100 Subject: [PATCH 1/7] t3004: add test for ls-files on symlinks via absolute paths When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead. This causes most high-level functions to misbehave when acting on symlinks given via absolute paths. For example $ git add /dir/repo/symlink attempts to add the target of the symlink rather than the symlink itself, which is usually not what the user intends to do. This is a regression introduced by 18e051a: setup: translate symlinks in filename when using absolute paths (which did not take symlinks inside the work tree into consideration). Add a known-breakage test using the ls-files function, checking both if the symlink leads to a target in the same directory, and a target in the above directory. Signed-off-by: Martin Erik Werner Tested-by: Martin Erik Werner Signed-off-by: Junio C Hamano --- t/t3004-ls-files-basic.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh index 8d9bc3c2af..e20c077e0f 100755 --- a/t/t3004-ls-files-basic.sh +++ b/t/t3004-ls-files-basic.sh @@ -36,4 +36,21 @@ test_expect_success 'ls-files -h in corrupt repository' ' test_i18ngrep "[Uu]sage: git ls-files " broken/usage ' +test_expect_failure SYMLINKS 'ls-files with absolute paths to symlinks' ' + mkdir subs && + ln -s nosuch link && + ln -s ../nosuch subs/link && + git add link subs/link && + git ls-files -s link subs/link >expect && + git ls-files -s "$(pwd)/link" "$(pwd)/subs/link" >actual && + test_cmp expect actual && + + ( + cd subs && + git ls-files -s link >../expect && + git ls-files -s "$(pwd)/link" >../actual + ) && + test_cmp expect actual +' + test_done From 74af95d6aac03e1a1227c78d0d0ea8bbc2fa2b4d Mon Sep 17 00:00:00 2001 From: Martin Erik Werner Date: Tue, 4 Feb 2014 15:25:16 +0100 Subject: [PATCH 2/7] t0060: add test for prefix_path on symlinks via absolute paths When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead. This applies to most high-level commands but prefix_path is the common denominator for all of them. Add a known-breakage tests using the prefix_path function, which currently uses real_path, causing the dereference. Signed-off-by: Martin Erik Werner Reviewed-by: Duy Nguyen Signed-off-by: Junio C Hamano --- t/t0060-path-utils.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 07c10c8dca..0bba9887e9 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -190,6 +190,11 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' test "$sym" = "$(test-path-utils real_path "$dir2/syml")" ' +test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' ' + ln -s target symlink && + test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink" +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/b c/ relative_path /foo/a//b//c/ ///foo/a/b// c/ POSIX From e5aa1fc472696bfc337b3f128d45c1ef2aba394d Mon Sep 17 00:00:00 2001 From: Martin Erik Werner Date: Tue, 4 Feb 2014 15:25:17 +0100 Subject: [PATCH 3/7] t0060: add test for prefix_path when path == work tree The current behaviour of prefix_path is to return an empty string if prefixing and absolute path that only contains exactly the work tree. This behaviour is a potential regression point. Signed-off-by: Martin Erik Werner Reviewed-by: Duy Nguyen Signed-off-by: Junio C Hamano --- t/t0060-path-utils.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 0bba9887e9..b8e92e1a2a 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink" ' +test_expect_success 'prefix_path works with only absolute path to work tree' ' + echo "" >expected && + test-path-utils prefix_path prefix "$(pwd)" >actual && + test_cmp expected actual +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/b c/ relative_path /foo/a//b//c/ ///foo/a/b// c/ POSIX From e131daa4c6baa53aba22e5d8bc1defa83b443bf0 Mon Sep 17 00:00:00 2001 From: Martin Erik Werner Date: Tue, 4 Feb 2014 15:25:18 +0100 Subject: [PATCH 4/7] t0060: add tests for prefix_path when path begins with work tree One edge-case that isn't currently checked in the tests is the beginning of the path matching the work tree, despite the target not actually being the work tree, for example: path = /dir/repoa work_tree = /dir/repo should fail since the path is outside the repo. However, if /dir/repoa is in fact a symlink that points to /dir/repo, it should instead succeed. Add two tests covering these cases, since they might be potential regression points. Signed-off-by: Martin Erik Werner Reviewed-by: Duy Nguyen Signed-off-by: Junio C Hamano --- t/t0060-path-utils.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index b8e92e1a2a..f8286b1c3f 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' ' test_cmp expected actual ' +test_expect_success 'prefix_path rejects absolute path to dir with same beginning as work tree' ' + test_must_fail test-path-utils prefix_path prefix "$(pwd)a" +' + +test_expect_success SYMLINKS 'prefix_path works with absolute path to a symlink to work tree having same beginning as work tree' ' + git init repo && + ln -s repo repolink && + test "a" = "$(cd repo && test-path-utils prefix_path prefix "$(pwd)/../repolink/a")" +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/b c/ relative_path /foo/a//b//c/ ///foo/a/b// c/ POSIX From ddc2a6281595fd24ea01497c496f88c40a59562f Mon Sep 17 00:00:00 2001 From: Martin Erik Werner Date: Tue, 4 Feb 2014 15:25:19 +0100 Subject: [PATCH 5/7] setup: add abspath_part_inside_repo() function In order to extract the part of an absolute path which lies inside the repo, it is not possible to directly use real_path, since that would dereference symlinks both outside and inside the work tree. Add an abspath_part_inside_repo() function which first checks if the work tree is already the prefix, then incrementally checks each path level by temporarily NUL-terminating at each '/' and comparing against the work tree path. If a match is found, it overwrites the input path with the remainder past the work tree (which will be the part inside the work tree). This function is currently only intended for use in 'prefix_path_gently'. Signed-off-by: Martin Erik Werner Reviewed-by: Duy Nguyen Signed-off-by: Junio C Hamano --- setup.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/setup.c b/setup.c index 5432a31b62..bb8b53190e 100644 --- a/setup.c +++ b/setup.c @@ -5,6 +5,70 @@ static int inside_git_dir = -1; static int inside_work_tree = -1; +/* + * The input parameter must contain an absolute path, and it must already be + * normalized. + * + * Find the part of an absolute path that lies inside the work tree by + * dereferencing symlinks outside the work tree, for example: + * /dir1/repo/dir2/file (work tree is /dir1/repo) -> dir2/file + * /dir/file (work tree is /) -> dir/file + * /dir/symlink1/symlink2 (symlink1 points to work tree) -> symlink2 + * /dir/repolink/file (repolink points to /dir/repo) -> file + * /dir/repo (exactly equal to work tree) -> (empty string) + */ +static int abspath_part_inside_repo(char *path) +{ + size_t len; + size_t wtlen; + char *path0; + int off; + const char *work_tree = get_git_work_tree(); + + if (!work_tree) + return -1; + wtlen = strlen(work_tree); + len = strlen(path); + off = 0; + + /* check if work tree is already the prefix */ + if (wtlen <= len && !strncmp(path, work_tree, wtlen)) { + if (path[wtlen] == '/') { + memmove(path, path + wtlen + 1, len - wtlen); + return 0; + } else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') { + /* work tree is the root, or the whole path */ + memmove(path, path + wtlen, len - wtlen + 1); + return 0; + } + /* work tree might match beginning of a symlink to work tree */ + off = wtlen; + } + path0 = path; + path += offset_1st_component(path) + off; + + /* check each '/'-terminated level */ + while (*path) { + path++; + if (*path == '/') { + *path = '\0'; + if (strcmp(real_path(path0), work_tree) == 0) { + memmove(path0, path + 1, len - (path - path0)); + return 0; + } + *path = '/'; + } + } + + /* check whole path */ + if (strcmp(real_path(path0), work_tree) == 0) { + *path0 = '\0'; + return 0; + } + + return -1; +} + /* * Normalize "path", prepending the "prefix" for relative paths. If * remaining_prefix is not NULL, return the actual prefix still From 655ee9ea3e6c0af57d320e84723ec3bf656cdbf7 Mon Sep 17 00:00:00 2001 From: Martin Erik Werner Date: Tue, 4 Feb 2014 15:25:20 +0100 Subject: [PATCH 6/7] setup: don't dereference in-tree symlinks for absolute paths The prefix_path_gently() function currently applies real_path to everything if given an absolute path, dereferencing symlinks both outside and inside the work tree. This causes most high-level functions to misbehave when acting on symlinks given via absolute paths. For example $ git add /dir/repo/symlink attempts to add the target of the symlink rather than the symlink itself, which is usually not what the user intends to do. In order to manipulate symlinks in the work tree using absolute paths, symlinks should only be dereferenced outside the work tree. Modify the prefix_path_gently() to first normalize the path in order to make sure path levels are separated by '/', then pass the result to 'abspath_part_inside_repo' to find the part inside the work tree (without dereferencing any symlinks inside the work tree). For absolute paths, prefix_path_gently() did not, nor does now do, any actual prefixing, hence the result from abspath_part_in_repo() is returned as-is. Fixes t0060-82 and t3004-5. Signed-off-by: Martin Erik Werner Reviewed-by: Duy Nguyen Signed-off-by: Junio C Hamano --- setup.c | 30 ++++++++++-------------------- t/t0060-path-utils.sh | 2 +- t/t3004-ls-files-basic.sh | 2 +- 3 files changed, 12 insertions(+), 22 deletions(-) diff --git a/setup.c b/setup.c index bb8b53190e..8295ba6bd8 100644 --- a/setup.c +++ b/setup.c @@ -86,11 +86,17 @@ char *prefix_path_gently(const char *prefix, int len, const char *orig = path; char *sanitized; if (is_absolute_path(orig)) { - const char *temp = real_path(path); - sanitized = xmalloc(len + strlen(temp) + 1); - strcpy(sanitized, temp); + sanitized = xmalloc(strlen(path) + 1); if (remaining_prefix) *remaining_prefix = 0; + if (normalize_path_copy_len(sanitized, path, remaining_prefix)) { + free(sanitized); + return NULL; + } + if (abspath_part_inside_repo(sanitized)) { + free(sanitized); + return NULL; + } } else { sanitized = xmalloc(len + strlen(path) + 1); if (len) @@ -98,26 +104,10 @@ char *prefix_path_gently(const char *prefix, int len, strcpy(sanitized + len, path); if (remaining_prefix) *remaining_prefix = len; - } - if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) - goto error_out; - if (is_absolute_path(orig)) { - size_t root_len, len, total; - const char *work_tree = get_git_work_tree(); - if (!work_tree) - goto error_out; - len = strlen(work_tree); - root_len = offset_1st_component(work_tree); - total = strlen(sanitized) + 1; - if (strncmp(sanitized, work_tree, len) || - (len > root_len && sanitized[len] != '\0' && sanitized[len] != '/')) { - error_out: + if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) { free(sanitized); return NULL; } - if (sanitized[len] == '/') - len++; - memmove(sanitized, sanitized + len, total - len); } return sanitized; } diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index f8286b1c3f..c0143a0a70 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' test "$sym" = "$(test-path-utils real_path "$dir2/syml")" ' -test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' ' +test_expect_success SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' ' ln -s target symlink && test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink" ' diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh index e20c077e0f..9c7adbdbe1 100755 --- a/t/t3004-ls-files-basic.sh +++ b/t/t3004-ls-files-basic.sh @@ -36,7 +36,7 @@ test_expect_success 'ls-files -h in corrupt repository' ' test_i18ngrep "[Uu]sage: git ls-files " broken/usage ' -test_expect_failure SYMLINKS 'ls-files with absolute paths to symlinks' ' +test_expect_success SYMLINKS 'ls-files with absolute paths to symlinks' ' mkdir subs && ln -s nosuch link && ln -s ../nosuch subs/link && From 6127ff63cf39471b2c7317c9861016424d3b884b Mon Sep 17 00:00:00 2001 From: Martin Erik Werner Date: Thu, 24 Apr 2014 15:06:09 +0200 Subject: [PATCH 7/7] setup: fix windows path buffer over-stepping Fix a buffer over-stepping issue triggered by providing an absolute path that is similar to the work tree path. abspath_part_inside_repo() may currently increment the path pointer by offset_1st_component() + wtlen, which is too much, since offset_1st_component() is a subset of wtlen. For the *nix-style prefix '/', this does (by luck) not cause any issues, since offset_1st_component() is 1 and there will always be a '/' or '\0' that can "absorb" this. In the case of DOS-style prefixes though, the offset_1st_component() is 3 and this can potentially over-step the string buffer. For example if work_tree = "c:/r" path = "c:/rl" Then wtlen is 4, and incrementing the path pointer by (3 + 4) would end up 2 bytes outside a string buffer of length 6. Similarly if work_tree = "c:/r" path = "c:/rl/d/a" Then (since the loop starts by also incrementing the pointer one step), this would mean that the function would miss checking if "c:/rl/d" could be the work_tree, arguably this is unlikely though, since it would only be possible with symlinks on windows. Fix this by simply avoiding to increment by offset_1st_component() and wtlen at the same time. Signed-off-by: Martin Erik Werner Signed-off-by: Junio C Hamano --- setup.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/setup.c b/setup.c index 8295ba6bd8..3857b58562 100644 --- a/setup.c +++ b/setup.c @@ -29,7 +29,7 @@ static int abspath_part_inside_repo(char *path) return -1; wtlen = strlen(work_tree); len = strlen(path); - off = 0; + off = offset_1st_component(path); /* check if work tree is already the prefix */ if (wtlen <= len && !strncmp(path, work_tree, wtlen)) { @@ -45,7 +45,7 @@ static int abspath_part_inside_repo(char *path) off = wtlen; } path0 = path; - path += offset_1st_component(path) + off; + path += off; /* check each '/'-terminated level */ while (*path) {