Correctly initialize 'installed_handlers'

From: Force Charlie <[email protected]>

Because static variables are not initialized properly, temporary files may not be deleted when receive-pack receives a signal.

---
Correctly initialize 'installed_handlers'

Because static variables are not initialized properly, temporary files may not be deleted when receive-pack receives a signal.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-630%2Ffcharlie%2Fincoming_fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-630/fcharlie/incoming_fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/630

 tmp-objdir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 91c00567f4d..c1ccf78e5ed 100644
@@ -123,7 +123,7 @@ static int setup_tmp_objdir(const char *root)
 
 struct tmp_objdir *tmp_objdir_create(void)
 {
-	static int installed_handlers;
+	static int installed_handlers = 0;
 	struct tmp_objdir *t;
 
 	if (the_tmp_objdir)

base-commit: 07d8ea56f2ecb64b75b92264770c0a664231ce17

gitgitgadget

what do you mean by "properly"?, all static variables are set to 0; that is a warranty of the language (all the way to K&R) and any C compiler should enforce that as part of the standard.

the way this is handled would seem to indicate otherwise

if (!installed_handlers) {
                atexit(remove_tmp_objdir);
                sigchain_push_common(remove_tmp_objdir_on_signal);
                installed_handlers++;
}

there is no explicit locking and so there might be a thread race condition, but the code below wouldn't make a difference in that case.

could you elaborate more on how to reproduce the problem?, I suspect that if there was a problem then suppressing whatever signal that was triggered before sigchain_push_common might help, but the window is too short to be a likely issue.

Carlo

It is a curious piece of code.

The "prepare a file-scope static and do something and increment it when it is 0" pattern expects the function to be called many times and do the guarded thing only once. However, there is this code:

	if (the_tmp_objdir)
		BUG(...);

before we do anything else, and then before that "arrange to clean up, but do so just once" block, there is

	the_tmp_objdir = t;

where t is the pointer to a "struct tmp_objdir" instance. So one part of the function expects to be called at most once, while another part is prepared to be called more than once.

Almost all of this function is attributed to 2564d994 (tmp-objdir:
introduce API for temporary object directories, 2016-10-03), so let's see if Peff remembers anything about this curiosity.

Thanks.

There is "only once per program" and "only one at a time". When the tmp_objdir is destroyed (either directly or via tmp_objdir_migrate), we set the_tmp_objdir back to NULL, and you are free to then create another one.

In practice there's only one caller (receive-pack) and it only ever uses one tmp_objdir per program, so it's mostly academic. But tmp-objdir.c was written to be as reusable and least-surprising as possible. I would have avoided the "one at a time" rule if I could, but the semantics are unclear (if you have two active, which one should object-writes go to?).

The atexit and signal handlers could be removed when there's no tmp_objdir active, but there's no easy way to remove them (there's nothing portable at all for atexit, and for sigchain we don't know if somebody else has pushed in the meantime).

-Peff