{"thread":{"id":"53431","subject":"[PATCH] Correctly initialize 'installed_handlers'","startedAt":"2020-05-08T07:05:19Z","lastAt":"2020-05-08T17:24:47Z","messageCount":4,"participants":["Force Charlie via GitGitGadget","Carlo Marcelo Arenas Belón","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"397394","messageId":"pull.630.git.1588921514146.gitgitgadget@gmail.com","threadId":"53431","inReplyTo":null,"subject":"[PATCH] Correctly initialize 'installed_handlers'","fromName":"Force Charlie via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-08T07:05:13Z","receivedAt":"2020-05-08T07:05:19Z","isPatch":true,"sender":{"key":"name:Force Charlie","avatar":null},"body":"From: Force Charlie <charlieio@outlook.com>\n\nBecause static variables are not initialized properly,\ntemporary files may not be deleted when receive-pack receives a signal.\n\nSigned-off-by: Force Charlie <charlieio@outlook.com>\n---\n    Correctly initialize 'installed_handlers'\n    \n    Because static variables are not initialized properly, temporary files\n    may not be deleted when receive-pack receives a signal.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-630%2Ffcharlie%2Fincoming_fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-630/fcharlie/incoming_fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/630\n\n tmp-objdir.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex 91c00567f4d..c1ccf78e5ed 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -123,7 +123,7 @@ static int setup_tmp_objdir(const char *root)\n \n struct tmp_objdir *tmp_objdir_create(void)\n {\n-\tstatic int installed_handlers;\n+\tstatic int installed_handlers = 0;\n \tstruct tmp_objdir *t;\n \n \tif (the_tmp_objdir)\n\nbase-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17\n-- \ngitgitgadget\n"},{"id":"397396","messageId":"20200508083340.GA36538@Carlos-MBP","threadId":"53431","inReplyTo":"pull.630.git.1588921514146.gitgitgadget@gmail.com","subject":"Re: [PATCH] Correctly initialize 'installed_handlers'","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-08T08:33:40Z","receivedAt":"2020-05-08T08:33:44Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, May 08, 2020 at 07:05:13AM +0000, Force Charlie via GitGitGadget wrote:\n> From: Force Charlie <charlieio@outlook.com>\n> \n> Because static variables are not initialized properly,\n\nwhat do you mean by \"properly\"?, all static variables are set to 0;\nthat is a warranty of the language (all the way to K&R) and any C compiler\nshould enforce that as part of the standard.\n\n> temporary files may not be deleted when receive-pack receives a signal.\n\nthe way this is handled would seem to indicate otherwise\n\nif (!installed_handlers) {\n                atexit(remove_tmp_objdir);\n                sigchain_push_common(remove_tmp_objdir_on_signal);\n                installed_handlers++;\n}\n\nthere is no explicit locking and so there might be a thread race\ncondition, but the code below wouldn't make a difference in that\ncase.\n\ncould you elaborate more on how to reproduce the problem?, I suspect\nthat if there was a problem then suppressing whatever signal that\nwas triggered before sigchain_push_common might help, but the window\nis too short to be a likely issue.\n\nCarlo\n"},{"id":"397410","messageId":"xmqqy2q2zbej.fsf@gitster.c.googlers.com","threadId":"53431","inReplyTo":"20200508083340.GA36538@Carlos-MBP","subject":"Re: [PATCH] Correctly initialize 'installed_handlers'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-08T15:36:36Z","receivedAt":"2020-05-08T15:36:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:\n\n> the way this is handled would seem to indicate otherwise\n>\n> if (!installed_handlers) {\n>                 atexit(remove_tmp_objdir);\n>                 sigchain_push_common(remove_tmp_objdir_on_signal);\n>                 installed_handlers++;\n> }\n\nIt is a curious piece of code.  \n\nThe \"prepare a file-scope static and do something and increment it\nwhen it is 0\" pattern expects the function to be called many times\nand do the guarded thing only once.  However, there is this code:\n\n\tif (the_tmp_objdir)\n\t\tBUG(...);\n\nbefore we do anything else, and then before that \"arrange to clean\nup, but do so just once\" block, there is\n\n\tthe_tmp_objdir = t;\n\nwhere t is the pointer to a \"struct tmp_objdir\" instance.  So one\npart of the function expects to be called at most once, while\nanother part is prepared to be called more than once.\n\nAlmost all of this function is attributed to 2564d994 (tmp-objdir:\nintroduce API for temporary object directories, 2016-10-03), so\nlet's see if Peff remembers anything about this curiosity.\n\nThanks.\n"},{"id":"397426","messageId":"20200508172445.GC637136@coredump.intra.peff.net","threadId":"53431","inReplyTo":"xmqqy2q2zbej.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] Correctly initialize 'installed_handlers'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-08T17:24:45Z","receivedAt":"2020-05-08T17:24:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 08, 2020 at 08:36:36AM -0700, Junio C Hamano wrote:\n\n> Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:\n> \n> > the way this is handled would seem to indicate otherwise\n> >\n> > if (!installed_handlers) {\n> >                 atexit(remove_tmp_objdir);\n> >                 sigchain_push_common(remove_tmp_objdir_on_signal);\n> >                 installed_handlers++;\n> > }\n> \n> It is a curious piece of code.  \n> \n> The \"prepare a file-scope static and do something and increment it\n> when it is 0\" pattern expects the function to be called many times\n> and do the guarded thing only once.  However, there is this code:\n> \n> \tif (the_tmp_objdir)\n> \t\tBUG(...);\n> \n> before we do anything else, and then before that \"arrange to clean\n> up, but do so just once\" block, there is\n> \n> \tthe_tmp_objdir = t;\n> \n> where t is the pointer to a \"struct tmp_objdir\" instance.  So one\n> part of the function expects to be called at most once, while\n> another part is prepared to be called more than once.\n> \n> Almost all of this function is attributed to 2564d994 (tmp-objdir:\n> introduce API for temporary object directories, 2016-10-03), so\n> let's see if Peff remembers anything about this curiosity.\n\nThere is \"only once per program\" and \"only one at a time\". When the\ntmp_objdir is destroyed (either directly or via tmp_objdir_migrate), we\nset the_tmp_objdir back to NULL, and you are free to then create another\none.\n\nIn practice there's only one caller (receive-pack) and it only ever uses\none tmp_objdir per program, so it's mostly academic. But tmp-objdir.c\nwas written to be as reusable and least-surprising as possible. I would\nhave avoided the \"one at a time\" rule if I could, but the semantics are\nunclear (if you have two active, which one should object-writes go to?).\n\nThe atexit and signal handlers could be removed when there's no\ntmp_objdir active, but there's no easy way to remove them (there's\nnothing portable at all for atexit, and for sigchain we don't know if\nsomebody else has pushed in the meantime).\n\n-Peff\n"}]}