git.exe crashes on exit in mi_thread_init on windows 2.40.1.vfs.0.0 about git HOT 17 CLOSED

Hmm. We probably could side-step this entire issue by ripping out these lines from run_post_command_hook():

	lock = getenv("COMMAND_HOOK_LOCK");
	if (!lock || strcmp(lock, "true"))
		return 0;

I do not see why they are needed: The preceding lines verify that run_post_hook is set to something non-zero, which is only the case after the environment variable COMMAND_HOOK_LOCK is defined to true anyway, and nothing in the same process seems to be able to override that, and child processes cannot override the current process' environment, either.

So I do not see why that getenv() call is needed at all, it is redundant, I think.

What do you think @jeffhostetler?

from git.

I don't understand how this "lock" is corrdinated with other concurrent Git processes

Well, if it wanted to talk to concurrent processes, then environment variables are the wrong way to do it, as they do not influence each other across processes (except child processes that inherit the parents' environment variables on startup).

from git.

Making notes here while I crawl back thru the stack trace...

• git.exe is in run_post_command_hook() and looking up COMMAND_HOOK_LOCK environment variable.
• The mingw_getenv() code is allocating a buffer to receiver the envvar.
• The compat/mimalloc code being used instead of the LIBC malloc routines.
• The _mi_malloc_generic() does not believe that the heap is initialized and tries to initialize it.
  • This is dubious, since we are about to shutdown and should have used lots of memory already.
• The _mi_thread_init() code initializes (er, ?re-initializes?) the process/thread state and calls _mi_heap_init().
• _mi_heap_init() does not think it is the main thread and needs to generate random keys for thread-local-data.
• mi_random_init_ex() calls os_random_buf() which (depending on some #ifdefs) calls either RtlGenRandom() or LoadLibrary() into bcrypt.dll.

from git.

Adding a little more context (which I started writing before @jeffhostetler posted an answer):

git.exe!mi_random_init_ex() Line 325 C++
git.exe!_mi_heap_init() Line 282 C++
git.exe!mi_thread_init() Line 437 C++
git.exe!_mi_malloc_generic() Line 423 C++

This is in the mimalloc code that is vendored into Git for Windows since v2.38.0.

from git.

https://github.com/microsoft/mimalloc/issues?q=is%3Aissue+is%3Aopen+bcryptprimitives lists some interesting tickets, looking now.

from git.

msvcrt.dll!doexit() Unknown

Oh wait! I think that's it. microsoft/mimalloc#521 (comment) suggests that there may be issues initializing the bcrypt routines while exiting.

from git.

Yeah, I was wondering about how much work we were doing in an atexit() callback.

from git.

Yeah, it's weird. We are "holding" the lock in env block of the current process so that child processes can inherit it?? So that that they don't try to take the lock maybe?? And the guard on run_post_hook guarantees that the process that took the "lock" is the one that releases it, so we don't have to worry about child processes prematurely/incorrectly releasing it.

However, I don't understand how this "lock" is corrdinated with other concurrent Git processes, but we don't have to solve that today....

from git.

IIRC, the gvfs mount daemon/service blocks the pre-command hook task until it is available. so they handle the concurrency thing inside the daemon. the code here is necessary to prevent child process from trying the lock and getting blocked.

I had forgotten about that.

from git.

I think the post-command hook check is to prevent endless recursion because the hook might execute Git commands. Removing it is unlikely to be a solution.

from git.

At startup (on locally built version of git.exe):

• crt/gccmain.c : __do_global_ctors() calls into mi_process_init() and mi_process_load() and mi_heap_main_init() which calls _mi_random_init_weak() at line init.c:172.
  • So we do not initialize bcrypt.dll at startup (we force "use_weak" while the OS handles the rest of the process startup stuff and use internal code to generate the random seed data).
  • _mi_random_init_ex() is not called again (well, in my attempts to repro it) until I forced it in an atexit() handler function.

See #579 for more discussion.

from git.

git-for-windows#4420

from git.

#580

from git.

@jeffhostetler I think the only thing left to do is to promote v2.40.1.vfs.0.1 as a full release, and then we can close this, right? When do you want to do that?

from git.

yeah.

from git.

button pushed.

from git.

thank you for fixing this!

from git.