{"thread":{"id":"37549","subject":"[PATCH] repack: call prune_packed_objects() and update_server_info() directly","startedAt":"2014-09-13T07:28:01Z","lastAt":"2014-09-15T23:56:59Z","messageCount":9,"participants":["René Scharfe","Stefan Beller","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"249335","messageId":"5413F201.8030005@web.de","threadId":"37549","inReplyTo":null,"subject":"[PATCH] repack: call prune_packed_objects() and update_server_info() directly","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-09-13T07:28:01Z","receivedAt":"2014-09-13T07:28:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Call the functions behind git prune-packed and git update-server-info\ndirectly instead of using run_command(). This is shorter, easier and\nquicker.\n\nSigned-off-by: Rene Scharfe \n---\n builtin/repack.c | 23 ++++++-----------------\n 1 file changed, 6 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex fc088db..2aae05d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -377,6 +377,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t/* End of pack replacement. */\n \n \tif (delete_redundant) {\n+\t\tint opts = 0;\n \t\tsort_string_list(&names);\n \t\tfor_each_string_list_item(item, &existing_packs) {\n \t\t\tchar *sha1;\n@@ -387,25 +388,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tif (!string_list_has_string(&names, sha1))\n \t\t\t\tremove_redundant_pack(packdir, item->string);\n \t\t}\n-\t\targv_array_push(&cmd_args, \"prune-packed\");\n-\t\tif (quiet)\n-\t\t\targv_array_push(&cmd_args, \"--quiet\");\n-\n-\t\tmemset(&cmd, 0, sizeof(cmd));\n-\t\tcmd.argv = cmd_args.argv;\n-\t\tcmd.git_cmd = 1;\n-\t\trun_command(&cmd);\n-\t\targv_array_clear(&cmd_args);\n+\t\tif (!quiet && isatty(2))\n+\t\t\topts |= PRUNE_PACKED_VERBOSE;\n+\t\tprune_packed_objects(opts);\n \t}\n \n-\tif (!no_update_server_info) {\n-\t\targv_array_push(&cmd_args, \"update-server-info\");\n-\t\tmemset(&cmd, 0, sizeof(cmd));\n-\t\tcmd.argv = cmd_args.argv;\n-\t\tcmd.git_cmd = 1;\n-\t\trun_command(&cmd);\n-\t\targv_array_clear(&cmd_args);\n-\t}\n+\tif (!no_update_server_info)\n+\t\tupdate_server_info(0);\n \tremove_temporary_files();\n \tstring_list_clear(&names, 0);\n \tstring_list_clear(&rollback, 0);\n-- \n2.1.0\n"},{"id":"249339","messageId":"54140757.4010300@gmail.com","threadId":"37549","inReplyTo":"5413F201.8030005@web.de","subject":"Re: [PATCH] repack: call prune_packed_objects() and update_server_info() directly","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2014-09-13T08:59:03Z","receivedAt":"2014-09-13T08:59:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 13.09.2014 09:28, René Scharfe wrote:\n> Call the functions behind git prune-packed and git update-server-info\n> directly instead of using run_command(). This is shorter, easier and\n> quicker.\n> \n> Signed-off-by: Rene Scharfe \n\nThanks for cleaning up the literal rewrite of the shell script\nand making it look more like a C program.\n\nReviewed-by: Stefan Beller \n\n> ---\n> builtin/repack.c | 23 ++++++-----------------\n> 1 file changed, 6 insertions(+), 17 deletions(-)\n> \n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index fc088db..2aae05d 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -377,6 +377,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t/* End of pack replacement. */\n> \n> \tif (delete_redundant) {\n> +\t\tint opts = 0;\n> \t\tsort_string_list(&names);\n> \t\tfor_each_string_list_item(item, &existing_packs) {\n> \t\t\tchar *sha1;\n> @@ -387,25 +388,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\t\tif (!string_list_has_string(&names, sha1))\n> \t\t\t\tremove_redundant_pack(packdir, item->string);\n> \t\t}\n> -\t\targv_array_push(&cmd_args, \"prune-packed\");\n> -\t\tif (quiet)\n> -\t\t\targv_array_push(&cmd_args, \"--quiet\");\n> -\n> -\t\tmemset(&cmd, 0, sizeof(cmd));\n> -\t\tcmd.argv = cmd_args.argv;\n> -\t\tcmd.git_cmd = 1;\n> -\t\trun_command(&cmd);\n> -\t\targv_array_clear(&cmd_args);\n> +\t\tif (!quiet && isatty(2))\n> +\t\t\topts |= PRUNE_PACKED_VERBOSE;\n> +\t\tprune_packed_objects(opts);\n> \t}\n> \n> -\tif (!no_update_server_info) {\n> -\t\targv_array_push(&cmd_args, \"update-server-info\");\n> -\t\tmemset(&cmd, 0, sizeof(cmd));\n> -\t\tcmd.argv = cmd_args.argv;\n> -\t\tcmd.git_cmd = 1;\n> -\t\trun_command(&cmd);\n> -\t\targv_array_clear(&cmd_args);\n> -\t}\n> +\tif (!no_update_server_info)\n> +\t\tupdate_server_info(0);\n> \tremove_temporary_files();\n> \tstring_list_clear(&names, 0);\n> \tstring_list_clear(&rollback, 0);\n> \n"},{"id":"249354","messageId":"20140913201538.GA24854@peff.net","threadId":"37549","inReplyTo":"5413F201.8030005@web.de","subject":"Re: [PATCH] repack: call prune_packed_objects() and update_server_info() directly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-13T20:15:38Z","receivedAt":"2014-09-13T20:15:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 13, 2014 at 09:28:01AM +0200, René Scharfe wrote:\n\n> Call the functions behind git prune-packed and git update-server-info\n> directly instead of using run_command(). This is shorter, easier and\n> quicker.\n\nIt can also introduce bugs, since a lot of git code assumes it is\nrunning in a single process and can die() or mark up global variables at\nwill. :)\n\nI gave a quick read-through of the code and I think these calls are OK.\nThe two things I noticed were:\n\n 1. We might die on a malloc failure that would otherwise go unnoticed\n in a sub-process. That's probably OK.\n\n 2. The info/packs file is generated from our internal packed_git list.\n This list can get crufty if you have a long-running process that\n accesses objects and other processes are repacking. I think that's\n OK here; the parent repack process is not very long-lived.\n\nI did, however, notice that the code we are calling has some problems of\nits own. :) Here are some fixes:\n\n [1/3]: prune-packed: fix minor memory leak\n [2/3]: make update-server-info more robust\n [3/3]: server-info: clean up after writing info/packs\n\n-Peff\n"},{"id":"249355","messageId":"20140913201653.GA27082@peff.net","threadId":"37549","inReplyTo":"20140913201538.GA24854@peff.net","subject":"[PATCH 1/3] prune-packed: fix minor memory leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-13T20:16:53Z","receivedAt":"2014-09-13T20:16:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We form all of our directories in a strbuf, but never release it.\n\nSigned-off-by: Jeff King \n---\n builtin/prune-packed.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/prune-packed.c b/builtin/prune-packed.c\nindex 6879468..d430731 100644\n--- a/builtin/prune-packed.c\n+++ b/builtin/prune-packed.c\n@@ -68,6 +68,7 @@ void prune_packed_objects(int opts)\n \t\trmdir(pathname.buf);\n \t}\n \tstop_progress(&progress);\n+\tstrbuf_release(&pathname);\n }\n \n int cmd_prune_packed(int argc, const char **argv, const char *prefix)\n-- \n2.1.0.373.g91ca799\n"},{"id":"249356","messageId":"20140913201920.GB27082@peff.net","threadId":"37549","inReplyTo":"20140913201538.GA24854@peff.net","subject":"[PATCH 2/3] make update-server-info more robust","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-13T20:19:20Z","receivedAt":"2014-09-13T20:19:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since \"git update-server-info\" may be called automatically\nas part of a push or a \"gc --auto\", we should be robust\nagainst two processes trying to update it simultaneously.\nHowever, we currently use a fixed tempfile, which means that\ntwo simultaneous writers may step on each other's toes and\nend up renaming junk into place.\n\nLet's instead switch to using a unique tempfile via mkstemp.\nWe do not want to use a lockfile here, because it's OK for\ntwo writers to simultaneously update (one will \"win\" the\nrename race, but that's OK; they should be writing the same\ninformation).\n\nWhile we're there, let's clean up a few other things:\n\n 1. Detect write errors. Report them and abort the update\n if any are found.\n\n 2. Free path memory rather than leaking it (and clean up\n the tempfile when necessary).\n\n 3. Use the pathdup functions consistently rather than\n static buffers or manually calculated lengths.\n\nThis last one fixes a potential overflow of \"infofile\" in\nupdate_info_packs (e.g., by putting large junk into\n$GIT_OBJECT_DIRECTORY). However, this overflow was probably\nnot an interesting attack vector for two reasons:\n\n a. The attacker would need to control the environment to\n do this, in which case it was already game-over.\n\n b. During its setup phase, git checks that the directory\n actually exists, which means it is probably shorter\n than PATH_MAX anyway.\n\nBecause both update_info_refs and update_info_packs share\nthese same failings (and largely duplicate each other), this\npatch factors out the improved error-checking version into a\nhelper function.\n\nSigned-off-by: Jeff King \n---\nI guess point (b) may not apply on systems that have a really small\nPATH_MAX that does not reflect what you can actually create in the\nfilesystem (Windows?). But I think point (a) still applies, so this is\nnot really a big deal security-wise (though it is certainly a bugfix for\nsuch systems).\n\n server-info.c | 116 +++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 71 insertions(+), 45 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex 9ec744e..d54a3d6 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -4,45 +4,80 @@\n #include \"commit.h\"\n #include \"tag.h\"\n \n-/* refs */\n-static FILE *info_ref_fp;\n+/*\n+ * Create the file \"path\" by writing to a temporary file and renaming\n+ * it into place. The contents of the file come from \"generate\", which\n+ * should return non-zero if it encounters an error.\n+ */\n+static int update_info_file(char *path, int (*generate)(FILE *))\n+{\n+\tchar *tmp = mkpathdup(\"%s_XXXXXX\", path);\n+\tint ret = -1;\n+\tint fd = -1;\n+\tFILE *fp = NULL;\n+\n+\tsafe_create_leading_directories(path);\n+\tfd = mkstemp(tmp);\n+\tif (fd < 0)\n+\t\tgoto out;\n+\tfp = fdopen(fd, \"w\");\n+\tif (!fp)\n+\t\tgoto out;\n+\tret = generate(fp);\n+\tif (ret)\n+\t\tgoto out;\n+\tif (fclose(fp))\n+\t\tgoto out;\n+\tif (adjust_shared_perm(tmp) < 0)\n+\t\tgoto out;\n+\tif (rename(tmp, path) < 0)\n+\t\tgoto out;\n+\tret = 0;\n+\n+out:\n+\tif (ret) {\n+\t\terror(\"unable to update %s: %s\", path, strerror(errno));\n+\t\tif (fp)\n+\t\t\tfclose(fp);\n+\t\telse if (fd >= 0)\n+\t\t\tclose(fd);\n+\t\tunlink(tmp);\n+\t}\n+\tfree(tmp);\n+\treturn ret;\n+}\n \n static int add_info_ref(const char *path, const unsigned char *sha1, int flag, void *cb_data)\n {\n+\tFILE *fp = cb_data;\n \tstruct object *o = parse_object(sha1);\n \tif (!o)\n \t\treturn -1;\n \n-\tfprintf(info_ref_fp, \"%s\t%s\\n\", sha1_to_hex(sha1), path);\n+\tif (fprintf(fp, \"%s\t%s\\n\", sha1_to_hex(sha1), path) < 0)\n+\t\treturn -1;\n+\n \tif (o->type == OBJ_TAG) {\n \t\to = deref_tag(o, path, 0);\n \t\tif (o)\n-\t\t\tfprintf(info_ref_fp, \"%s\t%s^{}\\n\",\n-\t\t\t\tsha1_to_hex(o->sha1), path);\n+\t\t\tif (fprintf(fp, \"%s\t%s^{}\\n\",\n+\t\t\t\tsha1_to_hex(o->sha1), path) < 0)\n+\t\t\t\treturn -1;\n \t}\n \treturn 0;\n }\n \n+static int generate_info_refs(FILE *fp)\n+{\n+\treturn for_each_ref(add_info_ref, fp);\n+}\n+\n static int update_info_refs(int force)\n {\n-\tchar *path0 = git_pathdup(\"info/refs\");\n-\tint len = strlen(path0);\n-\tchar *path1 = xmalloc(len + 2);\n-\n-\tstrcpy(path1, path0);\n-\tstrcpy(path1 + len, \"+\");\n-\n-\tsafe_create_leading_directories(path0);\n-\tinfo_ref_fp = fopen(path1, \"w\");\n-\tif (!info_ref_fp)\n-\t\treturn error(\"unable to update %s\", path1);\n-\tfor_each_ref(add_info_ref, NULL);\n-\tfclose(info_ref_fp);\n-\tadjust_shared_perm(path1);\n-\trename(path1, path0);\n-\tfree(path0);\n-\tfree(path1);\n-\treturn 0;\n+\tchar *path = git_pathdup(\"info/refs\");\n+\tint ret = update_info_file(path, generate_info_refs);\n+\tfree(path);\n+\treturn ret;\n }\n \n /* packs */\n@@ -198,36 +233,27 @@ static void init_pack_info(const char *infofile, int force)\n \t\tinfo[i]->new_num = i;\n }\n \n-static void write_pack_info_file(FILE *fp)\n+static int write_pack_info_file(FILE *fp)\n {\n \tint i;\n-\tfor (i = 0; i < num_pack; i++)\n-\t\tfprintf(fp, \"P %s\\n\", info[i]->p->pack_name + objdirlen + 6);\n-\tfputc('\\n', fp);\n+\tfor (i = 0; i < num_pack; i++) {\n+\t\tif (fprintf(fp, \"P %s\\n\", info[i]->p->pack_name + objdirlen + 6) < 0)\n+\t\t\treturn -1;\n+\t}\n+\tif (fputc('\\n', fp) == EOF)\n+\t\treturn -1;\n+\treturn 0;\n }\n \n static int update_info_packs(int force)\n {\n-\tchar infofile[PATH_MAX];\n-\tchar name[PATH_MAX];\n-\tint namelen;\n-\tFILE *fp;\n-\n-\tnamelen = sprintf(infofile, \"%s/info/packs\", get_object_directory());\n-\tstrcpy(name, infofile);\n-\tstrcpy(name + namelen, \"+\");\n+\tchar *infofile = mkpathdup(\"%s/info/packs\", get_object_directory());\n+\tint ret;\n \n \tinit_pack_info(infofile, force);\n-\n-\tsafe_create_leading_directories(name);\n-\tfp = fopen(name, \"w\");\n-\tif (!fp)\n-\t\treturn error(\"cannot open %s\", name);\n-\twrite_pack_info_file(fp);\n-\tfclose(fp);\n-\tadjust_shared_perm(name);\n-\trename(name, infofile);\n-\treturn 0;\n+\tret = update_info_file(infofile, write_pack_info_file);\n+\tfree(infofile);\n+\treturn ret;\n }\n \n /* public */\n-- \n2.1.0.373.g91ca799\n"},{"id":"249357","messageId":"20140913201938.GC27082@peff.net","threadId":"37549","inReplyTo":"20140913201538.GA24854@peff.net","subject":"[PATCH 3/3] server-info: clean up after writing info/packs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-13T20:19:38Z","receivedAt":"2014-09-13T20:19:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We allocate pack information in a static global list but\nnever clean it up. This leaks memory, and means that calling\nupdate_server_info twice will generate a buggy file (it will\nhave duplicate entries).\n\nSigned-off-by: Jeff King \n---\n server-info.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/server-info.c b/server-info.c\nindex d54a3d6..31f4a74 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -233,6 +233,14 @@ static void init_pack_info(const char *infofile, int force)\n \t\tinfo[i]->new_num = i;\n }\n \n+static void free_pack_info(void)\n+{\n+\tint i;\n+\tfor (i = 0; i < num_pack; i++)\n+\t\tfree(info[i]);\n+\tfree(info);\n+}\n+\n static int write_pack_info_file(FILE *fp)\n {\n \tint i;\n@@ -252,6 +260,7 @@ static int update_info_packs(int force)\n \n \tinit_pack_info(infofile, force);\n \tret = update_info_file(infofile, write_pack_info_file);\n+\tfree_pack_info();\n \tfree(infofile);\n \treturn ret;\n }\n-- \n2.1.0.373.g91ca799\n"},{"id":"249402","messageId":"5415D289.9080507@web.de","threadId":"37549","inReplyTo":"20140913201920.GB27082@peff.net","subject":"Re: [PATCH 2/3] make update-server-info more robust","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-09-14T17:38:17Z","receivedAt":"2014-09-14T17:38:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 13.09.2014 um 22:19 schrieb Jeff King:\n> Since \"git update-server-info\" may be called automatically\n> as part of a push or a \"gc --auto\", we should be robust\n> against two processes trying to update it simultaneously.\n> However, we currently use a fixed tempfile, which means that\n> two simultaneous writers may step on each other's toes and\n> end up renaming junk into place.\n> \n> Let's instead switch to using a unique tempfile via mkstemp.\n> We do not want to use a lockfile here, because it's OK for\n> two writers to simultaneously update (one will \"win\" the\n> rename race, but that's OK; they should be writing the same\n> information).\n> \n> While we're there, let's clean up a few other things:\n> \n> 1. Detect write errors. Report them and abort the update\n> if any are found.\n> \n> 2. Free path memory rather than leaking it (and clean up\n> the tempfile when necessary).\n> \n> 3. Use the pathdup functions consistently rather than\n> static buffers or manually calculated lengths.\n> \n> This last one fixes a potential overflow of \"infofile\" in\n> update_info_packs (e.g., by putting large junk into\n> $GIT_OBJECT_DIRECTORY). However, this overflow was probably\n> not an interesting attack vector for two reasons:\n> \n> a. The attacker would need to control the environment to\n> do this, in which case it was already game-over.\n> \n> b. During its setup phase, git checks that the directory\n> actually exists, which means it is probably shorter\n> than PATH_MAX anyway.\n> \n> Because both update_info_refs and update_info_packs share\n> these same failings (and largely duplicate each other), this\n> patch factors out the improved error-checking version into a\n> helper function.\n> \n> Signed-off-by: Jeff King \n> ---\n> I guess point (b) may not apply on systems that have a really small\n> PATH_MAX that does not reflect what you can actually create in the\n> filesystem (Windows?).\n\nIt's the other way around: PATH_MAX is an actual limit basically only\non Windows [1] unless you avoid using the Windows API [2].\n\nRegardless of the security implications, getting rid of more PATH_MAX\nbuffers is a good move.\n\nAnd I looked only briefly at your patch, but I like the three bullet\npoints above. :)\n\nRené\n\n\n[1] http://insanecoding.blogspot.de/2007/11/pathmax-simply-isnt.html\n[2] http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx\n"},{"id":"249444","messageId":"xmqqlhpk7mpb.fsf@gitster.dls.corp.google.com","threadId":"37549","inReplyTo":"20140913201920.GB27082@peff.net","subject":"Re: [PATCH 2/3] make update-server-info more robust","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-15T18:39:12Z","receivedAt":"2014-09-15T18:39:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Since \"git update-server-info\" may be called automatically\n> as part of a push or a \"gc --auto\", we should be robust\n> against two processes trying to update it simultaneously.\n> However, we currently use a fixed tempfile, which means that\n> two simultaneous writers may step on each other's toes and\n> end up renaming junk into place.\n\nThanks. I'll queue these clean-ups but we may want to start\nthinking about deprecating and removing the dumb http support.\n"},{"id":"249495","messageId":"20140915235659.GA5019@peff.net","threadId":"37549","inReplyTo":"xmqqlhpk7mpb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/3] make update-server-info more robust","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-15T23:56:59Z","receivedAt":"2014-09-15T23:56:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 15, 2014 at 11:39:12AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Since \"git update-server-info\" may be called automatically\n> > as part of a push or a \"gc --auto\", we should be robust\n> > against two processes trying to update it simultaneously.\n> > However, we currently use a fixed tempfile, which means that\n> > two simultaneous writers may step on each other's toes and\n> > end up renaming junk into place.\n> \n> Thanks. I'll queue these clean-ups but we may want to start\n> thinking about deprecating and removing the dumb http support.\n\nYeah, I have often thought about that (especially the push support,\nwhich has always been flaky and underused). However, some possible\nschemes for resumable clone could be easily implemented by shunting the\ncloner to a dumb-http conversation. So it may be worth keeping at least\nthe fetch side around for the time being. Food for thought.\n\n-Peff\n"}]}