{"thread":{"id":"9354","subject":"[PATCH] Fix set_work_tree on cygwin","startedAt":"2007-08-02T15:25:58Z","lastAt":"2007-08-02T22:04:13Z","messageCount":9,"participants":["Alex Riesen","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"49434","messageId":"81b0412b0708020825q4b64c47r3fa1d67858271b1e@mail.gmail.com","threadId":"9354","inReplyTo":null,"subject":"[PATCH] Fix set_work_tree on cygwin","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-08-02T15:25:58Z","receivedAt":"2007-08-02T15:25:58Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"I don't know why, but for some unknown reason the buffer did not\ncontain zeros. This broke t1500-rev-parse.sh (the test for\nGIT_DIR=../.git git rev-parse --show-prefix).\n\nSigned-off-by: Alex Riesen \n---\n\nIt could be a memory corruption somewhere, but I really was\nunable to find what could that, nor could I reproduce the problem\non a handy linux box.\n\n setup.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\n\nFrom e5e7adb7db0d9c00a938f32281774f6a7532b44a Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen \nDate: Thu, 2 Aug 2007 16:33:02 +0200\nSubject: [PATCH] Fix set_work_tree on cygwin\n\nI don't know why, but for some unknown reason the buffer did not\ncontain zeros. This broke t1500-rev-parse.sh (the test for\nGIT_DIR=../.git git rev-parse --show-prefix).\n\nSigned-off-by: Alex Riesen \n---\n setup.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 3653092..1beba7e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -209,7 +209,8 @@ const char *set_work_tree(const char *dir)\n \tlen = strlen(dir);\n \tif (len > postfix_len && !strcmp(dir + len - postfix_len,\n \t\t\t\t\"/\" DEFAULT_GIT_DIR_ENVIRONMENT)) {\n-\t\t\tstrncpy(dir_buffer, dir, len - postfix_len);\n+\t\tstrncpy(dir_buffer, dir, len - postfix_len);\n+\t\tdir_buffer[len - postfix_len] = '\\0';\n \n \t\t/* are we inside the default work tree? */\n \t\trel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);\n-- \n1.5.3.rc3.145.g4d9cdb\n\n"},{"id":"49437","messageId":"Pine.LNX.4.64.0708021636470.14781@racer.site","threadId":"9354","inReplyTo":"81b0412b0708020825q4b64c47r3fa1d67858271b1e@mail.gmail.com","subject":"Re: [PATCH] Fix set_work_tree on cygwin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-08-02T15:38:37Z","receivedAt":"2007-08-02T15:38:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 2 Aug 2007, Alex Riesen wrote:\n\n>@@ -209,7 +209,8 @@ const char *set_work_tree(const char *dir)\n> len = strlen(dir);\n> if (len > postfix_len && !strcmp(dir + len - postfix_len,\n> \"/\" DEFAULT_GIT_DIR_ENVIRONMENT)) {\n>- strncpy(dir_buffer, dir, len - postfix_len);\n>+ strncpy(dir_buffer, dir, len - postfix_len);\n>+ dir_buffer[len - postfix_len] = '\\0';\n>\n> /* are we inside the default work tree? */\n> rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);\n\nDarn, darn, darn. strncpy does _not_ NUL terminate. I keep forgetting \nthat.\n\nBetter use strlcpy()?\n\nCiao,\nDscho\n"},{"id":"49478","messageId":"20070802204909.GA2829@steel.home","threadId":"9354","inReplyTo":"Pine.LNX.4.64.0708021636470.14781@racer.site","subject":"Re: [PATCH] Fix set_work_tree on cygwin","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-08-02T20:49:09Z","receivedAt":"2007-08-02T20:49:09Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Johannes Schindelin, Thu, Aug 02, 2007 17:38:37 +0200:\n> Hi,\n> \n> On Thu, 2 Aug 2007, Alex Riesen wrote:\n> \n> >@@ -209,7 +209,8 @@ const char *set_work_tree(const char *dir)\n> > len = strlen(dir);\n> > if (len > postfix_len && !strcmp(dir + len - postfix_len,\n> > \"/\" DEFAULT_GIT_DIR_ENVIRONMENT)) {\n> >- strncpy(dir_buffer, dir, len - postfix_len);\n> >+ strncpy(dir_buffer, dir, len - postfix_len);\n> >+ dir_buffer[len - postfix_len] = '\\0';\n> >\n> > /* are we inside the default work tree? */\n> > rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);\n> \n> Darn, darn, darn. strncpy does _not_ NUL terminate. I keep forgetting \n> that.\n> \n> Better use strlcpy()?\n\nOf course, but it just should not be needed at all: static supposed to\nbe zeroed.\n"},{"id":"49483","messageId":"Pine.LNX.4.64.0708022204170.14781@racer.site","threadId":"9354","inReplyTo":"20070802204909.GA2829@steel.home","subject":"Re: [PATCH] Fix set_work_tree on cygwin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-08-02T21:04:56Z","receivedAt":"2007-08-02T21:04:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 2 Aug 2007, Alex Riesen wrote:\n\n> Johannes Schindelin, Thu, Aug 02, 2007 17:38:37 +0200:\n> \n> > On Thu, 2 Aug 2007, Alex Riesen wrote:\n> > \n> > >@@ -209,7 +209,8 @@ const char *set_work_tree(const char *dir)\n> > > len = strlen(dir);\n> > > if (len > postfix_len && !strcmp(dir + len - postfix_len,\n> > > \"/\" DEFAULT_GIT_DIR_ENVIRONMENT)) {\n> > >- strncpy(dir_buffer, dir, len - postfix_len);\n> > >+ strncpy(dir_buffer, dir, len - postfix_len);\n> > >+ dir_buffer[len - postfix_len] = '\\0';\n> > >\n> > > /* are we inside the default work tree? */\n> > > rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);\n> > \n> > Darn, darn, darn. strncpy does _not_ NUL terminate. I keep forgetting \n> > that.\n> > \n> > Better use strlcpy()?\n> \n> Of course, but it just should not be needed at all: static supposed to\n> be zeroed.\n\nCertainly. But reality outweighs theory, and so I Ack either your patch \nor replacing it by strlcpy().\n\nCiao,\nDscho\n"},{"id":"49485","messageId":"7vwswdtz98.fsf@assigned-by-dhcp.cox.net","threadId":"9354","inReplyTo":"Pine.LNX.4.64.0708022204170.14781@racer.site","subject":"Re: [PATCH] Fix set_work_tree on cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-02T21:14:43Z","receivedAt":"2007-08-02T21:14:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi,\n>\n> On Thu, 2 Aug 2007, Alex Riesen wrote:\n>\n>> Johannes Schindelin, Thu, Aug 02, 2007 17:38:37 +0200:\n>> \n>> > On Thu, 2 Aug 2007, Alex Riesen wrote:\n>> > \n>> > >@@ -209,7 +209,8 @@ const char *set_work_tree(const char *dir)\n>> > > len = strlen(dir);\n>> > > if (len > postfix_len && !strcmp(dir + len - postfix_len,\n>> > > \"/\" DEFAULT_GIT_DIR_ENVIRONMENT)) {\n>> > >- strncpy(dir_buffer, dir, len - postfix_len);\n>> > >+ strncpy(dir_buffer, dir, len - postfix_len);\n>> > >+ dir_buffer[len - postfix_len] = '\\0';\n>> > >\n>> > > /* are we inside the default work tree? */\n>> > > rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);\n>> > \n>> > Darn, darn, darn. strncpy does _not_ NUL terminate. I keep forgetting \n>> > that.\n>> > \n>> > Better use strlcpy()?\n>> \n>> Of course, but it just should not be needed at all: static supposed to\n>> be zeroed.\n>\n> Certainly. But reality outweighs theory, and so I Ack either your patch \n> or replacing it by strlcpy().\n\nStatic is supposed to be zeroed and also is supposed to retain\nthe value from the previous call. I am guessing from the change\nto make \"rel\" to non-static that this function is called twice\nperhaps?\n"},{"id":"49489","messageId":"Pine.LNX.4.64.0708022230070.14781@racer.site","threadId":"9354","inReplyTo":"7vwswdtz98.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix set_work_tree on cygwin","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-08-02T21:36:37Z","receivedAt":"2007-08-02T21:36:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 2 Aug 2007, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Hi,\n> >\n> > On Thu, 2 Aug 2007, Alex Riesen wrote:\n> >\n> >> Johannes Schindelin, Thu, Aug 02, 2007 17:38:37 +0200:\n> >> \n> >> > On Thu, 2 Aug 2007, Alex Riesen wrote:\n> >> > \n> >> > >@@ -209,7 +209,8 @@ const char *set_work_tree(const char *dir)\n> >> > > len = strlen(dir);\n> >> > > if (len > postfix_len && !strcmp(dir + len - postfix_len,\n> >> > > \"/\" DEFAULT_GIT_DIR_ENVIRONMENT)) {\n> >> > >- strncpy(dir_buffer, dir, len - postfix_len);\n> >> > >+ strncpy(dir_buffer, dir, len - postfix_len);\n> >> > >+ dir_buffer[len - postfix_len] = '\\0';\n> >> > >\n> >> > > /* are we inside the default work tree? */\n> >> > > rel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);\n> >> > \n> >> > Darn, darn, darn. strncpy does _not_ NUL terminate. I keep forgetting \n> >> > that.\n> >> > \n> >> > Better use strlcpy()?\n> >> \n> >> Of course, but it just should not be needed at all: static supposed to\n> >> be zeroed.\n> >\n> > Certainly. But reality outweighs theory, and so I Ack either your patch \n> > or replacing it by strlcpy().\n> \n> Static is supposed to be zeroed and also is supposed to retain\n> the value from the previous call. I am guessing from the change\n> to make \"rel\" to non-static that this function is called twice\n> perhaps?\n\nApparently (but I would feel safer with strlcpy() anyway). git-read-tree \nis the first and only offender which comes up in the test suite:\n\n-- snipsnap --\n[PATCH] read-tree: remove unnecessary call to setup_git_directory()\n\nread-tree is already marked with RUN_SETUP in git.c, so there is\nno need to call setup_git_directory() a second time.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin-read-tree.c | 1 -\n 1 files changed, 0 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-read-tree.c b/builtin-read-tree.c\nindex 41f8110..a3b17a3 100644\n--- a/builtin-read-tree.c\n+++ b/builtin-read-tree.c\n@@ -97,7 +97,6 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \tmemset(&opts, 0, sizeof(opts));\n \topts.head_idx = -1;\n \n-\tsetup_git_directory();\n \tgit_config(git_default_config);\n \n \tnewfd = hold_locked_index(&lock_file, 1);\n-- \n1.5.3.rc3.121.g7f37\n"},{"id":"49492","messageId":"7vk5sdtxx0.fsf@assigned-by-dhcp.cox.net","threadId":"9354","inReplyTo":"Pine.LNX.4.64.0708022230070.14781@racer.site","subject":"Re: [PATCH] Fix set_work_tree on cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-02T21:43:39Z","receivedAt":"2007-08-02T21:43:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Static is supposed to be zeroed and also is supposed to retain\n>> the value from the previous call. I am guessing from the change\n>> to make \"rel\" to non-static that this function is called twice\n>> perhaps?\n>\n> Apparently (but I would feel safer with strlcpy() anyway)...\n\nYup, send an appliable \"final\" version, somebody please?\n\n> ... git-read-tree \n> is the first and only offender which comes up in the test suite:\n\nIt is unclear.\n\nIs this an optimization, or enforcing the new world order? IOW,\nis it now banned to call setup twice?\n"},{"id":"49495","messageId":"20070802220255.GB2829@steel.home","threadId":"9354","inReplyTo":"Pine.LNX.4.64.0708022230070.14781@racer.site","subject":"[PATCH] Fix unterminated string copy in set_work_tree","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-08-02T22:02:55Z","receivedAt":"2007-08-02T22:02:55Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Use strlcpy which zero-terminates the output string\n\nSigned-off-by: Alex Riesen \n---\nJohannes Schindelin, Thu, Aug 02, 2007 23:36:37 +0200:\n> On Thu, 2 Aug 2007, Junio C Hamano wrote:\n> > \n> > Static is supposed to be zeroed and also is supposed to retain\n> > the value from the previous call. I am guessing from the change\n> > to make \"rel\" to non-static that this function is called twice\n> > perhaps?\n\nActually, I was very confused. When I wrote about cygwin problems,\nI actually debugged it for dir_buffer, real stack-based variable,\nwhich of course is not zero-initialized. For an unknown reason I\nconfused the variable with buffer, which is static. \"rel\" should\nbe left of this particular discussion (it just does not matter whether\nit is static or not in this context).\n\nSo the fix is a real fix for real problem which just happens to be\ninvisible on our linux systems.\n\n> Apparently (but I would feel safer with strlcpy() anyway). git-read-tree \n> is the first and only offender which comes up in the test suite:\n\nYes, I feel so too, so here it is.\n\n setup.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 3653092..27d585c 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -209,7 +209,7 @@ const char *set_work_tree(const char *dir)\n \tlen = strlen(dir);\n \tif (len > postfix_len && !strcmp(dir + len - postfix_len,\n \t\t\t\t\"/\" DEFAULT_GIT_DIR_ENVIRONMENT)) {\n-\t\t\tstrncpy(dir_buffer, dir, len - postfix_len);\n+\t\tstrlcpy(dir_buffer, dir, len - postfix_len + 1);\n \n \t\t/* are we inside the default work tree? */\n \t\trel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);\n-- \n1.5.3.rc3.139.ga57724\n"},{"id":"49496","messageId":"Pine.LNX.4.64.0708022302360.14781@racer.site","threadId":"9354","inReplyTo":"7vk5sdtxx0.fsf@assigned-by-dhcp.cox.net","subject":"[PATCH] Allow setup_work_tree() to be called several times","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-08-02T22:04:13Z","receivedAt":"2007-08-02T22:04:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nsetup_work_tree() used to rely on a static buffer being initialized to all \nzeroes. While at it, unstatify a pointer.\n\nNoticed by Alex Riesen.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tOn Thu, 2 Aug 2007, Junio C Hamano wrote:\n\n\t> Johannes Schindelin writes:\n\t> \n\t> >> Static is supposed to be zeroed and also is supposed to retain\n\t> >> the value from the previous call. I am guessing from the change\n\t> >> to make \"rel\" to non-static that this function is called twice\n\t> >> perhaps?\n\t> >\n\t> > Apparently (but I would feel safer with strlcpy() anyway)...\n\t> \n\t> Yup, send an appliable \"final\" version, somebody please?\n\n\tHere you are.\n\n\t> > ... git-read-tree \n\t> > is the first and only offender which comes up in the test suite:\n\t> \n\t> It is unclear.\n\t> \n\t> Is this an optimization, or enforcing the new world order? IOW,\n\t> is it now banned to call setup twice?\n\n\tIt is purely an optimization, because we allow calling setup twice \n\twith this patch... but we do not recommend it, as it is \n\tunnecessary.\n\n setup.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 3653092..16745f9 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -201,15 +201,15 @@ int is_inside_work_tree(void)\n */\n const char *set_work_tree(const char *dir)\n {\n-\tchar dir_buffer[PATH_MAX];\n-\tstatic char buffer[PATH_MAX + 1], *rel = NULL;\n+\tchar dir_buffer[PATH_MAX], *rel = NULL;\n+\tstatic char buffer[PATH_MAX + 1];\n \tint len, postfix_len = strlen(DEFAULT_GIT_DIR_ENVIRONMENT) + 1;\n \n \t/* strip the variable 'dir' of the postfix \"/.git\" if it has it */\n \tlen = strlen(dir);\n \tif (len > postfix_len && !strcmp(dir + len - postfix_len,\n \t\t\t\t\"/\" DEFAULT_GIT_DIR_ENVIRONMENT)) {\n-\t\t\tstrncpy(dir_buffer, dir, len - postfix_len);\n+\t\t\tstrlcpy(dir_buffer, dir, len - postfix_len + 1);\n \n \t\t/* are we inside the default work tree? */\n \t\trel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);\n-- \n1.5.3.rc3.121.g7f37\n"}]}