[PATCH] Fix set_work_tree on cygwin

[PATCH] Fix set_work_tree on cygwin

[Alex Riesen]

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

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

[-- Attachment #2: 0001-Fix-set_work_tree-on-cygwin.txt --]
[-- Type: text/plain, Size: 1037 bytes --]

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

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

Re: [PATCH] Fix set_work_tree on cygwin

[Johannes Schindelin]

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

Ciao,
Dscho

Re: [PATCH] Fix set_work_tree on cygwin

[Alex Riesen]

Johannes Schindelin, Thu, Aug 02, 2007 17:38:37 +0200:
> 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.

Re: [PATCH] Fix set_work_tree on cygwin

[Johannes Schindelin]

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

Ciao,
Dscho

Re: [PATCH] Fix set_work_tree on cygwin

[Junio C Hamano]

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?

Re: [PATCH] Fix set_work_tree on cygwin

[Johannes Schindelin]

Hi,

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

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

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

Re: [PATCH] Fix set_work_tree on cygwin

[Junio C Hamano]

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?

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

[PATCH] Fix unterminated string copy in set_work_tree

[Alex Riesen]

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

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

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

[Johannes Schindelin]

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

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