[PATCH] Fix unterminated string in set_work_tree and improve error handling

[PATCH] Fix unterminated string in set_work_tree and improve error handling

[Alex Riesen]

[-- Attachment #1: Type: text/plain, Size: 1779 bytes --]

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@gmail.com>
---
 setup.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

On 8/3/07, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

> >> 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.

[-- Attachment #2: 0001-Check-if-the-given-dir-fits-in-the-temporary-dir_buffe.txt --]
[-- Type: text/plain, Size: 1604 bytes --]

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(-)

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

Re: [PATCH] Fix unterminated string in set_work_tree and improve error handling

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

>> >> 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?

I sense you haven't read my replacement patch (Dscho Acked it).
I am talking about this part:

+	if (len > suffix_len &&
+	    !strcmp(dir + len - suffix_len, "/" DEFAULT_GIT_DIR_ENVIRONMENT)) {
+		if ((len - suffix_len) >= sizeof(dir_buffer))
+			die("directory name too long");
+		memcpy(dir_buffer, dir, len - suffix_len);
+		dir_buffer[len - suffix_len] = '\0';

Re: [PATCH] Fix unterminated string in set_work_tree and improve error handling

[Alex Riesen]

On 8/3/07, Junio C Hamano <gitster@pobox.com> wrote:
> I sense you haven't read my replacement patch (Dscho Acked it).

I did. I had to pay attention to what I read, though. Sorry.