{"thread":{"id":"36491","subject":"[PATCH] setup: Fix windows path buffer over-stepping","startedAt":"2014-04-24T13:06:09Z","lastAt":"2014-04-24T18:37:38Z","messageCount":2,"participants":["Martin Erik Werner","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"239564","messageId":"1398344769-32564-1-git-send-email-martinerikwerner@gmail.com","threadId":"36491","inReplyTo":null,"subject":"[PATCH] setup: Fix windows path buffer over-stepping","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2014-04-24T13:06:09Z","receivedAt":"2014-04-24T13:06:09Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"Fix a buffer over-stepping issue triggered by providing an absolute path\nthat is similar to the work tree path.\n\nabspath_part_inside_repo() may currently increment the path pointer by\noffset_1st_component() + wtlen, which is too much, since\noffset_1st_component() is a subset of wtlen.\n\nFor the *nix-style prefix '/', this does (by luck) not cause any issues,\nsince offset_1st_component() is 1 and there will always be a '/' or '\\0'\nthat can \"absorb\" this.\n\nIn the case of DOS-style prefixes though, the offset_1st_component() is\n3 and this can potentially over-step the string buffer. For example if\n\n    work_tree = \"c:/r\"\n    path      = \"c:/rl\"\n\nThen wtlen is 4, and incrementing the path pointer by (3 + 4) would\nend up 2 bytes outside a string buffer of length 6.\n\nSimilarly if\n\n    work_tree = \"c:/r\"\n    path      = \"c:/rl/d/a\"\n\nThen (since the loop starts by also incrementing the pointer one step),\nthis would mean that the function would miss checking if \"c:/rl/d\" could\nbe the work_tree, arguably this is unlikely though, since it would only\nbe possible with symlinks on windows.\n\nFix this by simply avoiding to increment by offset_1st_component() and\nwtlen at the same time.\n\nSigned-off-by: Martin Erik Werner <martinerikwerner@gmail.com>\n---\n\nThis is a follow-up on 655ee9e mw/symlinks which is currently merged into\nmaster, prospective for git v2.0.0, the issue only affects v2.0.0-rc0.\n\n setup.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 613e3b3..0a22f8b 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -29,7 +29,7 @@ static int abspath_part_inside_repo(char *path)\n \t\treturn -1;\n \twtlen = strlen(work_tree);\n \tlen = strlen(path);\n-\toff = 0;\n+\toff = offset_1st_component(path);\n \n \t/* check if work tree is already the prefix */\n \tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n@@ -45,7 +45,7 @@ static int abspath_part_inside_repo(char *path)\n \t\toff = wtlen;\n \t}\n \tpath0 = path;\n-\tpath += offset_1st_component(path) + off;\n+\tpath += off;\n \n \t/* check each '/'-terminated level */\n \twhile (*path) {\n-- \n1.9.2\n"},{"id":"239588","messageId":"xmqqsip2vaj1.fsf@gitster.dls.corp.google.com","threadId":"36491","inReplyTo":"1398344769-32564-1-git-send-email-martinerikwerner@gmail.com","subject":"Re: [PATCH] setup: Fix windows path buffer over-stepping","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-24T18:37:38Z","receivedAt":"2014-04-24T18:37:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Erik Werner <martinerikwerner@gmail.com> writes:\n\n> Fix a buffer over-stepping issue triggered by providing an absolute path\n> that is similar to the work tree path.\n>\n> abspath_part_inside_repo() may currently increment the path pointer by\n> offset_1st_component() + wtlen, which is too much, since\n> offset_1st_component() is a subset of wtlen.\n>\n> For the *nix-style prefix '/', this does (by luck) not cause any issues,\n> since offset_1st_component() is 1 and there will always be a '/' or '\\0'\n> that can \"absorb\" this.\n>\n> In the case of DOS-style prefixes though, the offset_1st_component() is\n> 3 and this can potentially over-step the string buffer. For example if\n>\n>     work_tree = \"c:/r\"\n>     path      = \"c:/rl\"\n>\n> Then wtlen is 4, and incrementing the path pointer by (3 + 4) would\n> end up 2 bytes outside a string buffer of length 6.\n>\n> Similarly if\n>\n>     work_tree = \"c:/r\"\n>     path      = \"c:/rl/d/a\"\n>\n> Then (since the loop starts by also incrementing the pointer one step),\n> this would mean that the function would miss checking if \"c:/rl/d\" could\n> be the work_tree, arguably this is unlikely though, since it would only\n> be possible with symlinks on windows.\n>\n> Fix this by simply avoiding to increment by offset_1st_component() and\n> wtlen at the same time.\n>\n> Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>\n> ---\n>\n> This is a follow-up on 655ee9e mw/symlinks which is currently merged into\n> master, prospective for git v2.0.0, the issue only affects v2.0.0-rc0.\n\nThanks for a fix and from a cursory read of the surrounding code, I\nthink the patch makes sense.\n\nI appreciate your doing so before the breakage hits a released\nversion very much.\n\n\n>  setup.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/setup.c b/setup.c\n> index 613e3b3..0a22f8b 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -29,7 +29,7 @@ static int abspath_part_inside_repo(char *path)\n>  \t\treturn -1;\n>  \twtlen = strlen(work_tree);\n>  \tlen = strlen(path);\n> -\toff = 0;\n> +\toff = offset_1st_component(path);\n>  \n>  \t/* check if work tree is already the prefix */\n>  \tif (wtlen <= len && !strncmp(path, work_tree, wtlen)) {\n> @@ -45,7 +45,7 @@ static int abspath_part_inside_repo(char *path)\n>  \t\toff = wtlen;\n>  \t}\n>  \tpath0 = path;\n> -\tpath += offset_1st_component(path) + off;\n> +\tpath += off;\n>  \n>  \t/* check each '/'-terminated level */\n>  \twhile (*path) {\n"}]}