Also, 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@xxxxxxxxx>
---
setup.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
On 8/3/07, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Alex Riesen <raa.lkml@xxxxxxxxx> writes:
>
> > Junio C Hamano, Thu, Aug 02, 2007 23:58:41 +0200:
> >> "Alex Riesen" <raa.lkml@xxxxxxxxx> 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.
> >> 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@xxxxxxxxx>
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@xxxxxxxxx>
---
setup.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
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
