{"thread":{"id":"38543","subject":"[PATCH] fast-import: avoid running end_packfile recursively","startedAt":"2015-02-10T01:07:19Z","lastAt":"2015-02-10T18:58:29Z","messageCount":3,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"255874","messageId":"20150210010719.GA31823@peff.net","threadId":"38543","inReplyTo":null,"subject":"[PATCH] fast-import: avoid running end_packfile recursively","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-10T01:07:19Z","receivedAt":"2015-02-10T01:07:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When an import has finished, we run end_packfile() to\nfinalize the data and move the packfile into place. If this\nprocess fails, we call die() and end up in our die_nicely()\nhandler.  Which unfortunately includes running end_packfile\nto save any progress we made. We enter the function again,\nand start operating on the pack_data struct while it is in\nan inconsistent state, leading to a segfault.\n\nOne way to trigger this is to simply start two identical\nfast-imports at the same time. They will both create the\nsame packfiles, which will then try to create identically\nnamed \".keep\" files. One will win the race, and the other\nwill die(), and end up with the segfault.\n\nSince 3c078b9, we already reset the pack_data pointer to\nNULL at the end of end_packfile. That covers the case of us\ncalling die() right after end_packfile, before we have\nreinitialized the pack_data pointer. This new problem is\nquite similar, except that we are worried about calling\ndie() _during_ end_packfile, not right after. Ideally we\nwould simply set pack_data to NULL as soon as we enter the\nfunction, and operate on a copy of the pointer.\n\nUnfortunately, it is not so easy. pack_data is a global, and\nend_packfile calls into other functions which operate on the\nglobal directly. We would have to teach each of these to\ntake an argument, and there is no guarantee that we would\ncatch all of the spots.\n\nInstead, we can simply use a static flag to avoid\nrecursively entering the function. This is a little less\nelegant, but it's short and fool-proof.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n fast-import.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex d0bd285..aac2c24 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -947,9 +947,12 @@ static void unkeep_all_packs(void)\n \n static void end_packfile(void)\n {\n-\tif (!pack_data)\n+\tstatic int running;\n+\n+\tif (running || !pack_data)\n \t\treturn;\n \n+\trunning = 1;\n \tclear_delta_base_cache();\n \tif (object_count) {\n \t\tstruct packed_git *new_p;\n@@ -999,6 +1002,7 @@ static void end_packfile(void)\n \t}\n \tfree(pack_data);\n \tpack_data = NULL;\n+\trunning = 0;\n \n \t/* We can't carry a delta across packfiles. */\n \tstrbuf_release(&last_blob.data);\n-- \n2.3.0.rc1.287.g761fd19\n"},{"id":"255892","messageId":"xmqqa90lwqpr.fsf@gitster.dls.corp.google.com","threadId":"38543","inReplyTo":"20150210010719.GA31823@peff.net","subject":"Re: [PATCH] fast-import: avoid running end_packfile recursively","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-10T18:45:20Z","receivedAt":"2015-02-10T18:45:20Z","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> When an import has finished, we run end_packfile() to\n> finalize the data and move the packfile into place. If this\n> process fails, we call die() and end up in our die_nicely()\n> handler.  Which unfortunately includes running end_packfile\n> to save any progress we made. We enter the function again,\n> and start operating on the pack_data struct while it is in\n> an inconsistent state, leading to a segfault.\n> ... This new problem is\n> quite similar, except that we are worried about calling\n> die() _during_ end_packfile, not right after. Ideally we\n> would simply set pack_data to NULL as soon as we enter the\n> function, and operate on a copy of the pointer.\n\nNicely analyzed and well done.\n\n> Unfortunately, it is not so easy. pack_data is a global, and\n> end_packfile calls into other functions which operate on the\n> global directly. We would have to teach each of these to\n> take an argument, and there is no guarantee that we would\n> catch all of the spots.\n\nWell, you can rename the global to something else to make sure ;-)\nBut I think that the approach with a simple flag is better.\n\nIf we were planning to do the global-to-parameter surgery for other\nreasons (perhaps need to make things reentrant?) then the equation\nmight become different, but I do not think we are doing that right\nnow, so...\n\nThanks.\n"},{"id":"255894","messageId":"20150210185828.GB20090@peff.net","threadId":"38543","inReplyTo":"xmqqa90lwqpr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] fast-import: avoid running end_packfile recursively","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-10T18:58:29Z","receivedAt":"2015-02-10T18:58:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 10, 2015 at 10:45:20AM -0800, Junio C Hamano wrote:\n\n> > Unfortunately, it is not so easy. pack_data is a global, and\n> > end_packfile calls into other functions which operate on the\n> > global directly. We would have to teach each of these to\n> > take an argument, and there is no guarantee that we would\n> > catch all of the spots.\n> \n> Well, you can rename the global to something else to make sure ;-)\n> But I think that the approach with a simple flag is better.\n\n:) True. The problems I had in mind were more:\n\n  1. One of the problems with that is that there are a whole bunch of\n     helper functions that use the variable. But only the ones that are\n     called as part of end_packfile need this treatment. So either we\n     need to touch all of them, or we need to figure out reliably which\n     ones are part of this code path.\n\n  2. Unless we get rid of the global completely, we open ourselves up to\n     end_packfile calling new functions, bringing the problem back. This\n     is probably not a huge concern, though, as this code has basically\n     not changed much since its inception.\n\nI did work through it, though, and the result is not _too_ bad. Here it\nis for reference, in case you want to change your mind. The remaining\nreferences are only in start_packfile, and in gfi_load_object, both of\nwhich are hopefully unlikely to be called from end_packfile.\n\n---\ndiff --git a/fast-import.c b/fast-import.c\nindex d0bd285..e842386 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -883,7 +883,7 @@ static void start_packfile(void)\n \tall_packs[pack_id] = p;\n }\n \n-static const char *create_index(void)\n+static const char *create_index(struct packed_git *pack)\n {\n \tconst char *tmpfile;\n \tstruct pack_idx_entry **idx, **c, **last;\n@@ -901,18 +901,18 @@ static const char *create_index(void)\n \tif (c != last)\n \t\tdie(\"internal consistency error creating the index\");\n \n-\ttmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts, pack_data->sha1);\n+\ttmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts, pack->sha1);\n \tfree(idx);\n \treturn tmpfile;\n }\n \n-static char *keep_pack(const char *curr_index_name)\n+static char *keep_pack(struct packed_git *pack, const char *curr_index_name)\n {\n \tstatic char name[PATH_MAX];\n \tstatic const char *keep_msg = \"fast-import\";\n \tint keep_fd;\n \n-\tkeep_fd = odb_pack_keep(name, sizeof(name), pack_data->sha1);\n+\tkeep_fd = odb_pack_keep(name, sizeof(name), pack->sha1);\n \tif (keep_fd < 0)\n \t\tdie_errno(\"cannot create keep file\");\n \twrite_or_die(keep_fd, keep_msg, strlen(keep_msg));\n@@ -920,12 +920,12 @@ static char *keep_pack(const char *curr_index_name)\n \t\tdie_errno(\"failed to write keep file\");\n \n \tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n-\t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n-\tif (move_temp_to_file(pack_data->pack_name, name))\n+\t\t get_object_directory(), sha1_to_hex(pack->sha1));\n+\tif (move_temp_to_file(pack->pack_name, name))\n \t\tdie(\"cannot store pack file\");\n \n \tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.idx\",\n-\t\t get_object_directory(), sha1_to_hex(pack_data->sha1));\n+\t\t get_object_directory(), sha1_to_hex(pack->sha1));\n \tif (move_temp_to_file(curr_index_name, name))\n \t\tdie(\"cannot store index file\");\n \tfree((void *)curr_index_name);\n@@ -947,8 +947,11 @@ static void unkeep_all_packs(void)\n \n static void end_packfile(void)\n {\n+\tstruct packed_git *old_p = pack_data;\n+\n \tif (!pack_data)\n \t\treturn;\n+\tpack_data = NULL;\n \n \tclear_delta_base_cache();\n \tif (object_count) {\n@@ -959,13 +962,13 @@ static void end_packfile(void)\n \t\tstruct branch *b;\n \t\tstruct tag *t;\n \n-\t\tclose_pack_windows(pack_data);\n+\t\tclose_pack_windows(old_p);\n \t\tsha1close(pack_file, cur_pack_sha1, 0);\n-\t\tfixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,\n-\t\t\t\t    pack_data->pack_name, object_count,\n+\t\tfixup_pack_header_footer(old_p->pack_fd, old_p->sha1,\n+\t\t\t\t    old_p->pack_name, object_count,\n \t\t\t\t    cur_pack_sha1, pack_size);\n-\t\tclose(pack_data->pack_fd);\n-\t\tidx_name = keep_pack(create_index());\n+\t\tclose(old_p->pack_fd);\n+\t\tidx_name = keep_pack(old_p, create_index(old_p));\n \n \t\t/* Register the packfile with core git's machinery. */\n \t\tnew_p = add_packed_git(idx_name, strlen(idx_name), 1);\n@@ -994,11 +997,10 @@ static void end_packfile(void)\n \t\tpack_id++;\n \t}\n \telse {\n-\t\tclose(pack_data->pack_fd);\n-\t\tunlink_or_warn(pack_data->pack_name);\n+\t\tclose(old_p->pack_fd);\n+\t\tunlink_or_warn(old_p->pack_name);\n \t}\n-\tfree(pack_data);\n-\tpack_data = NULL;\n+\tfree(old_p);\n \n \t/* We can't carry a delta across packfiles. */\n \tstrbuf_release(&last_blob.data);\n"}]}