# [PATCH] Fix set_work_tree on cygwin 9 messages from 2007-08-02 to 2007-08-02. Participants: Alex Riesen, Johannes Schindelin, Junio C Hamano. Thread: https://gitlist.dev/t/9354 ## Alex Riesen, 2007-08-02 15:25 Subject: [PATCH] Fix set_work_tree on cygwin Message-ID: <81b0412b0708020825q4b64c47r3fa1d67858271b1e@mail.gmail.com> URL: https://gitlist.dev/e/81b0412b0708020825q4b64c47r3fa1d67858271b1e%40mail.gmail.com ``` 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 --- 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 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 --- 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 ``` ## Johannes Schindelin, 2007-08-02 15:38 Subject: Re: [PATCH] Fix set_work_tree on cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0708021636470.14781%40racer.site In-Reply-To: <81b0412b0708020825q4b64c47r3fa1d67858271b1e@mail.gmail.com> ``` 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 ``` ## Alex Riesen, 2007-08-02 20:49 Subject: Re: [PATCH] Fix set_work_tree on cygwin Message-ID: <20070802204909.GA2829@steel.home> URL: https://gitlist.dev/e/20070802204909.GA2829%40steel.home In-Reply-To: ``` 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. ``` ## Johannes Schindelin, 2007-08-02 21:04 Subject: Re: [PATCH] Fix set_work_tree on cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0708022204170.14781%40racer.site In-Reply-To: <20070802204909.GA2829@steel.home> ``` 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 ``` ## Junio C Hamano, 2007-08-02 21:14 Subject: Re: [PATCH] Fix set_work_tree on cygwin Message-ID: <7vwswdtz98.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vwswdtz98.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Johannes Schindelin 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? ``` ## Johannes Schindelin, 2007-08-02 21:36 Subject: Re: [PATCH] Fix set_work_tree on cygwin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0708022230070.14781%40racer.site In-Reply-To: <7vwswdtz98.fsf@assigned-by-dhcp.cox.net> ``` Hi, On Thu, 2 Aug 2007, Junio C Hamano wrote: > Johannes Schindelin 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 --- 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 ``` ## Junio C Hamano, 2007-08-02 21:43 Subject: Re: [PATCH] Fix set_work_tree on cygwin Message-ID: <7vk5sdtxx0.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vk5sdtxx0.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Johannes Schindelin 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? ``` ## Alex Riesen, 2007-08-02 22:02 Subject: [PATCH] Fix unterminated string copy in set_work_tree Message-ID: <20070802220255.GB2829@steel.home> URL: https://gitlist.dev/e/20070802220255.GB2829%40steel.home In-Reply-To: ``` Use strlcpy which zero-terminates the output string Signed-off-by: Alex Riesen --- 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 ``` ## Johannes Schindelin, 2007-08-02 22:04 Subject: [PATCH] Allow setup_work_tree() to be called several times Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0708022302360.14781%40racer.site In-Reply-To: <7vk5sdtxx0.fsf@assigned-by-dhcp.cox.net> ``` 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 --- On Thu, 2 Aug 2007, Junio C Hamano wrote: > Johannes Schindelin 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 ```