[PATCH] Handle the errors from chdir in set_work_tree

[PATCH] Handle the errors from chdir in set_work_tree

[Alex Riesen]

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

These I haven't seen yet. Wouldn't like such a surprise though.
Also unstatic rel, it seems to be declared static accidentally.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

One gets paranoid when exposed to "commercial"
systems for too long.

 setup.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

[-- Attachment #2: 0002-Handle-the-errors-from-chdir-in-set_work_tree.txt --]
[-- Type: text/plain, Size: 1289 bytes --]

From 602d49acd3e5285974cc1b7c4337301b13bb54b8 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Thu, 2 Aug 2007 16:51:57 +0200
Subject: [PATCH] Handle the errors from chdir in set_work_tree

These I haven't seen yet. Wouldn't like such a surprise though.
Also unstatic rel, it seems to be declared static accidentally.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 setup.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/setup.c b/setup.c
index 1beba7e..4e6bb76 100644
--- a/setup.c
+++ b/setup.c
@@ -201,8 +201,8 @@ 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 */
@@ -220,8 +220,10 @@ 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);
-		strcat(rel, "/");
+		if (chdir(dir))
+			rel = NULL;
+		else
+			strcat(rel, "/");
 		inside_git_dir = 0;
 	} else {
 		rel = NULL;
-- 
1.5.3.rc3.145.g4d9cdb

Re: [PATCH] Handle the errors from chdir in set_work_tree

[Johannes Schindelin]

Hi,

On Thu, 2 Aug 2007, Alex Riesen wrote:

> These I haven't seen yet. Wouldn't like such a surprise though.
> Also unstatic rel, it seems to be declared static accidentally.
> 
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>

Acked-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

Ciao,
Dscho

Re: [PATCH] Handle the errors from chdir in set_work_tree

[Junio C Hamano]

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

> These I haven't seen yet. Wouldn't like such a surprise though.
> ...
> @@ -220,8 +220,10 @@ 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);
> -		strcat(rel, "/");
> +		if (chdir(dir))
> +			rel = NULL;
> +		else
> +			strcat(rel, "/");
>  		inside_git_dir = 0;
>  	} else {
>  		rel = NULL;

Shouldn't it die() instead, though?

Consolidating two of your patches, would this be Ok?

-- >8 --
Fix work-tree related breakages

In set_work_tree(), variable rel needs to be reinitialized to
NULL on every call (it should not be static).

Make sure the incoming dir variable is not too long before
copying to the temporary buffer, and make sure chdir to the
resulting directory succeeds.

---

 setup.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/setup.c b/setup.c
index 3653092..4945eb3 100644
--- a/setup.c
+++ b/setup.c
@@ -201,26 +201,32 @@ 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;
-	int len, postfix_len = strlen(DEFAULT_GIT_DIR_ENVIRONMENT) + 1;
+	char dir_buffer[PATH_MAX], *rel = NULL;
+	static char buffer[PATH_MAX + 1];
+	int len, suffix_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 > 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';
 
 		/* are we inside the default work tree? */
 		rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);
 	}
+
 	/* if rel is set, the cwd is _not_ the current working tree */
 	if (rel && *rel) {
 		if (!is_absolute_path(dir))
 			set_git_dir(make_absolute_path(dir));
 		dir = dir_buffer;
-		chdir(dir);
-		strcat(rel, "/");
+		if (chdir(dir))
+			die("cannot chdir to %s: %s", dir, strerror(errno));
+		else
+			strcat(rel, "/");
 		inside_git_dir = 0;
 	} else {
 		rel = NULL;

Re: [PATCH] Handle the errors from chdir in set_work_tree

[Johannes Schindelin]

Hi,

On Thu, 2 Aug 2007, Junio C Hamano wrote:

> Fix work-tree related breakages
> 
> In set_work_tree(), variable rel needs to be reinitialized to
> NULL on every call (it should not be static).
> 
> Make sure the incoming dir variable is not too long before
> copying to the temporary buffer, and make sure chdir to the
> resulting directory succeeds.

ACK.

(Forget my patch, please)

Ciao,
Dscho

Re: [PATCH] Handle the errors from chdir in set_work_tree

[Alex Riesen]

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.

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

Re: [PATCH] Handle the errors from chdir in set_work_tree

[Junio C Hamano]

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

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