git.exe crashes on exit in mi_thread_init on windows 2.40.1.vfs.0.0

[andygo-msft]

• I was not able to find an open
  or closed issue matching
  what I'm seeing, including in the git-for-windows/git tracker.

Setup

• Which version of microsoft/git are you using? Is it 32-bit or 64-bit?

$ git --version --build-options

git version 2.40.1.vfs.0.0
cpu: x86_64
built from commit: 27f6460705d34dd1692da5b8c9c26401dd2d5a89
sizeof-long: 4
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon

Are you using Scalar or VFS for Git?

Scalar

If VFS for Git, then what version?

$ gvfs version

N/A

• Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?

$ cmd.exe /c ver

Microsoft Windows [Version 10.0.19045.2846]

Win10 22H2 64 bit

• Any other interesting things about your environment that might be related
  to the issue you're seeing?

JIT debugger enabled

Details

• Which terminal/shell are you running Git from? e.g Bash/CMD/PowerShell/other

CMD.EXE

• What commands did you run to trigger this issue? If you can provide a
  Minimal, Complete, and Verifiable example
  this will help us understand the issue.

This was automatically invoked by something, I believe VS:

C:\Program Files\Git\mingw64\bin\git.exe  -c "gc.auto=0" -c "credential.helper=manager" -c "core.askpass=C:/Program Files/Git/mingw64/bin\git-askpass.exe" --no-pager rev-list --topo-order --use-bitmap-index --header HEAD --

another example that happened with VS running after changing branches at the cmd line:

C:\Program Files\Git\mingw64\bin\git.exe  -c "gc.auto=0" -c "credential.helper=manager" -c "core.askpass=C:/Program Files/Git/mingw64/bin\git-askpass.exe" -c "core.abbrev=40" --no-pager reflog

• What did you expect to occur after running these commands?

exit w/o a crash that starts the JIT debugger

• What actually happened instead?

the JIT debugger is invoked. windbgx cannot attach because the process is exiting. VS did attach successfully and showed an AV at this stack:

bcryptprimitives.dll!AesRNGState_generate() Unknown
bcryptprimitives.dll!GenRandomAes�() Unknown
bcryptprimitives.dll!MSCryptGenRandom() Unknown
bcrypt.dll!BCryptGenRandom() Unknown
bcrypt.dll!BCryptGenRandom() Unknown
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++
git.exe!mingw_getenv() Line 2359 C++
git.exe!run_post_command_hook() Line 451 C++
msvcrt.dll!doexit() Unknown
msvcrt.dll!__CRTDLL_INIT() Unknown
ntdll.dll!LdrpCallInitRoutine() Unknown
ntdll.dll!LdrShutdownProcess�() Unknown
ntdll.dll!RtlExitUserProcess() Unknown
KernelBase.dll!DefaultHandler�() Unknown
KernelBase.dll!CtrlRoutine() Unknown
kernel32.dll!BaseThreadInitThunk�() Unknown
ntdll.dll!RtlUserThreadStart�() Unknown

• If the problem was occurring with a specific repository, can you specify
  the repository?

  • Public repo: insert URL here
  • Windows monorepo
  • Office monorepo
  • Other Microsoft-internal repo: Substrate
  • Other internal repo.

[jeffhostetler]

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.

[dscho]

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.

[dscho]

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

[dscho]

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.

[jeffhostetler]

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

[dscho]

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?

[jeffhostetler]

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....

[dscho]

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).

[jeffhostetler]

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.

[derrickstolee]

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.

[jeffhostetler]

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.

[jeffhostetler]

git-for-windows#4420

[jeffhostetler]

#580

[derrickstolee]

@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?