{"thread":{"id":"9363","subject":"[PATCH] Fix unterminated string in set_work_tree and improve error handling","startedAt":"2007-08-03T05:47:24Z","lastAt":"2007-08-03T07:23:24Z","messageCount":3,"participants":["Alex Riesen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"49541","messageId":"81b0412b0708022247s1116f66al1e8751f33bcae581@mail.gmail.com","threadId":"9363","inReplyTo":null,"subject":"[PATCH] Fix unterminated string in set_work_tree and improve error handling","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-08-03T05:47:24Z","receivedAt":"2007-08-03T05:47:24Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Also, check if it fits in the temporary dir_buffer and can be chdir-ed into.\nDie for errors.\n\nSigned-off-by: Alex Riesen \n---\n setup.c | 12 ++++++++----\n 1 files changed, 8 insertions(+), 4 deletions(-)\n\nOn 8/3/07, Junio C Hamano wrote:\n> Alex Riesen writes:\n>\n> > Junio C Hamano, Thu, Aug 02, 2007 23:58:41 +0200:\n> >> \"Alex Riesen\" writes:\n> >> > + if (chdir(dir))\n> >> > + rel = NULL;\n> > ...\n> >>\n> >> Shouldn't it die() instead, though?\n> >\n> > Dunno. Don't like dying.\n>\n> I do not understand your reasoning. Why is it better to use\n> mysteriously truncated path, which may result in doing something\n> the user did not ask you to, rather than saying \"No, my\n> temporary buffer is not equipped to handle such an insanely long\n> pathname\"?\n\nAFAIU, it is not only a truncated path which is a problem for chdir,\nbut any failure to chdir, for any reason. And, if I understand\nset_work_tree returning NULL correctly (I assign rel NULL,\nwhich should be returned) - it is an error, and can be handled in\nthe caller. But...\nHmm... Looking at the code again, rel==NULL just means there\nis no prefix! You're right, better let it die.\n\n> >> Consolidating two of your patches, would this be Ok?\n> >\n> > Yes, but you may consider replacing strncpy with strlcpy:\n> >\n> >> + memcpy(dir_buffer, dir, len - suffix_len);\n> >> + dir_buffer[len - suffix_len] = '\\0';\n> >\n> > strlcpy(dir_buffer, dir, len - suffix_len + 1);\n>\n> Does that buy us that much? Before going to that codepath, we\n> have made sure the result fits, haven't we?\n\nNo, we haven't. The code just checks if the given work tree path\nis longer than \"/.git\", to be able to cut off that safely.\n\n\nFrom 7e55d4ed84e5867b217c9e0bc1964444f13eaef2 Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen \nDate: Fri, 3 Aug 2007 07:41:26 +0200\nSubject: [PATCH] Fix unterminated string in set_work_tree and improve error handling\n\n\nSigned-off-by: Alex Riesen \n---\n setup.c | 12 ++++++++----\n 1 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 3653092..6006fc5 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -201,15 +201,18 @@ 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\tif (len - postfix_len < sizeof(dir_buffer))\n+\t\t\tstrlcpy(dir_buffer, dir, len - postfix_len + 1);\n+\t\telse\n+\t\t\tdie(\"%s is too long\", dir);\n \n \t\t/* are we inside the default work tree? */\n \t\trel = get_relative_cwd(buffer, sizeof(buffer), dir_buffer);\n@@ -219,7 +222,8 @@ 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\tif (chdir(dir))\n+\t\t\tdie(\"cannot chdir to %s: %s\", dir, strerror(errno));\n \t\tstrcat(rel, \"/\");\n \t\tinside_git_dir = 0;\n \t} else {\n-- \n1.5.3.rc3.145.g4d9cdb\n\n"},{"id":"49547","messageId":"7v7iodqgg9.fsf@assigned-by-dhcp.cox.net","threadId":"9363","inReplyTo":"81b0412b0708022247s1116f66al1e8751f33bcae581@mail.gmail.com","subject":"Re: [PATCH] Fix unterminated string in set_work_tree and improve error handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-03T06:29:10Z","receivedAt":"2007-08-03T06:29:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Alex Riesen\" writes:\n\n>> >> Consolidating two of your patches, would this be Ok?\n>> >\n>> > Yes, but you may consider replacing strncpy with strlcpy:\n>> >\n>> >> + memcpy(dir_buffer, dir, len - suffix_len);\n>> >> + dir_buffer[len - suffix_len] = '\\0';\n>> >\n>> > strlcpy(dir_buffer, dir, len - suffix_len + 1);\n>>\n>> Does that buy us that much? Before going to that codepath, we\n>> have made sure the result fits, haven't we?\n\nI sense you haven't read my replacement patch (Dscho Acked it).\nI am talking about this part:\n\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"},{"id":"49556","messageId":"81b0412b0708030023m359ec79as3197deee310e87a5@mail.gmail.com","threadId":"9363","inReplyTo":"7v7iodqgg9.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Fix unterminated string in set_work_tree and improve error handling","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2007-08-03T07:23:24Z","receivedAt":"2007-08-03T07:23:24Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 8/3/07, Junio C Hamano wrote:\n> I sense you haven't read my replacement patch (Dscho Acked it).\n\nI did. I had to pay attention to what I read, though. Sorry.\n"}]}