{"thread":{"id":"31430","subject":"[PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path()","startedAt":"2012-09-04T17:29:22Z","lastAt":"2012-09-07T19:19:16Z","messageCount":3,"participants":["Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"198327","messageId":"50463A72.2000209@ramsay1.demon.co.uk","threadId":"31430","inReplyTo":null,"subject":"[PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-09-04T17:29:22Z","receivedAt":"2012-09-04T17:29:22Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nThe current implementation of git_path() is essentially the same as\nthat of vsnpath(), with two minor differences. First, git_path()\ncurrently insists that the git directory path is no longer than\nPATH_MAX-100 characters in length. However, vsnpath() does not\nattempt this arbitrary 100 character reservation for the remaining\npath components. Second, vsnpath() uses the \"is_dir_sep()\" macro,\nrather than comparing directly to '/', to determine if the git_dir\npath component ends with a path separator.\n\nIn order to benefit from the above improvements, along with increased\ncompatability with git_snpath() and git_pathdup(), we reimplement the\ngit_path() function using vsnpath().\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n path.c | 15 +++------------\n 1 file changed, 3 insertions(+), 12 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 741ae77..cbbdf7d 100644\n--- a/path.c\n+++ b/path.c\n@@ -119,23 +119,14 @@ char *mkpath(const char *fmt, ...)\n \n char *git_path(const char *fmt, ...)\n {\n-\tconst char *git_dir = get_git_dir();\n \tchar *pathname = get_pathname();\n \tva_list args;\n-\tunsigned len;\n+\tchar *ret;\n \n-\tlen = strlen(git_dir);\n-\tif (len > PATH_MAX-100)\n-\t\treturn bad_path;\n-\tmemcpy(pathname, git_dir, len);\n-\tif (len && git_dir[len-1] != '/')\n-\t\tpathname[len++] = '/';\n \tva_start(args, fmt);\n-\tlen += vsnprintf(pathname + len, PATH_MAX - len, fmt, args);\n+\tret = vsnpath(pathname, PATH_MAX, fmt, args);\n \tva_end(args);\n-\tif (len >= PATH_MAX)\n-\t\treturn bad_path;\n-\treturn cleanup_path(pathname);\n+\treturn ret;\n }\n \n void home_config_paths(char **global, char **xdg, char *file)\n-- \n1.7.12\n"},{"id":"198341","messageId":"7vsjax8rap.fsf@alter.siamese.dyndns.org","threadId":"31430","inReplyTo":"50463A72.2000209@ramsay1.demon.co.uk","subject":"Re: [PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-09-04T20:30:06Z","receivedAt":"2012-09-04T20:30:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> The current implementation of git_path() is essentially the same as\n> that of vsnpath(), with two minor differences. First, git_path()\n> currently insists that the git directory path is no longer than\n> PATH_MAX-100 characters in length. However, vsnpath() does not\n> attempt this arbitrary 100 character reservation for the remaining\n> path components. Second, vsnpath() uses the \"is_dir_sep()\" macro,\n> rather than comparing directly to '/', to determine if the git_dir\n> path component ends with a path separator.\n> In order to benefit from the above improvements,...\n\nIn the longer term, I think this goes in the right direction, but\nthe loss of reservation, especially when we know git_path() is used\nby some callers to get the base directory in $GIT_DIR that want to\nappend stuff after the returned directory path to form the final\npathname, is a bit worrysome.  It may be hiding a bug (lack of\nproper limit check) on the callers' side.\n\n> ... along with increased\n> compatability with git_snpath() and git_pathdup(), we reimplement the\n> git_path() function using vsnpath().\n>\n> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n> ---\n>  path.c | 15 +++------------\n>  1 file changed, 3 insertions(+), 12 deletions(-)\n>\n> diff --git a/path.c b/path.c\n> index 741ae77..cbbdf7d 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -119,23 +119,14 @@ char *mkpath(const char *fmt, ...)\n>  \n>  char *git_path(const char *fmt, ...)\n>  {\n> -\tconst char *git_dir = get_git_dir();\n>  \tchar *pathname = get_pathname();\n>  \tva_list args;\n> -\tunsigned len;\n> +\tchar *ret;\n>  \n> -\tlen = strlen(git_dir);\n> -\tif (len > PATH_MAX-100)\n> -\t\treturn bad_path;\n> -\tmemcpy(pathname, git_dir, len);\n> -\tif (len && git_dir[len-1] != '/')\n> -\t\tpathname[len++] = '/';\n>  \tva_start(args, fmt);\n> -\tlen += vsnprintf(pathname + len, PATH_MAX - len, fmt, args);\n> +\tret = vsnpath(pathname, PATH_MAX, fmt, args);\n>  \tva_end(args);\n> -\tif (len >= PATH_MAX)\n> -\t\treturn bad_path;\n> -\treturn cleanup_path(pathname);\n> +\treturn ret;\n>  }\n>  \n>  void home_config_paths(char **global, char **xdg, char *file)\n"},{"id":"198549","messageId":"504A48B4.9010109@ramsay1.demon.co.uk","threadId":"31430","inReplyTo":"7vsjax8rap.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path()","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-09-07T19:19:16Z","receivedAt":"2012-09-07T19:19:16Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n> \n>> The current implementation of git_path() is essentially the same as\n>> that of vsnpath(), with two minor differences. First, git_path()\n>> currently insists that the git directory path is no longer than\n>> PATH_MAX-100 characters in length. However, vsnpath() does not\n>> attempt this arbitrary 100 character reservation for the remaining\n>> path components. Second, vsnpath() uses the \"is_dir_sep()\" macro,\n>> rather than comparing directly to '/', to determine if the git_dir\n>> path component ends with a path separator.\n>> In order to benefit from the above improvements,...\n> \n> In the longer term, I think this goes in the right direction, but\n> the loss of reservation, especially when we know git_path() is used\n> by some callers to get the base directory in $GIT_DIR that want to\n> append stuff after the returned directory path to form the final\n> pathname, is a bit worrysome.  It may be hiding a bug (lack of\n> proper limit check) on the callers' side.\n\nHmm, at first I could not see what you found worrysome here.\nAfter all, the number of inputs which leads to success (i.e. does\nnot result in an \"/bad-path/\" return) has been *increased* with\nthis patch.\n\nHowever, I suppose you are concerned about something like this:\n\n    char *git_dir = git_path(\"\");\n    if (strcmp(git_dir, \"/bad-path/\") != 0) {\n        /*\n         * Having studied the implementation of git_path(), I know\n         * that the buffer pointed to by git_dir has space for an\n         * additional 100 chars. This is enough room to concatenate\n         * the doberry path, so this is safe ...\n         */\n        strcat(git_dir, doberry); /* oops */\n    }\n\nYes?\n\nHmm, yes it would be a little disapointing to see such parasitic code!\n;-)\n\nYou said above: \"... especially when we know git_path() is used\nby some callers to get the base directory in $GIT_DIR ...\". Can you\npoint me to an example of such a caller; I have been unable to find\nany code which does this.\n\nATB,\nRamsay Jones\n"}]}