# [PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path() 3 messages from 2012-09-04 to 2012-09-07. Participants: Ramsay Jones, Junio C Hamano. Thread: https://gitlist.dev/t/31430 ## Ramsay Jones, 2012-09-04 17:29 Subject: [PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path() Message-ID: <50463A72.2000209@ramsay1.demon.co.uk> URL: https://gitlist.dev/e/50463A72.2000209%40ramsay1.demon.co.uk ``` The current implementation of git_path() is essentially the same as that of vsnpath(), with two minor differences. First, git_path() currently insists that the git directory path is no longer than PATH_MAX-100 characters in length. However, vsnpath() does not attempt this arbitrary 100 character reservation for the remaining path components. Second, vsnpath() uses the "is_dir_sep()" macro, rather than comparing directly to '/', to determine if the git_dir path component ends with a path separator. In order to benefit from the above improvements, along with increased compatability with git_snpath() and git_pathdup(), we reimplement the git_path() function using vsnpath(). Signed-off-by: Ramsay Jones --- path.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/path.c b/path.c index 741ae77..cbbdf7d 100644 --- a/path.c +++ b/path.c @@ -119,23 +119,14 @@ char *mkpath(const char *fmt, ...) char *git_path(const char *fmt, ...) { - const char *git_dir = get_git_dir(); char *pathname = get_pathname(); va_list args; - unsigned len; + char *ret; - len = strlen(git_dir); - if (len > PATH_MAX-100) - return bad_path; - memcpy(pathname, git_dir, len); - if (len && git_dir[len-1] != '/') - pathname[len++] = '/'; va_start(args, fmt); - len += vsnprintf(pathname + len, PATH_MAX - len, fmt, args); + ret = vsnpath(pathname, PATH_MAX, fmt, args); va_end(args); - if (len >= PATH_MAX) - return bad_path; - return cleanup_path(pathname); + return ret; } void home_config_paths(char **global, char **xdg, char *file) -- 1.7.12 ``` ## Junio C Hamano, 2012-09-04 20:30 Subject: Re: [PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path() Message-ID: <7vsjax8rap.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vsjax8rap.fsf%40alter.siamese.dyndns.org In-Reply-To: <50463A72.2000209@ramsay1.demon.co.uk> ``` Ramsay Jones writes: > The current implementation of git_path() is essentially the same as > that of vsnpath(), with two minor differences. First, git_path() > currently insists that the git directory path is no longer than > PATH_MAX-100 characters in length. However, vsnpath() does not > attempt this arbitrary 100 character reservation for the remaining > path components. Second, vsnpath() uses the "is_dir_sep()" macro, > rather than comparing directly to '/', to determine if the git_dir > path component ends with a path separator. > In order to benefit from the above improvements,... In the longer term, I think this goes in the right direction, but the loss of reservation, especially when we know git_path() is used by some callers to get the base directory in $GIT_DIR that want to append stuff after the returned directory path to form the final pathname, is a bit worrysome. It may be hiding a bug (lack of proper limit check) on the callers' side. > ... along with increased > compatability with git_snpath() and git_pathdup(), we reimplement the > git_path() function using vsnpath(). > > Signed-off-by: Ramsay Jones > --- > path.c | 15 +++------------ > 1 file changed, 3 insertions(+), 12 deletions(-) > > diff --git a/path.c b/path.c > index 741ae77..cbbdf7d 100644 > --- a/path.c > +++ b/path.c > @@ -119,23 +119,14 @@ char *mkpath(const char *fmt, ...) > > char *git_path(const char *fmt, ...) > { > - const char *git_dir = get_git_dir(); > char *pathname = get_pathname(); > va_list args; > - unsigned len; > + char *ret; > > - len = strlen(git_dir); > - if (len > PATH_MAX-100) > - return bad_path; > - memcpy(pathname, git_dir, len); > - if (len && git_dir[len-1] != '/') > - pathname[len++] = '/'; > va_start(args, fmt); > - len += vsnprintf(pathname + len, PATH_MAX - len, fmt, args); > + ret = vsnpath(pathname, PATH_MAX, fmt, args); > va_end(args); > - if (len >= PATH_MAX) > - return bad_path; > - return cleanup_path(pathname); > + return ret; > } > > void home_config_paths(char **global, char **xdg, char *file) ``` ## Ramsay Jones, 2012-09-07 19:19 Subject: Re: [PATCH 3/5] path.c: Use vsnpath() in the implementation of git_path() Message-ID: <504A48B4.9010109@ramsay1.demon.co.uk> URL: https://gitlist.dev/e/504A48B4.9010109%40ramsay1.demon.co.uk In-Reply-To: <7vsjax8rap.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > Ramsay Jones writes: > >> The current implementation of git_path() is essentially the same as >> that of vsnpath(), with two minor differences. First, git_path() >> currently insists that the git directory path is no longer than >> PATH_MAX-100 characters in length. However, vsnpath() does not >> attempt this arbitrary 100 character reservation for the remaining >> path components. Second, vsnpath() uses the "is_dir_sep()" macro, >> rather than comparing directly to '/', to determine if the git_dir >> path component ends with a path separator. >> In order to benefit from the above improvements,... > > In the longer term, I think this goes in the right direction, but > the loss of reservation, especially when we know git_path() is used > by some callers to get the base directory in $GIT_DIR that want to > append stuff after the returned directory path to form the final > pathname, is a bit worrysome. It may be hiding a bug (lack of > proper limit check) on the callers' side. Hmm, at first I could not see what you found worrysome here. After all, the number of inputs which leads to success (i.e. does not result in an "/bad-path/" return) has been *increased* with this patch. However, I suppose you are concerned about something like this: char *git_dir = git_path(""); if (strcmp(git_dir, "/bad-path/") != 0) { /* * Having studied the implementation of git_path(), I know * that the buffer pointed to by git_dir has space for an * additional 100 chars. This is enough room to concatenate * the doberry path, so this is safe ... */ strcat(git_dir, doberry); /* oops */ } Yes? Hmm, yes it would be a little disapointing to see such parasitic code! ;-) You said above: "... especially when we know git_path() is used by some callers to get the base directory in $GIT_DIR ...". Can you point me to an example of such a caller; I have been unable to find any code which does this. ATB, Ramsay Jones ```