threads / patch / 9354

patchFix set_work_tree on cygwin

Subject: [PATCH] Fix set_work_tree on cygwin

## tl;dr

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

replies: 8people: 3as markdown or json

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

I don't know why, but for some unknown reason the buffer did not contain zeros. This broke t1500-rev-parse.sh (the test for GIT_DIR=../.git git rev-parse --show-prefix).

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

It could be a memory corruption somewhere, but I really was unable to find what could that, nor could I reproduce the problem on a handy linux box.

 setup.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
From e5e7adb7db0d9c00a938f32281774f6a7532b44a Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Thu, 2 Aug 2007 16:33:02 +0200
Subject: [PATCH] Fix set_work_tree on cygwin

I don't know why, but for some unknown reason the buffer did not contain zeros. This broke t1500-rev-parse.sh (the test for GIT_DIR=../.git git rev-parse --show-prefix).

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 setup.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to setup.c +2 −1
diff --git a/setup.c b/setup.c
index 3653092..1beba7e 100644
--- a/setup.c
+++ b/setup.c
@@ -209,7 +209,8 @@ const char *set_work_tree(const char *dir)
 	len = strlen(dir);
 	if (len > postfix_len && !strcmp(dir + len - postfix_len,
 				"/" DEFAULT_GIT_DIR_ENVIRONMENT)) {
-			strncpy(dir_buffer, dir, len - postfix_len);
+		strncpy(dir_buffer, dir, len - postfix_len);
+		dir_buffer[len - postfix_len] = '\0';
 
 		/* are we inside the default work tree? */
 		rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);

-- 
1.5.3.rc3.145.g4d9cdb
Johannes Schindelin· Aug 2, 2007, 15:38 UTC · re: Alex Riesen · lore

Re: [PATCH] Fix set_work_tree on cygwin

Hi,
On Thu, 2 Aug 2007, Alex Riesen wrote:
Show 10 quoted lines
>@@ -209,7 +209,8 @@ const char *set_work_tree(const char *dir)
>        len = strlen(dir);
>        if (len > postfix_len && !strcmp(dir + len - postfix_len,
>                                "/" DEFAULT_GIT_DIR_ENVIRONMENT)) {
>-                       strncpy(dir_buffer, dir, len - postfix_len);
>+               strncpy(dir_buffer, dir, len - postfix_len);
>+               dir_buffer[len - postfix_len] = '\0';
>
>                /* are we inside the default work tree? */
>                rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);

Darn, darn, darn. strncpy does _not_ NUL terminate. I keep forgetting that.

Better use strlcpy()?

Ciao, Dscho

Alex Riesen· Aug 2, 2007, 20:49 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Fix set_work_tree on cygwin

Johannes Schindelin, Thu, Aug 02, 2007 17:38:37 +0200:
Show 19 quoted lines
> Hi,
> 
> On Thu, 2 Aug 2007, Alex Riesen wrote:
> 
> >@@ -209,7 +209,8 @@ const char *set_work_tree(const char *dir)
> >        len = strlen(dir);
> >        if (len > postfix_len && !strcmp(dir + len - postfix_len,
> >                                "/" DEFAULT_GIT_DIR_ENVIRONMENT)) {
> >-                       strncpy(dir_buffer, dir, len - postfix_len);
> >+               strncpy(dir_buffer, dir, len - postfix_len);
> >+               dir_buffer[len - postfix_len] = '\0';
> >
> >                /* are we inside the default work tree? */
> >                rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);
> 
> Darn, darn, darn.  strncpy does _not_ NUL terminate.  I keep forgetting 
> that.
> 
> Better use strlcpy()?

Of course, but it just should not be needed at all: static supposed to be zeroed.

Johannes Schindelin· Aug 2, 2007, 21:04 UTC · re: Alex Riesen · lore

Re: [PATCH] Fix set_work_tree on cygwin

Hi,
On Thu, 2 Aug 2007, Alex Riesen wrote:
Show 22 quoted lines
> Johannes Schindelin, Thu, Aug 02, 2007 17:38:37 +0200:
> 
> > On Thu, 2 Aug 2007, Alex Riesen wrote:
> > 
> > >@@ -209,7 +209,8 @@ const char *set_work_tree(const char *dir)
> > >        len = strlen(dir);
> > >        if (len > postfix_len && !strcmp(dir + len - postfix_len,
> > >                                "/" DEFAULT_GIT_DIR_ENVIRONMENT)) {
> > >-                       strncpy(dir_buffer, dir, len - postfix_len);
> > >+               strncpy(dir_buffer, dir, len - postfix_len);
> > >+               dir_buffer[len - postfix_len] = '\0';
> > >
> > >                /* are we inside the default work tree? */
> > >                rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);
> > 
> > Darn, darn, darn.  strncpy does _not_ NUL terminate.  I keep forgetting 
> > that.
> > 
> > Better use strlcpy()?
> 
> Of course, but it just should not be needed at all: static supposed to
> be zeroed.

Certainly. But reality outweighs theory, and so I Ack either your patch or replacing it by strlcpy().

Ciao, Dscho

Junio C Hamano· Aug 2, 2007, 21:14 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Fix set_work_tree on cygwin

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 29 quoted lines
> Hi,
>
> On Thu, 2 Aug 2007, Alex Riesen wrote:
>
>> Johannes Schindelin, Thu, Aug 02, 2007 17:38:37 +0200:
>> 
>> > On Thu, 2 Aug 2007, Alex Riesen wrote:
>> > 
>> > >@@ -209,7 +209,8 @@ const char *set_work_tree(const char *dir)
>> > >        len = strlen(dir);
>> > >        if (len > postfix_len && !strcmp(dir + len - postfix_len,
>> > >                                "/" DEFAULT_GIT_DIR_ENVIRONMENT)) {
>> > >-                       strncpy(dir_buffer, dir, len - postfix_len);
>> > >+               strncpy(dir_buffer, dir, len - postfix_len);
>> > >+               dir_buffer[len - postfix_len] = '\0';
>> > >
>> > >                /* are we inside the default work tree? */
>> > >                rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);
>> > 
>> > Darn, darn, darn.  strncpy does _not_ NUL terminate.  I keep forgetting 
>> > that.
>> > 
>> > Better use strlcpy()?
>> 
>> Of course, but it just should not be needed at all: static supposed to
>> be zeroed.
>
> Certainly.  But reality outweighs theory, and so I Ack either your patch 
> or replacing it by strlcpy().

Static is supposed to be zeroed and also is supposed to retain the value from the previous call. I am guessing from the change to make "rel" to non-static that this function is called twice perhaps?

Johannes Schindelin· Aug 2, 2007, 21:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix set_work_tree on cygwin

Hi,
On Thu, 2 Aug 2007, Junio C Hamano wrote:
Show 36 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Hi,
> >
> > On Thu, 2 Aug 2007, Alex Riesen wrote:
> >
> >> Johannes Schindelin, Thu, Aug 02, 2007 17:38:37 +0200:
> >> 
> >> > On Thu, 2 Aug 2007, Alex Riesen wrote:
> >> > 
> >> > >@@ -209,7 +209,8 @@ const char *set_work_tree(const char *dir)
> >> > >        len = strlen(dir);
> >> > >        if (len > postfix_len && !strcmp(dir + len - postfix_len,
> >> > >                                "/" DEFAULT_GIT_DIR_ENVIRONMENT)) {
> >> > >-                       strncpy(dir_buffer, dir, len - postfix_len);
> >> > >+               strncpy(dir_buffer, dir, len - postfix_len);
> >> > >+               dir_buffer[len - postfix_len] = '\0';
> >> > >
> >> > >                /* are we inside the default work tree? */
> >> > >                rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);
> >> > 
> >> > Darn, darn, darn.  strncpy does _not_ NUL terminate.  I keep forgetting 
> >> > that.
> >> > 
> >> > Better use strlcpy()?
> >> 
> >> Of course, but it just should not be needed at all: static supposed to
> >> be zeroed.
> >
> > Certainly.  But reality outweighs theory, and so I Ack either your patch 
> > or replacing it by strlcpy().
> 
> Static is supposed to be zeroed and also is supposed to retain
> the value from the previous call.  I am guessing from the change
> to make "rel" to non-static that this function is called twice
> perhaps?

Apparently (but I would feel safer with strlcpy() anyway). git-read-tree is the first and only offender which comes up in the test suite:

-- snipsnap -- [PATCH] read-tree: remove unnecessary call to setup_git_directory()

read-tree is already marked with RUN_SETUP in git.c, so there is no need to call setup_git_directory() a second time.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-read-tree.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
Show changes to builtin-read-tree.c +0 −1
diff --git a/builtin-read-tree.c b/builtin-read-tree.c
index 41f8110..a3b17a3 100644
--- a/builtin-read-tree.c
+++ b/builtin-read-tree.c
@@ -97,7 +97,6 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = -1;
 
-	setup_git_directory();
 	git_config(git_default_config);
 
 	newfd = hold_locked_index(&lock_file, 1);

-- 
1.5.3.rc3.121.g7f37
Junio C Hamano· Aug 2, 2007, 21:43 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Fix set_work_tree on cygwin

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 6 quoted lines
>> Static is supposed to be zeroed and also is supposed to retain
>> the value from the previous call.  I am guessing from the change
>> to make "rel" to non-static that this function is called twice
>> perhaps?
>
> Apparently (but I would feel safer with strlcpy() anyway)...
Yup, send an appliable "final" version, somebody please?
> ...  git-read-tree 
> is the first and only offender which comes up in the test suite:
It is unclear.

Is this an optimization, or enforcing the new world order? IOW, is it now banned to call setup twice?

Johannes Schindelin· Aug 2, 2007, 22:04 UTC · re: Junio C Hamano · lore

[PATCH] Allow setup_work_tree() to be called several times

setup_work_tree() used to rely on a static buffer being initialized to all zeroes. While at it, unstatify a pointer.

Noticed by Alex Riesen.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	On Thu, 2 Aug 2007, Junio C Hamano wrote:
	> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
	> 
	> >> Static is supposed to be zeroed and also is supposed to retain
	> >> the value from the previous call.  I am guessing from the change
	> >> to make "rel" to non-static that this function is called twice
	> >> perhaps?
	> >
	> > Apparently (but I would feel safer with strlcpy() anyway)...
	> 
	> Yup, send an appliable "final" version, somebody please?
	Here you are.
	> > ...  git-read-tree 
	> > is the first and only offender which comes up in the test suite:
	> 
	> It is unclear.
	> 
	> Is this an optimization, or enforcing the new world order?  IOW,
	> is it now banned to call setup twice?
	It is purely an optimization, because we allow calling setup twice 
	with this patch...  but we do not recommend it, as it is 
	unnecessary.
 setup.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Show changes to setup.c +3 −3
diff --git a/setup.c b/setup.c
index 3653092..16745f9 100644
--- a/setup.c
+++ b/setup.c
@@ -201,15 +201,15 @@ 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);
+			strlcpy(dir_buffer, dir, len - postfix_len + 1);
 
 		/* are we inside the default work tree? */
 		rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);

-- 
1.5.3.rc3.121.g7f37
Alex Riesen· Aug 2, 2007, 22:02 UTC · re: Johannes Schindelin · lore

[PATCH] Fix unterminated string copy in set_work_tree

Use strlcpy which zero-terminates the output string
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Johannes Schindelin, Thu, Aug 02, 2007 23:36:37 +0200:
Show 6 quoted lines
> On Thu, 2 Aug 2007, Junio C Hamano wrote:
> > 
> > Static is supposed to be zeroed and also is supposed to retain
> > the value from the previous call.  I am guessing from the change
> > to make "rel" to non-static that this function is called twice
> > perhaps?

Actually, I was very confused. When I wrote about cygwin problems, I actually debugged it for dir_buffer, real stack-based variable, which of course is not zero-initialized. For an unknown reason I confused the variable with buffer, which is static. "rel" should be left of this particular discussion (it just does not matter whether it is static or not in this context).

So the fix is a real fix for real problem which just happens to be invisible on our linux systems.

> Apparently (but I would feel safer with strlcpy() anyway).  git-read-tree 
> is the first and only offender which comes up in the test suite:
Yes, I feel so too, so here it is.
 setup.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to setup.c +1 −1
diff --git a/setup.c b/setup.c
index 3653092..27d585c 100644
--- a/setup.c
+++ b/setup.c
@@ -209,7 +209,7 @@ const char *set_work_tree(const char *dir)
 	len = strlen(dir);
 	if (len > postfix_len && !strcmp(dir + len - postfix_len,
 				"/" DEFAULT_GIT_DIR_ENVIRONMENT)) {
-			strncpy(dir_buffer, dir, len - postfix_len);
+		strlcpy(dir_buffer, dir, len - postfix_len + 1);
 
 		/* are we inside the default work tree? */
 		rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);

-- 
1.5.3.rc3.139.ga57724

← back to recent threads