{"thread":{"id":"22336","subject":"[PATCH] git-mv: Fix error with multiple sources.","startedAt":"2010-01-21T20:39:41Z","lastAt":"2010-01-22T18:34:02Z","messageCount":10,"participants":["David Rydh","Junio C Hamano","Matthieu Moy","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"132377","messageId":"718290.769818367-sendEmail@darysmbp","threadId":"22336","inReplyTo":null,"subject":"[PATCH] git-mv: Fix error with multiple sources.","fromName":"David Rydh","fromEmail":"dary@math.berkeley.edu","sentAt":"2010-01-21T20:39:41Z","receivedAt":"2010-01-21T20:39:41Z","isPatch":true,"sender":{"key":"dary@math.berkeley.edu","avatar":null},"body":"Commit b8f26269 (fix directory separator treatment on Windows,\n30-06-2009) introduced a bug on Mac OS X. The problem is that\nbasename() may return a pointer to internal static storage space that\nwill be overwritten by subsequent calls:\n\n> git mv dir/a.txt dir/b.txt other/\n\n  Checking rename of 'dir/a.txt' to 'other/b.txt'\n  Checking rename of 'dir/b.txt' to 'other/b.txt'\n  fatal: multiple sources for the same target,\n  source=dir/b.txt, destination=other/b.txt\n\nThis commit also fixed two potentially dangerous uses of\nprefix_filename() -- which returns static storage space -- in\nbuiltin-config.c and hash-object.c.\n\nget_pathspec(): If called with an empty pathspec, allocate a new\npathspec with a copy of prefix. This is consistent with the behavior\nwhen called with a non-empty pathspec and prevents potential errors.\n\nSigned-off-by: David Rydh <dary@math.berkeley.edu>\n---\nThis is my first patch submission to git. Perhaps this patch should\nbe split into two? The one-line change to builtin-mv.c suffices to\nfix the bug.\n\n builtin-config.c |    4 ++--\n builtin-mv.c     |    2 +-\n hash-object.c    |    2 +-\n setup.c          |    4 ++--\n 4 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-config.c b/builtin-config.c\nindex 2e3ef91..bf60f93 100644\n--- a/builtin-config.c\n+++ b/builtin-config.c\n@@ -356,9 +356,9 @@ int cmd_config(int argc, const char **argv, const char *unused_prefix)\n \t\tconfig_exclusive_filename = git_etc_gitconfig();\n \telse if (given_config_file) {\n \t\tif (!is_absolute_path(given_config_file) && prefix)\n-\t\t\tconfig_exclusive_filename = prefix_filename(prefix,\n+\t\t\tconfig_exclusive_filename = xstrdup(prefix_filename(prefix,\n \t\t\t\t\t\t\t\t    strlen(prefix),\n-\t\t\t\t\t\t\t\t    argv[2]);\n+\t\t\t\t\t\t\t\t\targv[2]));\n \t\telse\n \t\t\tconfig_exclusive_filename = given_config_file;\n \t}\ndiff --git a/builtin-mv.c b/builtin-mv.c\nindex 8247186..1c1f8be 100644\n--- a/builtin-mv.c\n+++ b/builtin-mv.c\n@@ -27,7 +27,7 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,\n \t\tif (length > 0 && is_dir_sep(result[i][length - 1]))\n \t\t\tresult[i] = xmemdupz(result[i], length - 1);\n \t\tif (base_name)\n-\t\t\tresult[i] = basename((char *)result[i]);\n+\t\t\tresult[i] = xstrdup(basename((char *)result[i]));\n \t}\n \treturn get_pathspec(prefix, result);\n }\ndiff --git a/hash-object.c b/hash-object.c\nindex 9455dd0..3c509aa 100644\n--- a/hash-object.c\n+++ b/hash-object.c\n@@ -91,7 +91,7 @@ int main(int argc, const char **argv)\n \t\tprefix = setup_git_directory();\n \t\tprefix_length = prefix ? strlen(prefix) : 0;\n \t\tif (vpath && prefix)\n-\t\t\tvpath = prefix_filename(prefix, prefix_length, vpath);\n+\t\t\tvpath = xstrdup(prefix_filename(prefix, prefix_length, vpath));\n \t}\n \n \tgit_config(git_default_config, NULL);\ndiff --git a/setup.c b/setup.c\nindex 710e2f3..80cf535 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -132,8 +132,8 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n \t\treturn NULL;\n \n \tif (!entry) {\n-\t\tstatic const char *spec[2];\n-\t\tspec[0] = prefix;\n+\t\tconst char **spec = xmalloc(sizeof(char *) * 2);\n+\t\tspec[0] = xstrdup(prefix);\n \t\tspec[1] = NULL;\n \t\treturn spec;\n \t}\n-- \n1.6.6.1\n"},{"id":"132381","messageId":"7v8wbqbs26.fsf@alter.siamese.dyndns.org","threadId":"22336","inReplyTo":"718290.769818367-sendEmail@darysmbp","subject":"Re: [PATCH] git-mv: Fix error with multiple sources.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-22T05:57:05Z","receivedAt":"2010-01-22T05:57:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"David Rydh\" <dary@math.berkeley.edu> writes:\n\n> Commit b8f26269 (fix directory separator treatment on Windows,\n> 30-06-2009) introduced a bug on Mac OS X. The problem is that\n> basename() may return a pointer to internal static storage space that\n> will be overwritten by subsequent calls:\n>\n>> git mv dir/a.txt dir/b.txt other/\n>\n>   Checking rename of 'dir/a.txt' to 'other/b.txt'\n>   Checking rename of 'dir/b.txt' to 'other/b.txt'\n\nGood spotting.  basename(3)/dirname(3) are tricky functions to use\ncorrectly.  In addition to the \"static storage\" implementation and its\nassociated problem you encountered, they can also be implemented to modify\nthe given string in place, and can even return a pointer _into_ the given\nstring, which means that if you do:\n\n\tduped = strdup(string);\n        duped = basename(duped);\n        ... use duped ...\n\tfree(duped);\n\nyou may be burned quite badly.  The other call site of basename(3) is in\ndiff.c and it looks safe.\n\n> diff --git a/setup.c b/setup.c\n> index 710e2f3..80cf535 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -132,8 +132,8 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n>  \t\treturn NULL;\n>  \n>  \tif (!entry) {\n> -\t\tstatic const char *spec[2];\n> -\t\tspec[0] = prefix;\n> +\t\tconst char **spec = xmalloc(sizeof(char *) * 2);\n> +\t\tspec[0] = xstrdup(prefix);\n>  \t\tspec[1] = NULL;\n>  \t\treturn spec;\n>  \t}\n\nI don't understand this change.  Because elements of returned pathspec\nfrom this function are often simply the pathspec argument itself (which in\nturn is often argv[] of the calling program), and other times allocated by\nthis function, the callers are never going to free() them.\n"},{"id":"132382","messageId":"7vhbqeacjm.fsf@alter.siamese.dyndns.org","threadId":"22336","inReplyTo":"718290.769818367-sendEmail@darysmbp","subject":"Re: [PATCH] git-mv: Fix error with multiple sources.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-22T06:17:33Z","receivedAt":"2010-01-22T06:17:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"David Rydh\" <dary@math.berkeley.edu> writes:\n\n> This commit also fixed two potentially dangerous uses of\n> prefix_filename() -- which returns static storage space -- in\n> builtin-config.c and hash-object.c.\n\nThis should probably be a separate patch.  builtin-hash-object.c also uses\nprefix_filename() first to obtain vpath without strdup() and then uses the\nfunction to create arg, which seems to be unsafe to me.  I've looked at\nall the hits from \n\n    $ git grep -n -e prefix_filename\\( -- '*.c'\n\nand other places seem to be Ok.\n"},{"id":"132383","messageId":"7vd412ac8c.fsf@alter.siamese.dyndns.org","threadId":"22336","inReplyTo":"718290.769818367-sendEmail@darysmbp","subject":"Re: [PATCH] git-mv: Fix error with multiple sources.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-22T06:24:19Z","receivedAt":"2010-01-22T06:24:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"David Rydh\" <dary@math.berkeley.edu> writes:\n\n> diff --git a/builtin-mv.c b/builtin-mv.c\n> index 8247186..1c1f8be 100644\n> --- a/builtin-mv.c\n> +++ b/builtin-mv.c\n> @@ -27,7 +27,7 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,\n>  \t\tif (length > 0 && is_dir_sep(result[i][length - 1]))\n>  \t\t\tresult[i] = xmemdupz(result[i], length - 1);\n>  \t\tif (base_name)\n> -\t\t\tresult[i] = basename((char *)result[i]);\n> +\t\t\tresult[i] = xstrdup(basename((char *)result[i]));\n>  \t}\n>  \treturn get_pathspec(prefix, result);\n>  }\n\nGiven that basename(3) is allowed to modify its parameter, I think the\nabove code is still not portable.  casting constness away and feeding\nresult[i], especially when we didn't obtain our own copy by calling\nxmemdupz(), is especially problematic.\n\nPerhaps something ugly like this?\n\n\tfor (i = 0; i < count; i++) {\n\t\tint length = strlen(result[i]);\n\t\tint to_copy = length;\n                while (to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))\n\t\t\tto_copy--;\n\t\tif (to_copy != length || basename) {\n                \tchar *it = xmemdupz(result[i], to_copy);\n                        result[i] = base_name ? strdup(basename(it)) : it;\n\t\t}\n\t}\n"},{"id":"132384","messageId":"vpq7hrapqng.fsf@bauges.imag.fr","threadId":"22336","inReplyTo":"718290.769818367-sendEmail@darysmbp","subject":"Re: [PATCH] git-mv: Fix error with multiple sources.","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2010-01-22T07:03:47Z","receivedAt":"2010-01-22T07:03:47Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"\"David Rydh\" <dary@math.berkeley.edu> writes:\n\n>  \t\tif (!is_absolute_path(given_config_file) && prefix)\n> -\t\t\tconfig_exclusive_filename = prefix_filename(prefix,\n> +\t\t\tconfig_exclusive_filename = xstrdup(prefix_filename(prefix,\n>  \t\t\t\t\t\t\t\t    strlen(prefix),\n> -\t\t\t\t\t\t\t\t    argv[2]);\n> +\t\t\t\t\t\t\t\t\targv[2]));\n\nBroken indentation on the last two lines.\n\nOther than that, you use strdup without free-ing the result. Probably\nnot so serious, but it may be cleaner to actually call free, or to\nexplain why you don't (in commit message).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"132385","messageId":"4B5953D6.7020303@viscovery.net","threadId":"22336","inReplyTo":"718290.769818367-sendEmail@darysmbp","subject":"Re: [PATCH] git-mv: Fix error with multiple sources.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-01-22T07:29:26Z","receivedAt":"2010-01-22T07:29:26Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"David Rydh schrieb:\n> diff --git a/builtin-mv.c b/builtin-mv.c\n> index 8247186..1c1f8be 100644\n> --- a/builtin-mv.c\n> +++ b/builtin-mv.c\n> @@ -27,7 +27,7 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec,\n>  \t\tif (length > 0 && is_dir_sep(result[i][length - 1]))\n>  \t\t\tresult[i] = xmemdupz(result[i], length - 1);\n>  \t\tif (base_name)\n> -\t\t\tresult[i] = basename((char *)result[i]);\n> +\t\t\tresult[i] = xstrdup(basename((char *)result[i]));\n>  \t}\n>  \treturn get_pathspec(prefix, result);\n>  }\n\nWe are already leaking memory of magnitude O(argc*length of file names),\nand IMO, this new leak of the same magnitude doesn't hurt.\n\nIf you want to avoid it, you can set NO_LIBGEN_H in Makefile.\n\nThe other changes in this patch should really be a separate patch. They do\nnot fix an immediate problem, right?\n\n-- Hannes\n"},{"id":"132407","messageId":"28E55A63-C40D-4C5D-906F-742AC6FC9A88@math.berkeley.edu","threadId":"22336","inReplyTo":"7v8wbqbs26.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-mv: Fix error with multiple sources.","fromName":"David Rydh","fromEmail":"dary@math.berkeley.edu","sentAt":"2010-01-22T16:41:59Z","receivedAt":"2010-01-22T16:41:59Z","isPatch":true,"sender":{"key":"dary@math.berkeley.edu","avatar":null},"body":"On Jan 21, 2010, at 9:57 PM, Junio C Hamano wrote:\n\n>> diff --git a/setup.c b/setup.c\n>> index 710e2f3..80cf535 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -132,8 +132,8 @@ const char **get_pathspec(const char *prefix, const char **pathspec)\n>> \t\treturn NULL;\n>> \n>> \tif (!entry) {\n>> -\t\tstatic const char *spec[2];\n>> -\t\tspec[0] = prefix;\n>> +\t\tconst char **spec = xmalloc(sizeof(char *) * 2);\n>> +\t\tspec[0] = xstrdup(prefix);\n>> \t\tspec[1] = NULL;\n>> \t\treturn spec;\n>> \t}\n> \n> I don't understand this change.  Because elements of returned pathspec\n> from this function are often simply the pathspec argument itself (which in\n> turn is often argv[] of the calling program), and other times allocated by\n> this function, the callers are never going to free() them.\n\nThe xstrdup of prefix is for good measure. The important change is the removal of the static spec-array. Two invocations of get_pathspec with different prefixes could invalidate the contents of a pathspec in use.\n\nWhen called with a non-empty pathspec, all the entries are allocated so I think it is reasonable to allocate them in the degenerate case as well. My impression of the git-code is that it is leaking quite a bit when it comes to strings anyway.\n"},{"id":"132408","messageId":"D7FDF1CC-CED4-4CE8-9FEC-6A630830504C@math.berkeley.edu","threadId":"22336","inReplyTo":"7vhbqeacjm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-mv: Fix error with multiple sources.","fromName":"David Rydh","fromEmail":"dary@math.berkeley.edu","sentAt":"2010-01-22T16:49:08Z","receivedAt":"2010-01-22T16:49:08Z","isPatch":true,"sender":{"key":"dary@math.berkeley.edu","avatar":null},"body":"\nOn Jan 21, 2010, at 10:17 PM, Junio C Hamano wrote:\n\n> \"David Rydh\" <dary@math.berkeley.edu> writes:\n> \n>> This commit also fixed two potentially dangerous uses of\n>> prefix_filename() -- which returns static storage space -- in\n>> builtin-config.c and hash-object.c.\n> \n> This should probably be a separate patch.  builtin-hash-object.c also uses\n> prefix_filename() first to obtain vpath without strdup() and then uses the\n> function to create arg, which seems to be unsafe to me.  I've looked at\n> all the hits from \n> \n>    $ git grep -n -e prefix_filename\\( -- '*.c'\n> \n> and other places seem to be Ok.\n\n\nYes, this was how I found these two places. Do you agree that strdup'ing vpath as I did in the patch fixes this flaw? (I assume that builtin-hash-object.c = hash-object.c) I'll split it up into two patches.\n"},{"id":"132409","messageId":"CB7104C1-EB96-426F-8280-E33105378B18@math.berkeley.edu","threadId":"22336","inReplyTo":"7vr5pi8x6z.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-mv: Fix error with multiple sources.","fromName":"David Rydh","fromEmail":"dary@math.berkeley.edu","sentAt":"2010-01-22T17:30:09Z","receivedAt":"2010-01-22T17:30:09Z","isPatch":true,"sender":{"key":"dary@math.berkeley.edu","avatar":null},"body":"On Jan 21, 2010, at 10:34 PM, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> Given that basename(3) is allowed to modify its parameter, I think the\n>> above code is still not portable.  casting constness away and feeding\n>> result[i], especially when we didn't obtain our own copy by calling\n>> xmemdupz(), is especially problematic.\n>> \n>> Perhaps something ugly like this?\n>> \n>> \tfor (i = 0; i < count; i++) {\n>> \t\tint length = strlen(result[i]);\n>> \t\tint to_copy = length;\n>>                while (to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))\n>> \t\t\tto_copy--;\n>> \t\tif (to_copy != length || basename) {\n>>                \tchar *it = xmemdupz(result[i], to_copy);\n>>                        result[i] = base_name ? strdup(basename(it)) : it;\n>> \t\t}\n>> \t}\n\nThis looks fine. Currently we have the odd behavior\n\n> git mv -n dir/ new-dir\nChecking rename of 'dir' to 'new-dir'\nChecking rename of 'dir/a.txt' to 'new-dir/a.txt'\nChecking rename of 'dir/b.txt' to 'new-dir/b.txt'\n\n> git mv -n dir// new-dir\nChecking rename of 'dir/' to 'new-dir'\nfatal: source directory is empty, source=dir/, destination=new-dir\n\nNote that at the end of copy_pathspec we call get_pathspec which squashes multiple slashes (even if prefix=NULL) but does not remove a trailing slash so it is necessary to squash them all as above.\n"},{"id":"132413","messageId":"201001221934.03346.j6t@kdbg.org","threadId":"22336","inReplyTo":"7vr5pi8x6z.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-mv: Fix error with multiple sources.","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-22T18:34:02Z","receivedAt":"2010-01-22T18:34:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Freitag, 22. Januar 2010, Junio C Hamano wrote:\n> Why does '/' matter in builtin-mv.c that needed b8f2626, but we can\n> compare with '/' [in make_relative_path()]?\n\nWe should be using is_dir_sep() in make_relative_path() as well (see the fixup \nthat I posted in response to your patch). The thing is that the incorrect \nresult does not matter from a correctness-POV for the only caller of this \nroutine.\n\n-- Hannes\n"}]}