# [PATCH] fix misuse of prefix_path() 6 messages from 2008-02-04 to 2008-02-05. Participants: Junio C Hamano, Johannes Sixt, Johannes Schindelin. Thread: https://gitlist.dev/t/11863 ## Junio C Hamano, 2008-02-04 07:03 Subject: [PATCH] fix misuse of prefix_path() Message-ID: <7v3as9mce7.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v3as9mce7.fsf%40gitster.siamese.dyndns.org ``` When DEFAULT_GIT_TEMPLATE_DIR is specified as a relative path, init-db made it relative to exec_path using prefix_path(), which is wrong. prefix_path() is about a file inside the work tree. There was a similar misuse in config.c that takes relative ETC_GITCONFIG path. A convenience function prefix_filename() can concatenate two paths to form a path that points at somewhere outside the work tree. Use it in these codepaths instead. Signed-off-by: Junio C Hamano --- * This is probably an oversight in commits leading to 7f0e39fa and I think if this patch breaks, MinGW port would be affected, hence I CC'ed j6t. There are similar misuse in builtin-mv.c that needs to be fixed. But git-mv is more broken, independent from this prefix_path() issue. For example, it does not check if src and dst are inside work tree, so (do not try this in a repository you care about) "git mv compat /tmp/outer-space" would throw tracked files in a work tree to outer space and then fail without touching the index. It needs a serious overhaul, but because I do not use it myself, the level of motivation to fix it myself is very low. A low hanging fruit, that is, for any git hacker wannabes... builtin-init-db.c | 3 +-- config.c | 5 +++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin-init-db.c b/builtin-init-db.c index e1393b8..e51d447 100644 --- a/builtin-init-db.c +++ b/builtin-init-db.c @@ -142,8 +142,7 @@ static void copy_templates(const char *git_dir, int len, const char *template_di template_dir = DEFAULT_GIT_TEMPLATE_DIR; if (!is_absolute_path(template_dir)) { const char *exec_path = git_exec_path(); - template_dir = prefix_path(exec_path, strlen(exec_path), - template_dir); + template_dir = prefix_filename(exec_path, strlen(exec_path), template_dir); } } strcpy(template_path, template_dir); diff --git a/config.c b/config.c index 526a3f4..0b0c9bd 100644 --- a/config.c +++ b/config.c @@ -485,8 +485,9 @@ const char *git_etc_gitconfig(void) if (!is_absolute_path(system_wide)) { /* interpret path relative to exec-dir */ const char *exec_path = git_exec_path(); - system_wide = prefix_path(exec_path, strlen(exec_path), - system_wide); + system_wide = strdup(prefix_filename(exec_path, + strlen(exec_path), + system_wide)); } } return system_wide; -- 1.5.4.18.gd0b8 ``` ## Junio C Hamano, 2008-02-04 08:09 Subject: [PATCH] builtin-mv: minimum fix to avoid losing files Message-ID: <7vhcgpkuqz.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhcgpkuqz.fsf%40gitster.siamese.dyndns.org In-Reply-To: <7v3as9mce7.fsf@gitster.siamese.dyndns.org> ``` 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 --- * This minimally fixes the issue and applies on top of the "setup: sanitize absolute and funny paths in get_pathspec()" patch I showed during the rc freeze. Dscho CC'ed as he owns the largest number of lines in this source file. builtin-mv.c | 6 +++++- t/t7001-mv.sh | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletions(-) diff --git a/builtin-mv.c b/builtin-mv.c index 94f6dd2..68aa2a6 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 b1243b4..fa382c5 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 -- 1.5.4.18.gd0b8 ``` ## Johannes Sixt, 2008-02-04 11:13 Subject: Re: [PATCH] fix misuse of prefix_path() Message-ID: <1202123606.47a6f3567ebb9@webmail.eunet.at> URL: https://gitlist.dev/e/1202123606.47a6f3567ebb9%40webmail.eunet.at In-Reply-To: <7v3as9mce7.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > When DEFAULT_GIT_TEMPLATE_DIR is specified as a relative path, > init-db made it relative to exec_path using prefix_path(), which > is wrong. prefix_path() is about a file inside the work tree. > There was a similar misuse in config.c that takes relative > ETC_GITCONFIG path. > > A convenience function prefix_filename() can concatenate two paths > to form a path that points at somewhere outside the work tree. > Use it in these codepaths instead. Thanks for catching that. But, no, your patch does not work. The reason is that prefix_filename() assumes that the prefix ends with a directory separator ('/'), but git_exec_path() doesn't have one. prefix_path() has the same assumption, but at the same time it does the "../" sanitization. In the MinGW configuration both relative paths begin with "../" and prefix_path() removed one path component from the prefix, but left a trailing '/' behind. IOW, so far the code worked by accident, not by design ;) I'll work on a fix (later today). -- Hannes ``` ## Johannes Schindelin, 2008-02-04 18:08 Subject: Re: [PATCH] builtin-mv: minimum fix to avoid losing files Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802041807230.7372%40racer.site In-Reply-To: <7vhcgpkuqz.fsf@gitster.siamese.dyndns.org> ``` Hi, On Mon, 4 Feb 2008, Junio C Hamano wrote: > * This minimally fixes the issue and applies on top of the > "setup: sanitize absolute and funny paths in get_pathspec()" > patch I showed during the rc freeze. > > Dscho CC'ed as he owns the largest number of lines in this > source file. Well, "owns" is a bit too strong, wouldn't you say? "Is at fault for" maybe ;-) Your patch looks obviously correct; I do not have time to look into other breakages of git-mv right now. Sorry. Thanks, Dscho ``` ## Johannes Sixt, 2008-02-05 08:17 Subject: [PATCH] Fix misuse of prefix_path() Message-ID: <47A81B9D.3090209@viscovery.net> URL: https://gitlist.dev/e/47A81B9D.3090209%40viscovery.net In-Reply-To: <1202123606.47a6f3567ebb9@webmail.eunet.at> ``` From: Johannes Sixt When DEFAULT_GIT_TEMPLATE_DIR is specified as a relative path, init-db made it relative to exec_path using prefix_path(), which is wrong. prefix_path() is about a file inside the work tree. There was a similar misuse in config.c that takes relative ETC_GITCONFIG path. Noticed by Junio C Hamano. We concatenate the paths manually. (prefix_filename() won't do because it expects a prefix with a trailing '/'.) Signed-off-by: Johannes Sixt --- builtin-init-db.c | 6 +++--- config.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/builtin-init-db.c b/builtin-init-db.c index e1393b8..5d7cdda 100644 --- a/builtin-init-db.c +++ b/builtin-init-db.c @@ -141,9 +141,9 @@ static void copy_templates(const char *git_dir, int len, const char *template_di */ template_dir = DEFAULT_GIT_TEMPLATE_DIR; if (!is_absolute_path(template_dir)) { - const char *exec_path = git_exec_path(); - template_dir = prefix_path(exec_path, strlen(exec_path), - template_dir); + struct strbuf d = STRBUF_INIT; + strbuf_addf(&d, "%s/%s", git_exec_path(), template_dir); + template_dir = strbuf_detach(&d, NULL); } } strcpy(template_path, template_dir); diff --git a/config.c b/config.c index 526a3f4..498259e 100644 --- a/config.c +++ b/config.c @@ -484,9 +484,9 @@ const char *git_etc_gitconfig(void) system_wide = ETC_GITCONFIG; if (!is_absolute_path(system_wide)) { /* interpret path relative to exec-dir */ - const char *exec_path = git_exec_path(); - system_wide = prefix_path(exec_path, strlen(exec_path), - system_wide); + struct strbuf d = STRBUF_INIT; + strbuf_addf(&d, "%s/%s", git_exec_path(), system_wide); + system_wide = strbuf_detach(&d, NULL); } } return system_wide; -- 1.5.4.rc5.842.ge7ba5 ``` ## Junio C Hamano, 2008-02-05 09:45 Subject: Re: [PATCH] Fix misuse of prefix_path() Message-ID: <7v3as7d9eq.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v3as7d9eq.fsf%40gitster.siamese.dyndns.org In-Reply-To: <47A81B9D.3090209@viscovery.net> ``` Sorry, I pushed out the previous one by mistake. Will apply this as a fix-up. ```