threads / patch / 9355

patchHandle the errors from chdir in set_work_tree

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

## tl;dr

6 messages between Aug 2, 2007 and Aug 2, 2007. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Alex Riesen· Aug 2, 2007, 15:27 UTC · lore

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(-)
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(-)
Show changes to setup.c +6 −4
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
Johannes Schindelin· Aug 2, 2007, 15:40 UTC · re: Alex Riesen · lore

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

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

Junio C Hamano· Aug 2, 2007, 21:58 UTC · re: Alex Riesen · lore

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

"Alex Riesen" <raa.lkml@gmail.com> writes:
Show 15 quoted lines
> 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(-)
Show changes to setup.c +14 −8
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;
Johannes Schindelin· Aug 2, 2007, 22:07 UTC · re: Junio C Hamano · lore

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

Hi,
On Thu, 2 Aug 2007, Junio C Hamano wrote:
Show 8 quoted lines
> 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

Alex Riesen· Aug 2, 2007, 22:15 UTC · re: Junio C Hamano · lore

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

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);
Junio C Hamano· Aug 2, 2007, 22:18 UTC · re: Alex Riesen · lore

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

Alex Riesen <raa.lkml@gmail.com> writes:
Show 9 quoted lines
> 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"?

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

← back to recent threads