{"thread":{"id":"35539","subject":"[PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer","startedAt":"2013-12-17T13:43:55Z","lastAt":"2013-12-20T09:30:48Z","messageCount":21,"participants":["Michael Haggerty","Stefan Beller","Duy Nguyen","Junio C Hamano","Antoine Pelisse","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"232101","messageId":"1387287838-25779-1-git-send-email-mhagger@alum.mit.edu","threadId":"35539","inReplyTo":null,"subject":"[PATCH 0/3] Fix two buffer overflows and remove a redundant var","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-12-17T13:43:55Z","receivedAt":"2013-12-17T13:43:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"These patches fix three things I found when I was puttering around.\nThe first two patches fix buffer overflows.  (They don't seem to be\nexploitable.)\n\nThe third removes a redundant local variable.\n\nThe patches apply to master.\n\nMichael Haggerty (3):\n  prune-packed: fix a possible buffer overflow\n  prune_object_dir(): verify that path fits in the temporary buffer\n  cmd_repack(): remove redundant local variable \"nr_packs\"\n\n builtin/prune-packed.c | 4 ++--\n builtin/prune.c        | 4 +++-\n builtin/repack.c       | 6 ++----\n 3 files changed, 7 insertions(+), 7 deletions(-)\n\n-- \n1.8.5.1\n"},{"id":"232098","messageId":"1387287838-25779-2-git-send-email-mhagger@alum.mit.edu","threadId":"35539","inReplyTo":"1387287838-25779-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/3] prune-packed: fix a possible buffer overflow","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-12-17T13:43:56Z","receivedAt":"2013-12-17T13:43:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The pathname character array might hold:\n\n    strlen(pathname) -- the pathname argument\n    '/'              -- a slash, if not already present in pathname\n    %02x/            -- the first two characters of the SHA-1 plus slash\n    38 characters    -- the last 38 characters of the SHA-1\n    NUL              -- terminator\n    ---------------------\n    strlen(pathname) + 43\n\n(Actually, the NUL character is not written explicitly to pathname;\nrather, the code relies on pathname being initialized to zeros and the\nzero following the pathname still being there after the other\ncharacters are written to the array.)\n\nBut the old pathname variable was PATH_MAX characters long, whereas\nthe check was (len > PATH_MAX - 42).  So there might have been one\nbyte too many stored in pathname.  This would have resulted in it's\nnot being NUL-terminated.\n\nSo, increase the size of the pathname variable by one byte to avoid\nthis possibility.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n builtin/prune-packed.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\nindex fa6ce42..81bc786 100644\n--- a/builtin/prune-packed.c\n+++ b/builtin/prune-packed.c\n@@ -37,7 +37,7 @@ static void prune_dir(int i, DIR *dir, char *pathname, int len, int opts)\n void prune_packed_objects(int opts)\n {\n \tint i;\n-\tstatic char pathname[PATH_MAX];\n+\tstatic char pathname[PATH_MAX + 1];\n \tconst char *dir = get_object_directory();\n \tint len = strlen(dir);\n \n@@ -45,7 +45,7 @@ void prune_packed_objects(int opts)\n \t\tprogress = start_progress_delay(\"Removing duplicate objects\",\n \t\t\t256, 95, 2);\n \n-\tif (len > PATH_MAX - 42)\n+\tif (len + 42 > PATH_MAX)\n \t\tdie(\"impossible object directory\");\n \tmemcpy(pathname, dir, len);\n \tif (len && pathname[len-1] != '/')\n-- \n1.8.5.1\n"},{"id":"232097","messageId":"1387287838-25779-3-git-send-email-mhagger@alum.mit.edu","threadId":"35539","inReplyTo":"1387287838-25779-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-12-17T13:43:57Z","receivedAt":"2013-12-17T13:43:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Dimension the buffer based on PATH_MAX rather than a magic number, and\nverify that the path fits in it before continuing.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n\nI don't think that this problem is remotely exploitable, because the\nsize of the string doesn't depend on inputs that can be influenced by\na client (at least not within Git).\n\n builtin/prune.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 6366917..ae34d04 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -96,7 +96,9 @@ static void prune_object_dir(const char *path)\n {\n \tint i;\n \tfor (i = 0; i < 256; i++) {\n-\t\tstatic char dir[4096];\n+\t\tstatic char dir[PATH_MAX + 1];\n+\t\tif (strlen(path) + 3 > PATH_MAX)\n+\t\t\tdie(\"impossible object directory\");\n \t\tsprintf(dir, \"%s/%02x\", path, i);\n \t\tprune_dir(i, dir);\n \t}\n-- \n1.8.5.1\n"},{"id":"232099","messageId":"1387287838-25779-4-git-send-email-mhagger@alum.mit.edu","threadId":"35539","inReplyTo":"1387287838-25779-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/3] cmd_repack(): remove redundant local variable \"nr_packs\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-12-17T13:43:58Z","receivedAt":"2013-12-17T13:43:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Its value is the same as the number of entries in the \"names\"\nstring_list, so just use \"names.nr\" in its place.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\nThis is just a trivial simplification.  Take it or leave it.\n\n builtin/repack.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a0ff5c7..91e2363 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -123,7 +123,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct string_list rollback = STRING_LIST_INIT_NODUP;\n \tstruct string_list existing_packs = STRING_LIST_INIT_DUP;\n \tstruct strbuf line = STRBUF_INIT;\n-\tint nr_packs, ext, ret, failed;\n+\tint ext, ret, failed;\n \tFILE *out;\n \n \t/* variables to be filled by option parsing */\n@@ -233,13 +233,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (ret)\n \t\treturn ret;\n \n-\tnr_packs = 0;\n \tout = xfdopen(cmd.out, \"r\");\n \twhile (strbuf_getline(&line, out, '\\n') != EOF) {\n \t\tif (line.len != 40)\n \t\t\tdie(\"repack: Expecting 40 character sha1 lines only from pack-objects.\");\n \t\tstring_list_append(&names, line.buf);\n-\t\tnr_packs++;\n \t}\n \tfclose(out);\n \tret = finish_command(&cmd);\n@@ -247,7 +245,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\treturn ret;\n \targv_array_clear(&cmd_args);\n \n-\tif (!nr_packs && !quiet)\n+\tif (!names.nr && !quiet)\n \t\tprintf(\"Nothing new to pack.\\n\");\n \n \t/*\n-- \n1.8.5.1\n"},{"id":"232100","messageId":"52B055CD.9040905@googlemail.com","threadId":"35539","inReplyTo":"1387287838-25779-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 3/3] cmd_repack(): remove redundant local variable \"nr_packs\"","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-12-17T13:46:53Z","receivedAt":"2013-12-17T13:46:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 17.12.2013 14:43, Michael Haggerty wrote:\n> Its value is the same as the number of entries in the \"names\"\n> string_list, so just use \"names.nr\" in its place.\n> \n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n\nAcked-by: Stefan Beller <stefanbeller@googlemail.com>\n\n> ---\n> This is just a trivial simplification.  Take it or leave it.\n> \n>  builtin/repack.c | 6 ++----\n>  1 file changed, 2 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index a0ff5c7..91e2363 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -123,7 +123,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tstruct string_list rollback = STRING_LIST_INIT_NODUP;\n>  \tstruct string_list existing_packs = STRING_LIST_INIT_DUP;\n>  \tstruct strbuf line = STRBUF_INIT;\n> -\tint nr_packs, ext, ret, failed;\n> +\tint ext, ret, failed;\n>  \tFILE *out;\n>  \n>  \t/* variables to be filled by option parsing */\n> @@ -233,13 +233,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tif (ret)\n>  \t\treturn ret;\n>  \n> -\tnr_packs = 0;\n>  \tout = xfdopen(cmd.out, \"r\");\n>  \twhile (strbuf_getline(&line, out, '\\n') != EOF) {\n>  \t\tif (line.len != 40)\n>  \t\t\tdie(\"repack: Expecting 40 character sha1 lines only from pack-objects.\");\n>  \t\tstring_list_append(&names, line.buf);\n> -\t\tnr_packs++;\n>  \t}\n>  \tfclose(out);\n>  \tret = finish_command(&cmd);\n> @@ -247,7 +245,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t\treturn ret;\n>  \targv_array_clear(&cmd_args);\n>  \n> -\tif (!nr_packs && !quiet)\n> +\tif (!names.nr && !quiet)\n>  \t\tprintf(\"Nothing new to pack.\\n\");\n>  \n>  \t/*\n> \n"},{"id":"232102","messageId":"CACsJy8DVc4soBrS1RbTLv93b5rere3htyL1DjRw=UcT4zVN9FA@mail.gmail.com","threadId":"35539","inReplyTo":"1387287838-25779-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-12-17T13:57:29Z","receivedAt":"2013-12-17T13:57:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Dec 17, 2013 at 8:43 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> The pathname character array might hold:\n>\n>     strlen(pathname) -- the pathname argument\n>     '/'              -- a slash, if not already present in pathname\n>     %02x/            -- the first two characters of the SHA-1 plus slash\n>     38 characters    -- the last 38 characters of the SHA-1\n>     NUL              -- terminator\n>     ---------------------\n>     strlen(pathname) + 43\n>\n> (Actually, the NUL character is not written explicitly to pathname;\n> rather, the code relies on pathname being initialized to zeros and the\n> zero following the pathname still being there after the other\n> characters are written to the array.)\n>\n> But the old pathname variable was PATH_MAX characters long, whereas\n> the check was (len > PATH_MAX - 42).  So there might have been one\n> byte too many stored in pathname.  This would have resulted in it's\n> not being NUL-terminated.\n>\n> So, increase the size of the pathname variable by one byte to avoid\n> this possibility.\n\nWhy don't we take this opportunity to replace that array with a\nstrbuf? The conversion looks simple with this function.\n\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>  builtin/prune-packed.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\n> index fa6ce42..81bc786 100644\n> --- a/builtin/prune-packed.c\n> +++ b/builtin/prune-packed.c\n> @@ -37,7 +37,7 @@ static void prune_dir(int i, DIR *dir, char *pathname, int len, int opts)\n>  void prune_packed_objects(int opts)\n>  {\n>         int i;\n> -       static char pathname[PATH_MAX];\n> +       static char pathname[PATH_MAX + 1];\n>         const char *dir = get_object_directory();\n>         int len = strlen(dir);\n>\n> @@ -45,7 +45,7 @@ void prune_packed_objects(int opts)\n>                 progress = start_progress_delay(\"Removing duplicate objects\",\n>                         256, 95, 2);\n>\n> -       if (len > PATH_MAX - 42)\n> +       if (len + 42 > PATH_MAX)\n>                 die(\"impossible object directory\");\n>         memcpy(pathname, dir, len);\n>         if (len && pathname[len-1] != '/')\n> --\n-- \nDuy\n"},{"id":"232110","messageId":"xmqqd2kvmi85.fsf@gitster.dls.corp.google.com","threadId":"35539","inReplyTo":"CACsJy8DVc4soBrS1RbTLv93b5rere3htyL1DjRw=UcT4zVN9FA@mail.gmail.com","subject":"Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-17T18:43:06Z","receivedAt":"2013-12-17T18:43:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n> Why don't we take this opportunity to replace that array with a\n> strbuf? The conversion looks simple with this function.\n\nIndeed.  Something like this, perhaps?\n\n builtin/prune-packed.c | 38 +++++++++++++++++++++-----------------\n 1 file changed, 21 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\nindex fa6ce42..fcf5fb6 100644\n--- a/builtin/prune-packed.c\n+++ b/builtin/prune-packed.c\n@@ -10,58 +10,62 @@ static const char * const prune_packed_usage[] = {\n \n static struct progress *progress;\n \n-static void prune_dir(int i, DIR *dir, char *pathname, int len, int opts)\n+static void prune_dir(int i, DIR *dir, struct strbuf *pathname, int opts)\n {\n \tstruct dirent *de;\n \tchar hex[40];\n+\tint top_len = pathname->len;\n \n \tsprintf(hex, \"%02x\", i);\n \twhile ((de = readdir(dir)) != NULL) {\n \t\tunsigned char sha1[20];\n \t\tif (strlen(de->d_name) != 38)\n \t\t\tcontinue;\n-\t\tmemcpy(hex+2, de->d_name, 38);\n+\t\tmemcpy(hex + 2, de->d_name, 38);\n \t\tif (get_sha1_hex(hex, sha1))\n \t\t\tcontinue;\n \t\tif (!has_sha1_pack(sha1))\n \t\t\tcontinue;\n-\t\tmemcpy(pathname + len, de->d_name, 38);\n+\n+\t\tstrbuf_add(pathname, de->d_name, 38);\n \t\tif (opts & PRUNE_PACKED_DRY_RUN)\n-\t\t\tprintf(\"rm -f %s\\n\", pathname);\n+\t\t\tprintf(\"rm -f %s\\n\", pathname->buf);\n \t\telse\n-\t\t\tunlink_or_warn(pathname);\n+\t\t\tunlink_or_warn(pathname->buf);\n \t\tdisplay_progress(progress, i + 1);\n+\t\tstrbuf_setlen(pathname, top_len);\n \t}\n }\n \n void prune_packed_objects(int opts)\n {\n \tint i;\n-\tstatic char pathname[PATH_MAX];\n \tconst char *dir = get_object_directory();\n-\tint len = strlen(dir);\n+\tstruct strbuf pathname = STRBUF_INIT;\n+\tint top_len;\n \n+\tstrbuf_addstr(&pathname, dir);\n \tif (opts & PRUNE_PACKED_VERBOSE)\n \t\tprogress = start_progress_delay(\"Removing duplicate objects\",\n \t\t\t256, 95, 2);\n \n-\tif (len > PATH_MAX - 42)\n-\t\tdie(\"impossible object directory\");\n-\tmemcpy(pathname, dir, len);\n-\tif (len && pathname[len-1] != '/')\n-\t\tpathname[len++] = '/';\n+\tif (pathname.len && pathname.buf[pathname.len - 1] != '/')\n+\t\tstrbuf_addch(&pathname, '/');\n+\n+\ttop_len = pathname.len;\n \tfor (i = 0; i < 256; i++) {\n \t\tDIR *d;\n \n \t\tdisplay_progress(progress, i + 1);\n-\t\tsprintf(pathname + len, \"%02x/\", i);\n-\t\td = opendir(pathname);\n+\t\tstrbuf_setlen(&pathname, top_len);\n+\t\tstrbuf_addf(&pathname, \"%02x/\", i);\n+\t\td = opendir(pathname.buf);\n \t\tif (!d)\n \t\t\tcontinue;\n-\t\tprune_dir(i, d, pathname, len + 3, opts);\n+\t\tprune_dir(i, d, &pathname, opts);\n \t\tclosedir(d);\n-\t\tpathname[len + 2] = '\\0';\n-\t\trmdir(pathname);\n+\t\tstrbuf_setlen(&pathname, top_len + 2);\n+\t\trmdir(pathname.buf);\n \t}\n \tstop_progress(&progress);\n }\n"},{"id":"232113","messageId":"xmqq8uvjmhu5.fsf@gitster.dls.corp.google.com","threadId":"35539","inReplyTo":"1387287838-25779-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-17T18:51:30Z","receivedAt":"2013-12-17T18:51:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> Dimension the buffer based on PATH_MAX rather than a magic number, and\n> verify that the path fits in it before continuing.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>\n> I don't think that this problem is remotely exploitable, because the\n> size of the string doesn't depend on inputs that can be influenced by\n> a client (at least not within Git).\n\nThis is shrinking the buffer on some platforms where PATH_MAX is\nlower than 4k---granted, we will die() with the new check instead of\ncrashing uncontrolled, but it still feels somewhat wrong.\n\n>  builtin/prune.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index 6366917..ae34d04 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -96,7 +96,9 @@ static void prune_object_dir(const char *path)\n>  {\n>  \tint i;\n>  \tfor (i = 0; i < 256; i++) {\n> -\t\tstatic char dir[4096];\n> +\t\tstatic char dir[PATH_MAX + 1];\n> +\t\tif (strlen(path) + 3 > PATH_MAX)\n> +\t\t\tdie(\"impossible object directory\");\n>  \t\tsprintf(dir, \"%s/%02x\", path, i);\n>  \t\tprune_dir(i, dir);\n>  \t}\n"},{"id":"232114","messageId":"CALWbr2xmBXHaxb89665ub_cS8mFxf0C6tJnhBsdnUnX7DoCPfw@mail.gmail.com","threadId":"35539","inReplyTo":"1387287838-25779-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-12-17T18:56:59Z","receivedAt":"2013-12-17T18:56:59Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"On Tue, Dec 17, 2013 at 2:43 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> Dimension the buffer based on PATH_MAX rather than a magic number, and\n> verify that the path fits in it before continuing.\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> ---\n>\n> I don't think that this problem is remotely exploitable, because the\n> size of the string doesn't depend on inputs that can be influenced by\n> a client (at least not within Git).\n>\n>  builtin/prune.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index 6366917..ae34d04 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -96,7 +96,9 @@ static void prune_object_dir(const char *path)\n>  {\n>         int i;\n>         for (i = 0; i < 256; i++) {\n> -               static char dir[4096];\n> +               static char dir[PATH_MAX + 1];\n> +               if (strlen(path) + 3 > PATH_MAX)\n> +                       die(\"impossible object directory\");\n>                 sprintf(dir, \"%s/%02x\", path, i);\n>                 prune_dir(i, dir);\n>         }\n> --\n> 1.8.5.1\n\nObviously correct,\n\nThanks,\nAntoine,\n"},{"id":"232124","messageId":"20131217232231.GA14807@sigill.intra.peff.net","threadId":"35539","inReplyTo":"xmqq8uvjmhu5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-17T23:22:31Z","receivedAt":"2013-12-17T23:22:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 17, 2013 at 10:51:30AM -0800, Junio C Hamano wrote:\n\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n> > Dimension the buffer based on PATH_MAX rather than a magic number, and\n> > verify that the path fits in it before continuing.\n> >\n> > Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n> > ---\n> >\n> > I don't think that this problem is remotely exploitable, because the\n> > size of the string doesn't depend on inputs that can be influenced by\n> > a client (at least not within Git).\n> \n> This is shrinking the buffer on some platforms where PATH_MAX is\n> lower than 4k---granted, we will die() with the new check instead of\n> crashing uncontrolled, but it still feels somewhat wrong.\n\nOn such a system, though, does the resulting prune_dir call actually do\nanything? We will feed the result to opendir(), which I would think\nwould choke on the long name.\n\nStill, it is probably better to do everything we can and let the OS\nchoke (especially because we would want to continue operating on other\npaths in this case).\n\nConverting it to use strbuf looks like it will actually let us drop a\nbunch of copying, too, as we just end up in mkpath at the very lowest\nlevel. I.e., something like below.\n\nAs an aside, I have noticed us using this \"push/pop\" approach to treating a\nstrbuf as a stack of paths elsewhere, too. I.e:\n\n  size_t baselen = base->len;\n  strbuf_addf(base, \"/%s\", some_thing);\n  some_call(base);\n  base->len = baselen;\n\nI wondered if there was any kind of helper we could add to make it look\nnicer. But I don't think so; the hairy part is that you must remember to\nreset base->len after the call, and there is no easy way around that in\nC. If you had object destructors that ran as the stack unwound, or\ndynamic scoping, it would be easy to manipulate the object. Wrapping\nwon't work because strbuf isn't just a length wrapping an immutable\nbuffer; it actually has to move the NUL in the buffer.\n\nAnyway, not important, but perhaps somebody is more clever than I am.\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 6366917..4ca8ec1 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -17,9 +17,8 @@ static int verbose;\n static unsigned long expire;\n static int show_progress = -1;\n \n-static int prune_tmp_object(const char *path, const char *filename)\n+static int prune_tmp_object(const char *fullpath)\n {\n-\tconst char *fullpath = mkpath(\"%s/%s\", path, filename);\n \tstruct stat st;\n \tif (lstat(fullpath, &st))\n \t\treturn error(\"Could not stat '%s'\", fullpath);\n@@ -32,9 +31,8 @@ static int prune_tmp_object(const char *path, const char *filename)\n \treturn 0;\n }\n \n-static int prune_object(char *path, const char *filename, const unsigned char *sha1)\n+static int prune_object(const char *fullpath, const unsigned char *sha1)\n {\n-\tconst char *fullpath = mkpath(\"%s/%s\", path, filename);\n \tstruct stat st;\n \tif (lstat(fullpath, &st))\n \t\treturn error(\"Could not stat '%s'\", fullpath);\n@@ -50,9 +48,10 @@ static int prune_object(char *path, const char *filename, const unsigned char *s\n \treturn 0;\n }\n \n-static int prune_dir(int i, char *path)\n+static int prune_dir(int i, struct strbuf *path)\n {\n-\tDIR *dir = opendir(path);\n+\tsize_t baselen = path->len;\n+\tDIR *dir = opendir(path->buf);\n \tstruct dirent *de;\n \n \tif (!dir)\n@@ -77,28 +76,39 @@ static int prune_dir(int i, char *path)\n \t\t\tif (lookup_object(sha1))\n \t\t\t\tcontinue;\n \n-\t\t\tprune_object(path, de->d_name, sha1);\n+\t\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n+\t\t\tprune_object(path->buf, sha1);\n+\t\t\tpath->len = baselen;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!prefixcmp(de->d_name, \"tmp_obj_\")) {\n-\t\t\tprune_tmp_object(path, de->d_name);\n+\t\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n+\t\t\tprune_tmp_object(path->buf);\n+\t\t\tpath->len = baselen;\n \t\t\tcontinue;\n \t\t}\n-\t\tfprintf(stderr, \"bad sha1 file: %s/%s\\n\", path, de->d_name);\n+\t\tfprintf(stderr, \"bad sha1 file: %s/%s\\n\", path->buf, de->d_name);\n \t}\n \tclosedir(dir);\n \tif (!show_only)\n-\t\trmdir(path);\n+\t\trmdir(path->buf);\n \treturn 0;\n }\n \n static void prune_object_dir(const char *path)\n {\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tsize_t baselen;\n \tint i;\n+\n+\tstrbuf_addstr(&buf, path);\n+\tstrbuf_addch(&buf, '/');\n+\tbaselen = buf.len;\n+\n \tfor (i = 0; i < 256; i++) {\n-\t\tstatic char dir[4096];\n-\t\tsprintf(dir, \"%s/%02x\", path, i);\n-\t\tprune_dir(i, dir);\n+\t\tstrbuf_addf(&buf, \"%02x\", i);\n+\t\tprune_dir(i, &buf);\n+\t\tbuf.len = baselen;\n \t}\n }\n \n@@ -120,7 +130,7 @@ static void remove_temporary_files(const char *path)\n \t}\n \twhile ((de = readdir(dir)) != NULL)\n \t\tif (!prefixcmp(de->d_name, \"tmp_\"))\n-\t\t\tprune_tmp_object(path, de->d_name);\n+\t\t\tprune_tmp_object(mkpath(\"%s/%s\", path, de->d_name));\n \tclosedir(dir);\n }\n \n"},{"id":"232186","messageId":"xmqqa9fyhrzt.fsf@gitster.dls.corp.google.com","threadId":"35539","inReplyTo":"20131217232231.GA14807@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-18T19:35:34Z","receivedAt":"2013-12-18T19:35:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Converting it to use strbuf looks like it will actually let us drop a\n> bunch of copying, too, as we just end up in mkpath at the very lowest\n> level. I.e., something like below.\n\nThanks; I may have a few minor comments, but overall, I like the\nplacement of mkpath() in the resulting callchain a lot better than\nthe original.\n\n> As an aside, I have noticed us using this \"push/pop\" approach to treating a\n> strbuf as a stack of paths elsewhere, too. I.e:\n>\n>   size_t baselen = base->len;\n>   strbuf_addf(base, \"/%s\", some_thing);\n>   some_call(base);\n>   base->len = baselen;\n>\n> I wondered if there was any kind of helper we could add to make it look\n> nicer. But I don't think so; the hairy part is that you must remember to\n> reset base->len after the call, and there is no easy way around that in\n> C. If you had object destructors that ran as the stack unwound, or\n> dynamic scoping, it would be easy to manipulate the object. Wrapping\n> won't work because strbuf isn't just a length wrapping an immutable\n> buffer; it actually has to move the NUL in the buffer.\n>\n> Anyway, not important, but perhaps somebody is more clever than I am.\n\nHmph... interesting but we would need a lot more thought than the\ntime necessary to respond to one piece of e-mail for this ;-)\nPerhaps later...\n\n> diff --git a/builtin/prune.c b/builtin/prune.c\n> index 6366917..4ca8ec1 100644\n> --- a/builtin/prune.c\n> +++ b/builtin/prune.c\n> @@ -17,9 +17,8 @@ static int verbose;\n>  static unsigned long expire;\n>  static int show_progress = -1;\n>  \n> -static int prune_tmp_object(const char *path, const char *filename)\n> +static int prune_tmp_object(const char *fullpath)\n>  {\n> -\tconst char *fullpath = mkpath(\"%s/%s\", path, filename);\n>  \tstruct stat st;\n>  \tif (lstat(fullpath, &st))\n>  \t\treturn error(\"Could not stat '%s'\", fullpath);\n\nThis function is called to remove\n\n * Any tmp_* found directly in .git/objects/\n * Any tmp_* found directly in .git/objects/pack/\n * Any tmp_obj_* found directly in .git/objects/??/\n\nand shares the same expiration logic with prune_object().  The only\ndifference from the other function is what the file is called in\ndry-run or verbose report (\"stale temporary file\" vs \"<sha-1> <typename>\").\n\nWe may want to rename it to prune_tmp_file(); its usage may have\nbeen limited to an unborn loose object file at some point in the\nhistory, but it does not look that way in today's code.\n\n> -static int prune_dir(int i, char *path)\n> +static int prune_dir(int i, struct strbuf *path)\n>  {\n> -\tDIR *dir = opendir(path);\n> +\tsize_t baselen = path->len;\n> +\tDIR *dir = opendir(path->buf);\n>  \tstruct dirent *de;\n>  \n>  \tif (!dir)\n> @@ -77,28 +76,39 @@ static int prune_dir(int i, char *path)\n>  \t\t\tif (lookup_object(sha1))\n>  \t\t\t\tcontinue;\n>  \n> -\t\t\tprune_object(path, de->d_name, sha1);\n> +\t\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n> +\t\t\tprune_object(path->buf, sha1);\n> +\t\t\tpath->len = baselen;\n\nThis is minor, but I prefer using strbuf_setlen() for this.\n\nThanks.\n"},{"id":"232190","messageId":"20131218200043.GA10244@sigill.intra.peff.net","threadId":"35539","inReplyTo":"xmqqa9fyhrzt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-18T20:00:43Z","receivedAt":"2013-12-18T20:00:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 18, 2013 at 11:35:34AM -0800, Junio C Hamano wrote:\n\n> This function is called to remove\n> \n>  * Any tmp_* found directly in .git/objects/\n>  * Any tmp_* found directly in .git/objects/pack/\n>  * Any tmp_obj_* found directly in .git/objects/??/\n> \n> and shares the same expiration logic with prune_object().  The only\n> difference from the other function is what the file is called in\n> dry-run or verbose report (\"stale temporary file\" vs \"<sha-1> <typename>\").\n> \n> We may want to rename it to prune_tmp_file(); its usage may have\n> been limited to an unborn loose object file at some point in the\n> history, but it does not look that way in today's code.\n\nYes, certainly the rename makes sense (and I think the history is as you\nmentioned). I noticed the similarity between the two, as well. It might\nbe simple to refactor into a single function.\n\n> > -\t\t\tprune_object(path, de->d_name, sha1);\n> > +\t\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n> > +\t\t\tprune_object(path->buf, sha1);\n> > +\t\t\tpath->len = baselen;\n> \n> This is minor, but I prefer using strbuf_setlen() for this.\n\nGood catch. I do not think it is minor at all; my version is buggy.\nAfter the loop ends, path->len does not match the NUL in path->buf. That\nis OK if the next caller is strbuf-aware, but if it were to pass\npath->buf straight to a system call, that would be rather...confusing.\n\n-Peff\n"},{"id":"232192","messageId":"xmqqwqj1hqjd.fsf@gitster.dls.corp.google.com","threadId":"35539","inReplyTo":"20131218200043.GA10244@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-18T20:07:02Z","receivedAt":"2013-12-18T20:07:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> > +\t\t\tprune_object(path->buf, sha1);\n>> > +\t\t\tpath->len = baselen;\n>> \n>> This is minor, but I prefer using strbuf_setlen() for this.\n>\n> Good catch. I do not think it is minor at all; my version is buggy.\n> After the loop ends, path->len does not match the NUL in path->buf. That\n> is OK if the next caller is strbuf-aware, but if it were to pass\n> path->buf straight to a system call, that would be rather...confusing.\n\nHmph, rmdir(path->buf) at the end of prune_dir() may have that exact\nissue.\n\nWill squash in the following.\n\n builtin/prune.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 4ca8ec1..99f3f35 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -17,7 +17,7 @@ static int verbose;\n static unsigned long expire;\n static int show_progress = -1;\n \n-static int prune_tmp_object(const char *fullpath)\n+static int prune_tmp_file(const char *fullpath)\n {\n \tstruct stat st;\n \tif (lstat(fullpath, &st))\n@@ -78,13 +78,13 @@ static int prune_dir(int i, struct strbuf *path)\n \n \t\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n \t\t\tprune_object(path->buf, sha1);\n-\t\t\tpath->len = baselen;\n+\t\t\tstrbuf_setlen(&path, baselen);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!prefixcmp(de->d_name, \"tmp_obj_\")) {\n \t\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n-\t\t\tprune_tmp_object(path->buf);\n-\t\t\tpath->len = baselen;\n+\t\t\tprune_tmp_file(path->buf);\n+\t\t\tstrbuf_setlen(&path, baselen);\n \t\t\tcontinue;\n \t\t}\n \t\tfprintf(stderr, \"bad sha1 file: %s/%s\\n\", path->buf, de->d_name);\n@@ -108,7 +108,7 @@ static void prune_object_dir(const char *path)\n \tfor (i = 0; i < 256; i++) {\n \t\tstrbuf_addf(&buf, \"%02x\", i);\n \t\tprune_dir(i, &buf);\n-\t\tbuf.len = baselen;\n+\t\tstrbuf_setlen(&buf, baselen);\n \t}\n }\n \n@@ -130,7 +130,7 @@ static void remove_temporary_files(const char *path)\n \t}\n \twhile ((de = readdir(dir)) != NULL)\n \t\tif (!prefixcmp(de->d_name, \"tmp_\"))\n-\t\t\tprune_tmp_object(mkpath(\"%s/%s\", path, de->d_name));\n+\t\t\tprune_tmp_file(mkpath(\"%s/%s\", path, de->d_name));\n \tclosedir(dir);\n }\n \n"},{"id":"232193","messageId":"20131218201101.GA10507@sigill.intra.peff.net","threadId":"35539","inReplyTo":"xmqqwqj1hqjd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-18T20:11:01Z","receivedAt":"2013-12-18T20:11:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 18, 2013 at 12:07:02PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> > +\t\t\tprune_object(path->buf, sha1);\n> >> > +\t\t\tpath->len = baselen;\n> >> \n> >> This is minor, but I prefer using strbuf_setlen() for this.\n> >\n> > Good catch. I do not think it is minor at all; my version is buggy.\n> > After the loop ends, path->len does not match the NUL in path->buf. That\n> > is OK if the next caller is strbuf-aware, but if it were to pass\n> > path->buf straight to a system call, that would be rather...confusing.\n> \n> Hmph, rmdir(path->buf) at the end of prune_dir() may have that exact\n> issue.\n> \n> Will squash in the following.\n\nThanks. Are you picking this up with a commit message, or did you want\nme to re-send with the usual message/signoff?\n\n-Peff\n"},{"id":"232194","messageId":"xmqqppothq5k.fsf@gitster.dls.corp.google.com","threadId":"35539","inReplyTo":"20131218201101.GA10507@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-18T20:15:19Z","receivedAt":"2013-12-18T20:15:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Dec 18, 2013 at 12:07:02PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> >> > +\t\t\tprune_object(path->buf, sha1);\n>> >> > +\t\t\tpath->len = baselen;\n>> >> \n>> >> This is minor, but I prefer using strbuf_setlen() for this.\n>> >\n>> > Good catch. I do not think it is minor at all; my version is buggy.\n>> > After the loop ends, path->len does not match the NUL in path->buf. That\n>> > is OK if the next caller is strbuf-aware, but if it were to pass\n>> > path->buf straight to a system call, that would be rather...confusing.\n>> \n>> Hmph, rmdir(path->buf) at the end of prune_dir() may have that exact\n>> issue.\n>> \n>> Will squash in the following.\n>\n> Thanks. Are you picking this up with a commit message, or did you want\n> me to re-send with the usual message/signoff?\n\nI think this should be sufficient ;-)\n\n-- >8 --\nFrom: Jeff King <peff@peff.net>\nDate: Tue, 17 Dec 2013 18:22:31 -0500\nSubject: [PATCH] builtin/prune.c: use strbuf to avoid having to worry about PATH_MAX\n\nWhile at it, rename prune_tmp_object(), which used to be a helper to\nremove temporary files that were created to become loose object\nfiles, to prune_tmp_file(), as the function is also used to remove\nany random cruft whose name begins with tmp_ directly in .git/object\nor .git/object/pack directories these days.\n\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/prune.c | 38 ++++++++++++++++++++++++--------------\n 1 file changed, 24 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 6366917..99f3f35 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -17,9 +17,8 @@ static int verbose;\n static unsigned long expire;\n static int show_progress = -1;\n \n-static int prune_tmp_object(const char *path, const char *filename)\n+static int prune_tmp_file(const char *fullpath)\n {\n-\tconst char *fullpath = mkpath(\"%s/%s\", path, filename);\n \tstruct stat st;\n \tif (lstat(fullpath, &st))\n \t\treturn error(\"Could not stat '%s'\", fullpath);\n@@ -32,9 +31,8 @@ static int prune_tmp_object(const char *path, const char *filename)\n \treturn 0;\n }\n \n-static int prune_object(char *path, const char *filename, const unsigned char *sha1)\n+static int prune_object(const char *fullpath, const unsigned char *sha1)\n {\n-\tconst char *fullpath = mkpath(\"%s/%s\", path, filename);\n \tstruct stat st;\n \tif (lstat(fullpath, &st))\n \t\treturn error(\"Could not stat '%s'\", fullpath);\n@@ -50,9 +48,10 @@ static int prune_object(char *path, const char *filename, const unsigned char *s\n \treturn 0;\n }\n \n-static int prune_dir(int i, char *path)\n+static int prune_dir(int i, struct strbuf *path)\n {\n-\tDIR *dir = opendir(path);\n+\tsize_t baselen = path->len;\n+\tDIR *dir = opendir(path->buf);\n \tstruct dirent *de;\n \n \tif (!dir)\n@@ -77,28 +76,39 @@ static int prune_dir(int i, char *path)\n \t\t\tif (lookup_object(sha1))\n \t\t\t\tcontinue;\n \n-\t\t\tprune_object(path, de->d_name, sha1);\n+\t\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n+\t\t\tprune_object(path->buf, sha1);\n+\t\t\tstrbuf_setlen(&path, baselen);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!prefixcmp(de->d_name, \"tmp_obj_\")) {\n-\t\t\tprune_tmp_object(path, de->d_name);\n+\t\t\tstrbuf_addf(path, \"/%s\", de->d_name);\n+\t\t\tprune_tmp_file(path->buf);\n+\t\t\tstrbuf_setlen(&path, baselen);\n \t\t\tcontinue;\n \t\t}\n-\t\tfprintf(stderr, \"bad sha1 file: %s/%s\\n\", path, de->d_name);\n+\t\tfprintf(stderr, \"bad sha1 file: %s/%s\\n\", path->buf, de->d_name);\n \t}\n \tclosedir(dir);\n \tif (!show_only)\n-\t\trmdir(path);\n+\t\trmdir(path->buf);\n \treturn 0;\n }\n \n static void prune_object_dir(const char *path)\n {\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tsize_t baselen;\n \tint i;\n+\n+\tstrbuf_addstr(&buf, path);\n+\tstrbuf_addch(&buf, '/');\n+\tbaselen = buf.len;\n+\n \tfor (i = 0; i < 256; i++) {\n-\t\tstatic char dir[4096];\n-\t\tsprintf(dir, \"%s/%02x\", path, i);\n-\t\tprune_dir(i, dir);\n+\t\tstrbuf_addf(&buf, \"%02x\", i);\n+\t\tprune_dir(i, &buf);\n+\t\tstrbuf_setlen(&buf, baselen);\n \t}\n }\n \n@@ -120,7 +130,7 @@ static void remove_temporary_files(const char *path)\n \t}\n \twhile ((de = readdir(dir)) != NULL)\n \t\tif (!prefixcmp(de->d_name, \"tmp_\"))\n-\t\t\tprune_tmp_object(path, de->d_name);\n+\t\t\tprune_tmp_file(mkpath(\"%s/%s\", path, de->d_name));\n \tclosedir(dir);\n }\n \n-- \n1.8.5.2-297-g3e57c29\n"},{"id":"232197","messageId":"20131218202722.GA10774@sigill.intra.peff.net","threadId":"35539","inReplyTo":"xmqqppothq5k.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-18T20:27:22Z","receivedAt":"2013-12-18T20:27:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 18, 2013 at 12:15:19PM -0800, Junio C Hamano wrote:\n\n> > Thanks. Are you picking this up with a commit message, or did you want\n> > me to re-send with the usual message/signoff?\n> \n> I think this should be sufficient ;-)\n> \n> -- >8 --\n> From: Jeff King <peff@peff.net>\n> Date: Tue, 17 Dec 2013 18:22:31 -0500\n> Subject: [PATCH] builtin/prune.c: use strbuf to avoid having to worry about PATH_MAX\n> \n> While at it, rename prune_tmp_object(), which used to be a helper to\n> remove temporary files that were created to become loose object\n> files, to prune_tmp_file(), as the function is also used to remove\n> any random cruft whose name begins with tmp_ directly in .git/object\n> or .git/object/pack directories these days.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nGreat, thanks. You might also want to stick a:\n\n Noticed-by: Michael Haggerty <mhagger@alum.mit.edu>\n\nin there.\n\n-Peff\n"},{"id":"232211","messageId":"52B2256A.3060802@alum.mit.edu","threadId":"35539","inReplyTo":"xmqqd2kvmi85.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-12-18T22:44:58Z","receivedAt":"2013-12-18T22:44:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/17/2013 07:43 PM, Junio C Hamano wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n> \n>> Why don't we take this opportunity to replace that array with a\n>> strbuf? The conversion looks simple with this function.\n> \n> Indeed.  Something like this, perhaps?\n> [...]\n\nFrankly, with my initial patches I was just trying to paper over the bug\nwith the smallest possible change.  It's nice that people are attempting\nbigger improvements.\n\nI went in a slightly different direction: I am spiking out an API for\niterating over loose object files.  It would be useful in a couple of\nplaces.\n\n[While doing so, I got sidetracked by the question: what happens if a\nprune process deletes the \"objects/XX\" directory just the same moment\nthat another process is trying to write an object into that directory?\nI think the relevant function is sha1_file.c:create_tmpfile().  It looks\nlike there is a nonzero but very small race window that could result in\na spurious \"unable to create temporary file\" error, but even then I\ndon't think there would be any corruption or anything.]\n\nBut don't let me stop you; the cleanups you are working on are\ndefinitely nice and are complementary to my ideas.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"232215","messageId":"20131219000415.GA17420@sigill.intra.peff.net","threadId":"35539","inReplyTo":"52B2256A.3060802@alum.mit.edu","subject":"Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-19T00:04:15Z","receivedAt":"2013-12-19T00:04:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 18, 2013 at 11:44:58PM +0100, Michael Haggerty wrote:\n\n> [While doing so, I got sidetracked by the question: what happens if a\n> prune process deletes the \"objects/XX\" directory just the same moment\n> that another process is trying to write an object into that directory?\n> I think the relevant function is sha1_file.c:create_tmpfile().  It looks\n> like there is a nonzero but very small race window that could result in\n> a spurious \"unable to create temporary file\" error, but even then I\n> don't think there would be any corruption or anything.]\n\nThere's a race there, but I think it's hard to trigger.\n\nOur strategy with object creation is to call open, recognize ENOENT,\nmkdir, and then try again. If the rmdir happens before our call to open,\nthen we're fine. If open happens first, then the rmdir will fail.\n\nBut we don't loop on ENOENT. So if the rmdir happens in the middle,\nafter the mkdir but before we call open again, we'd fail, because we\ndon't treat ENOENT specially in the second call to open. That is\nunlikely to happen, though, as prune would not be removing a directory\nit did not just enter and clean up an object from (in which case we\nwould not have gotten the first ENOENT in the creator). I think you'd So\nyou'd have to have something creating and then pruning the directory in\nthe time between our open and mkdir. It would probably be more likely to\nsee it if you had two prunes running (the first one kills the directory,\ncreator notices and calls mkdir, then the second prune kills the\ndirectory, too).\n\nSo it seems unlikely and the worst case is a temporary failure, not a\ncorruption. It's probably not worth caring too much about, but we could\nsolve it pretty easily by looping on ENOENT on creation.\n\nOn a similar note, I imagine that a simultaneous \"branch foo/bar\" and\n\"branch -d foo/baz\" could race over the creation/deletion of\n\"refs/heads/foo\", but I didn't look into it.\n\n-Peff\n"},{"id":"232221","messageId":"CACsJy8CxOAY7MSGx45W60i0ETj4mrgh4rVbVBKrBrLck4bahEg@mail.gmail.com","threadId":"35539","inReplyTo":"xmqqd2kvmi85.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-12-19T00:37:40Z","receivedAt":"2013-12-19T00:37:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Dec 18, 2013 at 1:43 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>> Why don't we take this opportunity to replace that array with a\n>> strbuf? The conversion looks simple with this function.\n>\n> Indeed.  Something like this, perhaps?\n\nYes, looking good.\n\n>  void prune_packed_objects(int opts)\n>  {\n>         int i;\n> -       static char pathname[PATH_MAX];\n>         const char *dir = get_object_directory();\n> -       int len = strlen(dir);\n> +       struct strbuf pathname = STRBUF_INIT;\n> +       int top_len;\n>\n> +       strbuf_addstr(&pathname, dir);\n>         if (opts & PRUNE_PACKED_VERBOSE)\n>                 progress = start_progress_delay(\"Removing duplicate objects\",\n>                         256, 95, 2);\n>\n> -       if (len > PATH_MAX - 42)\n> -               die(\"impossible object directory\");\n> -       memcpy(pathname, dir, len);\n> -       if (len && pathname[len-1] != '/')\n> -               pathname[len++] = '/';\n> +       if (pathname.len && pathname.buf[pathname.len - 1] != '/')\n> +               strbuf_addch(&pathname, '/');\n\nI see this pattern (add a trailing slash) in a few places too. Maybe\nwe could make a wrapper for it.\n-- \nDuy\n"},{"id":"232239","messageId":"52B31FF3.1060102@alum.mit.edu","threadId":"35539","inReplyTo":"20131219000415.GA17420@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-12-19T16:33:55Z","receivedAt":"2013-12-19T16:33:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/19/2013 01:04 AM, Jeff King wrote:\n> On Wed, Dec 18, 2013 at 11:44:58PM +0100, Michael Haggerty wrote:\n> \n>> [While doing so, I got sidetracked by the question: what happens if a\n>> prune process deletes the \"objects/XX\" directory just the same moment\n>> that another process is trying to write an object into that directory?\n>> I think the relevant function is sha1_file.c:create_tmpfile().  It looks\n>> like there is a nonzero but very small race window that could result in\n>> a spurious \"unable to create temporary file\" error, but even then I\n>> don't think there would be any corruption or anything.]\n> \n> There's a race there, but I think it's hard to trigger.\n> \n> Our strategy with object creation is to call open, recognize ENOENT,\n> mkdir, and then try again. If the rmdir happens before our call to open,\n> then we're fine. If open happens first, then the rmdir will fail.\n> \n> But we don't loop on ENOENT. So if the rmdir happens in the middle,\n> after the mkdir but before we call open again, we'd fail, because we\n> don't treat ENOENT specially in the second call to open. That is\n> unlikely to happen, though, as prune would not be removing a directory\n> it did not just enter and clean up an object from (in which case we\n> would not have gotten the first ENOENT in the creator). [...]\n\nThe way I read it, prune tries to delete the directory whether or not\nthere were any files in it.  So the race could be triggered by a single\nwriter that wants to write an object to a not-yet-existent shard\ndirectory and a single prune process that encounters the directory\nbetween when it is created and when the object file is added.\n\nBut that doesn't mean I disagree with your conclusion:\n\n> So it seems unlikely and the worst case is a temporary failure, not a\n> corruption. It's probably not worth caring too much about, but we could\n> solve it pretty easily by looping on ENOENT on creation.\n\nRegarding references:\n\n> On a similar note, I imagine that a simultaneous \"branch foo/bar\" and\n> \"branch -d foo/baz\" could race over the creation/deletion of\n> \"refs/heads/foo\", but I didn't look into it.\n\nDeleting a loose reference doesn't cause the directory containing it to\nbe deleted.  The directory is only deleted by pack-refs (and then only\nwhen a reference in the directory was just packed) or when there is an\nattempt to create a new reference that conflicts with the directory.  So\nthe question is whether the creation of a loose ref file is robust\nagainst the disappearance of a directory that it just created.\n\nAnd the answer is \"no\".  It looks like there are a bunch of places where\nsimilar races occur involving references.  And probably many others\nelsewhere in the code.  (Any caller of safe_create_leading_directories()\nis a candidate problem point, and in fact that function itself has an\ninternal race.)  I've started fixing some of these but it might take a\nwhile.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"232281","messageId":"20131220093048.GB9637@sigill.intra.peff.net","threadId":"35539","inReplyTo":"52B31FF3.1060102@alum.mit.edu","subject":"Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-20T09:30:48Z","receivedAt":"2013-12-20T09:30:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 19, 2013 at 05:33:55PM +0100, Michael Haggerty wrote:\n\n> > But we don't loop on ENOENT. So if the rmdir happens in the middle,\n> > after the mkdir but before we call open again, we'd fail, because we\n> > don't treat ENOENT specially in the second call to open. That is\n> > unlikely to happen, though, as prune would not be removing a directory\n> > it did not just enter and clean up an object from (in which case we\n> > would not have gotten the first ENOENT in the creator). [...]\n> \n> The way I read it, prune tries to delete the directory whether or not\n> there were any files in it.  So the race could be triggered by a single\n> writer that wants to write an object to a not-yet-existent shard\n> directory and a single prune process that encounters the directory\n> between when it is created and when the object file is added.\n\nYes, that's true. It does make the race slightly more difficult than a\nstraight deletion because the prune has to catch it in the moment where\nit exists but does not yet have an object. But it's still possible.\n\n> But that doesn't mean I disagree with your conclusion:\n\nI think we're in violent agreement at this point. :)\n\n> Regarding references:\n> \n> > On a similar note, I imagine that a simultaneous \"branch foo/bar\" and\n> > \"branch -d foo/baz\" could race over the creation/deletion of\n> > \"refs/heads/foo\", but I didn't look into it.\n> \n> Deleting a loose reference doesn't cause the directory containing it to\n> be deleted.  The directory is only deleted by pack-refs (and then only\n> when a reference in the directory was just packed) or when there is an\n> attempt to create a new reference that conflicts with the directory.  So\n> the question is whether the creation of a loose ref file is robust\n> against the disappearance of a directory that it just created.\n\nAh, right, I forgot we leave the directories sitting around after\ndeletion. So we may run into a collision with another creator, but by\ndefinition we would have a D/F conflict with such a creator anyway, so\nwe cannot both succeed.\n\nBut we can hit the problem with pack-refs, as you note:\n\n> And the answer is \"no\".  It looks like there are a bunch of places where\n> similar races occur involving references.  And probably many others\n> elsewhere in the code.  (Any caller of safe_create_leading_directories()\n> is a candidate problem point, and in fact that function itself has an\n> internal race.)  I've started fixing some of these but it might take a\n> while.\n\nYeah, I think you'd have to teach safe_create_leading_directories to\natomically try-to-create-and-check-errno rather than stat+mkdir. And\nthen teach it to backtrack when an expected leading path goes missing\nafter we created it (so mkdir(\"foo\"), then mkdir(\"foo/bar\"), then step\nback to mkdir(\"foo\") if we got ENOENT).\n\nI don't think the races are a big deal, though. As with the prune case,\nwe will ultimately fail to create the lockfile and get a temporary\nfailure rather than a corruption. So unless we actually have reports of\nit happening (and I have seen none), it's probably not worth spending\nmuch time on.\n\n-Peff\n"}]}