{"thread":{"id":"11863","subject":"[PATCH] fix misuse of prefix_path()","startedAt":"2008-02-04T07:03:28Z","lastAt":"2008-02-05T09:45:01Z","messageCount":6,"participants":["Junio C Hamano","Johannes Sixt","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"67322","messageId":"7v3as9mce7.fsf@gitster.siamese.dyndns.org","threadId":"11863","inReplyTo":null,"subject":"[PATCH] fix misuse of prefix_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-04T07:03:28Z","receivedAt":"2008-02-04T07:03:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When DEFAULT_GIT_TEMPLATE_DIR is specified as a relative path,\ninit-db made it relative to exec_path using prefix_path(), which\nis wrong. prefix_path() is about a file inside the work tree.\nThere was a similar misuse in config.c that takes relative\nETC_GITCONFIG path.\n\nA convenience function prefix_filename() can concatenate two paths\nto form a path that points at somewhere outside the work tree.\nUse it in these codepaths instead.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This is probably an oversight in commits leading to\n 7f0e39fa and I think if this patch breaks, MinGW port would\n be affected, hence I CC'ed j6t.\n\n There are similar misuse in builtin-mv.c that needs to be\n fixed. But git-mv is more broken, independent from this\n prefix_path() issue. For example, it does not check if src\n and dst are inside work tree, so (do not try this in a\n repository you care about) \"git mv compat /tmp/outer-space\"\n would throw tracked files in a work tree to outer space and\n then fail without touching the index. It needs a serious\n overhaul, but because I do not use it myself, the level of\n motivation to fix it myself is very low. A low hanging\n fruit, that is, for any git hacker wannabes...\n\n builtin-init-db.c | 3 +--\n config.c | 5 +++--\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex e1393b8..e51d447 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -142,8 +142,7 @@ static void copy_templates(const char *git_dir, int len, const char *template_di\n \t\ttemplate_dir = DEFAULT_GIT_TEMPLATE_DIR;\n \t\tif (!is_absolute_path(template_dir)) {\n \t\t\tconst char *exec_path = git_exec_path();\n-\t\t\ttemplate_dir = prefix_path(exec_path, strlen(exec_path),\n-\t\t\t\t\t\t template_dir);\n+\t\t\ttemplate_dir = prefix_filename(exec_path, strlen(exec_path), template_dir);\n \t\t}\n \t}\n \tstrcpy(template_path, template_dir);\ndiff --git a/config.c b/config.c\nindex 526a3f4..0b0c9bd 100644\n--- a/config.c\n+++ b/config.c\n@@ -485,8 +485,9 @@ const char *git_etc_gitconfig(void)\n \t\tif (!is_absolute_path(system_wide)) {\n \t\t\t/* interpret path relative to exec-dir */\n \t\t\tconst char *exec_path = git_exec_path();\n-\t\t\tsystem_wide = prefix_path(exec_path, strlen(exec_path),\n-\t\t\t\t\t\tsystem_wide);\n+\t\t\tsystem_wide = strdup(prefix_filename(exec_path,\n+\t\t\t\t\t\t\t strlen(exec_path),\n+\t\t\t\t\t\t\t system_wide));\n \t\t}\n \t}\n \treturn system_wide;\n-- \n1.5.4.18.gd0b8\n"},{"id":"67333","messageId":"7vhcgpkuqz.fsf@gitster.siamese.dyndns.org","threadId":"11863","inReplyTo":"7v3as9mce7.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] builtin-mv: minimum fix to avoid losing files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-04T08:09:56Z","receivedAt":"2008-02-04T08:09:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"An incorrect command \"git mv subdir /outer/space\" threw the\nsubdirectory to outside of the repository and then noticed that\n/outer/space/subdir/ would be outside of the repository. The\nerror checking is backwards.\n\nThis fixes the issue by being careful about use of the return\nvalue of get_pathspec(). Since the implementation already has\nhandcrafted loop to munge each path on the command line, we use\nprefix_path() instead.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This minimally fixes the issue and applies on top of the\n \"setup: sanitize absolute and funny paths in get_pathspec()\"\n patch I showed during the rc freeze.\n\n Dscho CC'ed as he owns the largest number of lines in this\n source file.\n\n builtin-mv.c | 6 +++++-\n t/t7001-mv.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex 94f6dd2..68aa2a6 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -19,6 +19,7 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,\n \t\t\t\t int count, int base_name)\n {\n \tint i;\n+\tint len = prefix ? strlen(prefix) : 0;\n \tconst char **result = xmalloc((count + 1) * sizeof(const char *));\n \tmemcpy(result, pathspec, count * sizeof(const char *));\n \tresult[count] = NULL;\n@@ -32,8 +33,11 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,\n \t\t\tif (last_slash)\n \t\t\t\tresult[i] = last_slash + 1;\n \t\t}\n+\t\tresult[i] = prefix_path(prefix, len, result[i]);\n+\t\tif (!result[i])\n+\t\t\texit(1); /* error already given */\n \t}\n-\treturn get_pathspec(prefix, result);\n+\treturn result;\n }\n \n static void show_list(const char *label, struct path_list *list)\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex b1243b4..fa382c5 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -118,4 +118,42 @@ test_expect_success \"Sergey Vlasov's test case\" '\n \tgit mv ab a\n '\n \n+test_expect_success 'absolute pathname' '(\n+\n+\trm -fr mine &&\n+\tmkdir mine &&\n+\tcd mine &&\n+\ttest_create_repo one &&\n+\tcd one &&\n+\tmkdir sub &&\n+\t>sub/file &&\n+\tgit add sub/file &&\n+\n+\tgit mv sub \"$(pwd)/in\" &&\n+\t! test -d sub &&\n+\ttest -d in &&\n+\tgit ls-files --error-unmatch in/file\n+\n+\n+)'\n+\n+test_expect_success 'absolute pathname outside should fail' '(\n+\n+\trm -fr mine &&\n+\tmkdir mine &&\n+\tcd mine &&\n+\tout=$(pwd) &&\n+\ttest_create_repo one &&\n+\tcd one &&\n+\tmkdir sub &&\n+\t>sub/file &&\n+\tgit add sub/file &&\n+\n+\t! git mv sub \"$out/out\" &&\n+\ttest -d sub &&\n+\t! test -d ../in &&\n+\tgit ls-files --error-unmatch sub/file\n+\n+)'\n+\n test_done\n-- \n1.5.4.18.gd0b8\n"},{"id":"67347","messageId":"1202123606.47a6f3567ebb9@webmail.eunet.at","threadId":"11863","inReplyTo":"7v3as9mce7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] fix misuse of prefix_path()","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2008-02-04T11:13:26Z","receivedAt":"2008-02-04T11:13:26Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano wrote:\n> When DEFAULT_GIT_TEMPLATE_DIR is specified as a relative path,\n> init-db made it relative to exec_path using prefix_path(), which\n> is wrong. prefix_path() is about a file inside the work tree.\n> There was a similar misuse in config.c that takes relative\n> ETC_GITCONFIG path.\n>\n> A convenience function prefix_filename() can concatenate two paths\n> to form a path that points at somewhere outside the work tree.\n> Use it in these codepaths instead.\n\nThanks for catching that. But, no, your patch does not work. The reason is\nthat prefix_filename() assumes that the prefix ends with a directory\nseparator ('/'), but git_exec_path() doesn't have one.\n\nprefix_path() has the same assumption, but at the same time it does the\n\"../\" sanitization. In the MinGW configuration both relative paths begin\nwith \"../\" and prefix_path() removed one path component from the prefix,\nbut left a trailing '/' behind. IOW, so far the code worked by accident,\nnot by design ;)\n\nI'll work on a fix (later today).\n\n-- Hannes\n"},{"id":"67394","messageId":"alpine.LSU.1.00.0802041807230.7372@racer.site","threadId":"11863","inReplyTo":"7vhcgpkuqz.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-mv: minimum fix to avoid losing files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-04T18:08:25Z","receivedAt":"2008-02-04T18:08:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 4 Feb 2008, Junio C Hamano wrote:\n\n> * This minimally fixes the issue and applies on top of the\n> \"setup: sanitize absolute and funny paths in get_pathspec()\"\n> patch I showed during the rc freeze.\n> \n> Dscho CC'ed as he owns the largest number of lines in this\n> source file.\n\nWell, \"owns\" is a bit too strong, wouldn't you say? \"Is at fault for\" \nmaybe ;-)\n\nYour patch looks obviously correct; I do not have time to look into other \nbreakages of git-mv right now. Sorry.\n\nThanks,\nDscho\n"},{"id":"67489","messageId":"47A81B9D.3090209@viscovery.net","threadId":"11863","inReplyTo":"1202123606.47a6f3567ebb9@webmail.eunet.at","subject":"[PATCH] Fix misuse of prefix_path()","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-02-05T08:17:33Z","receivedAt":"2008-02-05T08:17:33Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt \n\nWhen DEFAULT_GIT_TEMPLATE_DIR is specified as a relative path,\ninit-db made it relative to exec_path using prefix_path(), which\nis wrong. prefix_path() is about a file inside the work tree.\nThere was a similar misuse in config.c that takes relative\nETC_GITCONFIG path. Noticed by Junio C Hamano.\n\nWe concatenate the paths manually. (prefix_filename() won't do\nbecause it expects a prefix with a trailing '/'.)\n\nSigned-off-by: Johannes Sixt \n---\n builtin-init-db.c | 6 +++---\n config.c | 6 +++---\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex e1393b8..5d7cdda 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -141,9 +141,9 @@ static void copy_templates(const char *git_dir, int len, const char *template_di\n \t\t */\n \t\ttemplate_dir = DEFAULT_GIT_TEMPLATE_DIR;\n \t\tif (!is_absolute_path(template_dir)) {\n-\t\t\tconst char *exec_path = git_exec_path();\n-\t\t\ttemplate_dir = prefix_path(exec_path, strlen(exec_path),\n-\t\t\t\t\t\t template_dir);\n+\t\t\tstruct strbuf d = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&d, \"%s/%s\", git_exec_path(), template_dir);\n+\t\t\ttemplate_dir = strbuf_detach(&d, NULL);\n \t\t}\n \t}\n \tstrcpy(template_path, template_dir);\ndiff --git a/config.c b/config.c\nindex 526a3f4..498259e 100644\n--- a/config.c\n+++ b/config.c\n@@ -484,9 +484,9 @@ const char *git_etc_gitconfig(void)\n \t\tsystem_wide = ETC_GITCONFIG;\n \t\tif (!is_absolute_path(system_wide)) {\n \t\t\t/* interpret path relative to exec-dir */\n-\t\t\tconst char *exec_path = git_exec_path();\n-\t\t\tsystem_wide = prefix_path(exec_path, strlen(exec_path),\n-\t\t\t\t\t\tsystem_wide);\n+\t\t\tstruct strbuf d = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&d, \"%s/%s\", git_exec_path(), system_wide);\n+\t\t\tsystem_wide = strbuf_detach(&d, NULL);\n \t\t}\n \t}\n \treturn system_wide;\n-- \n1.5.4.rc5.842.ge7ba5\n"},{"id":"67495","messageId":"7v3as7d9eq.fsf@gitster.siamese.dyndns.org","threadId":"11863","inReplyTo":"47A81B9D.3090209@viscovery.net","subject":"Re: [PATCH] Fix misuse of prefix_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-05T09:45:01Z","receivedAt":"2008-02-05T09:45:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sorry, I pushed out the previous one by mistake. Will apply\nthis as a fix-up.\n"}]}