Re: [PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:59:45
Martin Erik Werner [off-list ref] writes:
In order to manipulate symliks in the work tree using absolute paths, symlinks should only be dereferenced outside the work tree.
I agree 100% with this reasoning (modulo s/symliks/symlinks/). As to the implementation, it looks a bit overly complicated, though. I haven't tried writing the same myself, but * I suspect that strbuf would help simplifying the allocation and deallocation; * Also I suspect that use of string-list to split and then join is making the code unnecessarily complex. In Python/Perl, that would be a normal approach, but in C, it would be a lot simpler if you prepare a writable temporary in wtpart[], walk from left to right finding '/' and replacing it temporarily with NUL to terminate in order to check with real_path(), restore the NUL back to '/' to check deeper, and rinse and repeat. Having said that, I am not absolutely sure if the repeated calls to real_path() are doing the right thing, though ;-)
quoted hunk
diff --git a/setup.c b/setup.c index 5432a31..0789a96 100644 --- a/setup.c +++ b/setup.c@@ -22,11 +22,51 @@ 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); + int i, match; + size_t wtpartlen; + char *npath, *wtpart; + struct string_list list = STRING_LIST_INIT_DUP; + const char *work_tree = get_git_work_tree(); + if (!work_tree) + return NULL; + npath = xmalloc(strlen(path) + 1); if (remaining_prefix) *remaining_prefix = 0; + if (normalize_path_copy_len(npath, path, remaining_prefix)) { + free(npath); + return NULL; + } + + string_list_split(&list, npath, '/', -1); + wtpart = xmalloc(strlen(npath) + 1); + i = 0; + match = 0; + strcpy(wtpart, list.items[i++].string); + strcat(wtpart, "/"); + if (strcmp(real_path(wtpart), work_tree) == 0) { + match = 1; + } else { + while (i < list.nr) { + strcat(wtpart, list.items[i++].string); + if (strcmp(real_path(wtpart), work_tree) == 0) { + match = 1; + break; + } + strcat(wtpart, "/"); + } + } + string_list_clear(&list, 0); + if (!match) { + free(npath); + free(wtpart); + return NULL; + } + + wtpartlen = strlen(wtpart); + sanitized = xmalloc(strlen(npath) - wtpartlen); + strcpy(sanitized, npath + wtpartlen + 1); + free(npath); + free(wtpart); } else { sanitized = xmalloc(len + strlen(path) + 1); if (len)@@ -34,26 +74,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 3a0677a..03a12ac 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 work tree symlinks' ' +test_expect_success SYMLINKS 'prefix_path works with work tree symlinks' ' ln -s target symlink && test "$(test-path-utils prefix_path prefix "$(pwd)/symlink")" = "symlink"