{"thread":{"id":"9355","subject":"[PATCH] Handle the errors from chdir in set_work_tree","startedAt":"2007-08-02T15:27:43Z","lastAt":"2007-08-02T22:18:47Z","messageCount":6,"participants":["Alex Riesen","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"49435","messageId":"81b0412b0708020827p174515b7tc05fefde77f7d7c4@mail.gmail.com","threadId":"9355","inReplyTo":null,"subject":"[PATCH] Handle the errors from chdir in set_work_tree","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-08-02T15:27:43Z","receivedAt":"2007-08-02T15:27:43Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"These I haven't seen yet. Wouldn't like such a surprise though.\nAlso unstatic rel, it seems to be declared static accidentally.\n\nSigned-off-by: Alex Riesen \n---\n\nOne gets paranoid when exposed to \"commercial\"\nsystems for too long.\n\n setup.c | 10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\n\nFrom 602d49acd3e5285974cc1b7c4337301b13bb54b8 Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen \nDate: Thu, 2 Aug 2007 16:51:57 +0200\nSubject: [PATCH] Handle the errors from chdir in set_work_tree\n\nThese I haven't seen yet. Wouldn't like such a surprise though.\nAlso unstatic rel, it seems to be declared static accidentally.\n\nSigned-off-by: Alex Riesen \n---\n setup.c | 10 ++++++----\n 1 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 1beba7e..4e6bb76 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -201,8 +201,8 @@ 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@@ -220,8 +220,10 @@ const char *set_work_tree(const char *dir)\n \t\tif (!is_absolute_path(dir))\n \t\t\tset_git_dir(make_absolute_path(dir));\n \t\tdir = dir_buffer;\n-\t\tchdir(dir);\n-\t\tstrcat(rel, \"/\");\n+\t\tif (chdir(dir))\n+\t\t\trel = NULL;\n+\t\telse\n+\t\t\tstrcat(rel, \"/\");\n \t\tinside_git_dir = 0;\n \t} else {\n \t\trel = NULL;\n-- \n1.5.3.rc3.145.g4d9cdb\n\n"},{"id":"49438","messageId":"Pine.LNX.4.64.0708021640350.14781@racer.site","threadId":"9355","inReplyTo":"81b0412b0708020827p174515b7tc05fefde77f7d7c4@mail.gmail.com","subject":"Re: [PATCH] Handle the errors from chdir in set_work_tree","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-08-02T15:40:50Z","receivedAt":"2007-08-02T15:40:50Z","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> These I haven't seen yet. Wouldn't like such a surprise though.\n> Also unstatic rel, it seems to be declared static accidentally.\n> \n> Signed-off-by: Alex Riesen \n\nAcked-by: Johannes Schindelin \n\nCiao,\nDscho\n"},{"id":"49493","messageId":"7v8x8ttx7y.fsf@assigned-by-dhcp.cox.net","threadId":"9355","inReplyTo":"81b0412b0708020827p174515b7tc05fefde77f7d7c4@mail.gmail.com","subject":"Re: [PATCH] Handle the errors from chdir in set_work_tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-02T21:58:41Z","receivedAt":"2007-08-02T21:58:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n> These I haven't seen yet. Wouldn't like such a surprise though.\n> ...\n> @@ -220,8 +220,10 @@ const char *set_work_tree(const char *dir)\n> \t\tif (!is_absolute_path(dir))\n> \t\t\tset_git_dir(make_absolute_path(dir));\n> \t\tdir = dir_buffer;\n> -\t\tchdir(dir);\n> -\t\tstrcat(rel, \"/\");\n> +\t\tif (chdir(dir))\n> +\t\t\trel = NULL;\n> +\t\telse\n> +\t\t\tstrcat(rel, \"/\");\n> \t\tinside_git_dir = 0;\n> \t} else {\n> \t\trel = NULL;\n\nShouldn't it die() instead, though?\n\nConsolidating two of your patches, would this be Ok?\n\n-- >8 --\nFix work-tree related breakages\n\nIn set_work_tree(), variable rel needs to be reinitialized to\nNULL on every call (it should not be static).\n\nMake sure the incoming dir variable is not too long before\ncopying to the temporary buffer, and make sure chdir to the\nresulting directory succeeds.\n\n---\n\n setup.c | 22 ++++++++++++++--------\n 1 files changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 3653092..4945eb3 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -201,26 +201,32 @@ 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-\tint len, postfix_len = strlen(DEFAULT_GIT_DIR_ENVIRONMENT) + 1;\n+\tchar dir_buffer[PATH_MAX], *rel = NULL;\n+\tstatic char buffer[PATH_MAX + 1];\n+\tint len, suffix_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+\tif (len > suffix_len &&\n+\t !strcmp(dir + len - suffix_len, \"/\" DEFAULT_GIT_DIR_ENVIRONMENT)) {\n+\t\tif ((len - suffix_len) >= sizeof(dir_buffer))\n+\t\t\tdie(\"directory name too long\");\n+\t\tmemcpy(dir_buffer, dir, len - suffix_len);\n+\t\tdir_buffer[len - suffix_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 \t}\n+\n \t/* if rel is set, the cwd is _not_ the current working tree */\n \tif (rel && *rel) {\n \t\tif (!is_absolute_path(dir))\n \t\t\tset_git_dir(make_absolute_path(dir));\n \t\tdir = dir_buffer;\n-\t\tchdir(dir);\n-\t\tstrcat(rel, \"/\");\n+\t\tif (chdir(dir))\n+\t\t\tdie(\"cannot chdir to %s: %s\", dir, strerror(errno));\n+\t\telse\n+\t\t\tstrcat(rel, \"/\");\n \t\tinside_git_dir = 0;\n \t} else {\n \t\trel = NULL;\n"},{"id":"49497","messageId":"Pine.LNX.4.64.0708022306530.14781@racer.site","threadId":"9355","inReplyTo":"7v8x8ttx7y.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Handle the errors from chdir in set_work_tree","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-08-02T22:07:15Z","receivedAt":"2007-08-02T22:07:15Z","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> Fix work-tree related breakages\n> \n> In set_work_tree(), variable rel needs to be reinitialized to\n> NULL on every call (it should not be static).\n> \n> Make sure the incoming dir variable is not too long before\n> copying to the temporary buffer, and make sure chdir to the\n> resulting directory succeeds.\n\nACK.\n\n(Forget my patch, please)\n\nCiao,\nDscho\n"},{"id":"49500","messageId":"20070802221518.GC2829@steel.home","threadId":"9355","inReplyTo":"7v8x8ttx7y.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Handle the errors from chdir in set_work_tree","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-08-02T22:15:18Z","receivedAt":"2007-08-02T22:15:18Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Thu, Aug 02, 2007 23:58:41 +0200:\n> \"Alex Riesen\" writes:\n> > +\t\tif (chdir(dir))\n> > +\t\t\trel = NULL;\n...\n> \n> Shouldn't it die() instead, though?\n\nDunno. Don't like dying.\n\n> Consolidating two of your patches, would this be Ok?\n\nYes, but you may consider replacing strncpy with strlcpy:\n\n> +\t\tmemcpy(dir_buffer, dir, len - suffix_len);\n> +\t\tdir_buffer[len - suffix_len] = '\\0';\n\nstrlcpy(dir_buffer, dir, len - suffix_len + 1);\n"},{"id":"49501","messageId":"7v4pjhtwag.fsf@assigned-by-dhcp.cox.net","threadId":"9355","inReplyTo":"20070802221518.GC2829@steel.home","subject":"Re: [PATCH] Handle the errors from chdir in set_work_tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-02T22:18:47Z","receivedAt":"2007-08-02T22:18:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> Junio C Hamano, Thu, Aug 02, 2007 23:58:41 +0200:\n>> \"Alex Riesen\" writes:\n>> > +\t\tif (chdir(dir))\n>> > +\t\t\trel = NULL;\n> ...\n>> \n>> Shouldn't it die() instead, though?\n>\n> Dunno. Don't like dying.\n\nI do not understand your reasoning. Why is it better to use\nmysteriously truncated path, which may result in doing something\nthe user did not ask you to, rather than saying \"No, my\ntemporary buffer is not equipped to handle such an insanely long\npathname\"?\n\n>> Consolidating two of your patches, would this be Ok?\n>\n> Yes, but you may consider replacing strncpy with strlcpy:\n>\n>> +\t\tmemcpy(dir_buffer, dir, len - suffix_len);\n>> +\t\tdir_buffer[len - suffix_len] = '\\0';\n>\n> strlcpy(dir_buffer, dir, len - suffix_len + 1);\n\nDoes that buy us that much? Before going to that codepath, we\nhave made sure the result fits, haven't we?\n"}]}