* [PATCH] Fix set_work_tree on cygwin @ 2007-08-02 15:25 Alex Riesen 2007-08-02 15:38 ` Johannes Schindelin 0 siblings, 1 reply; 9+ messages in thread From: Alex Riesen @ 2007-08-02 15:25 UTC (permalink / raw) To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano [-- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix set_work_tree on cygwin 2007-08-02 15:25 [PATCH] Fix set_work_tree on cygwin Alex Riesen @ 2007-08-02 15:38 ` Johannes Schindelin 2007-08-02 20:49 ` Alex Riesen 0 siblings, 1 reply; 9+ messages in thread From: Johannes Schindelin @ 2007-08-02 15:38 UTC (permalink / raw) To: Alex Riesen; +Cc: Git Mailing List, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix set_work_tree on cygwin 2007-08-02 15:38 ` Johannes Schindelin @ 2007-08-02 20:49 ` Alex Riesen 2007-08-02 21:04 ` Johannes Schindelin 0 siblings, 1 reply; 9+ messages in thread From: Alex Riesen @ 2007-08-02 20:49 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Git Mailing List, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix set_work_tree on cygwin 2007-08-02 20:49 ` Alex Riesen @ 2007-08-02 21:04 ` Johannes Schindelin 2007-08-02 21:14 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Johannes Schindelin @ 2007-08-02 21:04 UTC (permalink / raw) To: Alex Riesen; +Cc: Git Mailing List, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix set_work_tree on cygwin 2007-08-02 21:04 ` Johannes Schindelin @ 2007-08-02 21:14 ` Junio C Hamano 2007-08-02 21:36 ` Johannes Schindelin 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2007-08-02 21:14 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Alex Riesen, Git Mailing List 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix set_work_tree on cygwin 2007-08-02 21:14 ` Junio C Hamano @ 2007-08-02 21:36 ` Johannes Schindelin 2007-08-02 21:43 ` Junio C Hamano 2007-08-02 22:02 ` [PATCH] Fix unterminated string copy in set_work_tree Alex Riesen 0 siblings, 2 replies; 9+ messages in thread From: Johannes Schindelin @ 2007-08-02 21:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: Alex Riesen, Git Mailing List 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Fix set_work_tree on cygwin 2007-08-02 21:36 ` Johannes Schindelin @ 2007-08-02 21:43 ` Junio C Hamano 2007-08-02 22:04 ` [PATCH] Allow setup_work_tree() to be called several times Johannes Schindelin 2007-08-02 22:02 ` [PATCH] Fix unterminated string copy in set_work_tree Alex Riesen 1 sibling, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2007-08-02 21:43 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Alex Riesen, Git Mailing List 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] Allow setup_work_tree() to be called several times 2007-08-02 21:43 ` Junio C Hamano @ 2007-08-02 22:04 ` Johannes Schindelin 0 siblings, 0 replies; 9+ messages in thread From: Johannes Schindelin @ 2007-08-02 22:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: Alex Riesen, Git Mailing List 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] Fix unterminated string copy in set_work_tree 2007-08-02 21:36 ` Johannes Schindelin 2007-08-02 21:43 ` Junio C Hamano @ 2007-08-02 22:02 ` Alex Riesen 1 sibling, 0 replies; 9+ messages in thread From: Alex Riesen @ 2007-08-02 22:02 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Junio C Hamano, Git Mailing List 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2007-08-02 22:04 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-08-02 15:25 [PATCH] Fix set_work_tree on cygwin Alex Riesen 2007-08-02 15:38 ` Johannes Schindelin 2007-08-02 20:49 ` Alex Riesen 2007-08-02 21:04 ` Johannes Schindelin 2007-08-02 21:14 ` Junio C Hamano 2007-08-02 21:36 ` Johannes Schindelin 2007-08-02 21:43 ` Junio C Hamano 2007-08-02 22:04 ` [PATCH] Allow setup_work_tree() to be called several times Johannes Schindelin 2007-08-02 22:02 ` [PATCH] Fix unterminated string copy in set_work_tree Alex Riesen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).