Alex RiesenAlso, check if it fits in the temporary dir_buffer and can be chdir-ed into. Die for errors.
Signed-off-by: Alex Riesen <raa.lkml@gmail.com> --- setup.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-)
On 8/3/07, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> Alex Riesen <raa.lkml@gmail.com> writes: > > > Junio C Hamano, Thu, Aug 02, 2007 23:58:41 +0200: > >> "Alex Riesen" <raa.lkml@gmail.com> writes: > >> > + if (chdir(dir)) > >> > + rel = NULL; > > ... > >> > >> Shouldn't it die() instead, though? > > > > Dunno. Don't like dying. > > I do not understand your reasoning. Why is it better to use > mysteriously truncated path, which may result in doing something > the user did not ask you to, rather than saying "No, my > temporary buffer is not equipped to handle such an insanely long > pathname"?
AFAIU, it is not only a truncated path which is a problem for chdir, but any failure to chdir, for any reason. And, if I understand set_work_tree returning NULL correctly (I assign rel NULL, which should be returned) - it is an error, and can be handled in the caller. But... Hmm... Looking at the code again, rel==NULL just means there is no prefix! You're right, better let it die.
Show 11 quoted lines
> >> Consolidating two of your patches, would this be Ok? > > > > Yes, but you may consider replacing strncpy with strlcpy: > > > >> + memcpy(dir_buffer, dir, len - suffix_len); > >> + dir_buffer[len - suffix_len] = '\0'; > > > > strlcpy(dir_buffer, dir, len - suffix_len + 1); > > Does that buy us that much? Before going to that codepath, we > have made sure the result fits, haven't we?
No, we haven't. The code just checks if the given work tree path is longer than "/.git", to be able to cut off that safely.
From 7e55d4ed84e5867b217c9e0bc1964444f13eaef2 Mon Sep 17 00:00:00 2001 From: Alex Riesen <raa.lkml@gmail.com> Date: Fri, 3 Aug 2007 07:41:26 +0200 Subject: [PATCH] Fix unterminated string in set_work_tree and improve error handling
Signed-off-by: Alex Riesen <raa.lkml@gmail.com> --- setup.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-)
Show changes to setup.c +8 −4
diff --git a/setup.c b/setup.c index 3653092..6006fc5 100644 --- a/setup.c +++ b/setup.c @@ -201,15 +201,18 @@ int is_inside_work_tree(void) */ const char *set_work_tree(const char *dir) { - char dir_buffer[PATH_MAX]; - static char buffer[PATH_MAX + 1], *rel = NULL; + char dir_buffer[PATH_MAX], *rel = NULL; + static char buffer[PATH_MAX + 1]; int len, postfix_len = strlen(DEFAULT_GIT_DIR_ENVIRONMENT) + 1; /* strip the variable 'dir' of the postfix "/.git" if it has it */ len = strlen(dir); if (len > postfix_len && !strcmp(dir + len - postfix_len, "/" DEFAULT_GIT_DIR_ENVIRONMENT)) { - strncpy(dir_buffer, dir, len - postfix_len); + if (len - postfix_len < sizeof(dir_buffer)) + strlcpy(dir_buffer, dir, len - postfix_len + 1); + else + die("%s is too long", dir); /* are we inside the default work tree? */ rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer); @@ -219,7 +222,8 @@ const char *set_work_tree(const char *dir) if (!is_absolute_path(dir)) set_git_dir(make_absolute_path(dir)); dir = dir_buffer; - chdir(dir); + if (chdir(dir)) + die("cannot chdir to %s: %s", dir, strerror(errno)); strcat(rel, "/"); inside_git_dir = 0; } else {
-- 1.5.3.rc3.145.g4d9cdb
