git staTUS spawns infinite(?) amounts of child processes.

[tamlin-mike]

Tested on Windows 10 64-bit, versions 2.15 and 2.16.

Mis-spell "status" as "staTUS", and it seems all POSIX platforms complain and just terminate. Windows, not so much. It spawns an insane amount of child processes, before terminating due to lack of resources.

[shiftkey]

@tamlin-mike was this in Git Bash or Git CMD?

[tamlin-mike]

CMD, but IIRC I tested it with bash too, and it also failed.

My guess is there's a strcmp() for "status" or "git-status" on argv[0], but since it's a case insensitive filesystem argv[0] could be "git-staTUS".

[dscho]

@tamlin-mike how about giving the fix a shot yourself? It should be as easy as inserting some code somewhere here: https://github.com/git-for-windows/MINGW-packages/blob/7637a1c656d24a4e58dd69838b00891a34f5afc7/mingw-w64-git/git-wrapper.c#L554-L562

Either add basename_lower (and then do something like basename_lower = wcsdup(basename); _wcslwr(basename_lower); (see https://msdn.microsoft.com/en-us/library/sch3dy08.aspx for details), or use iswlower() in a loop over basename to verify that it is all lowercase and error out if it is not.

[tamlin-mike]

@dscho Thanks for the pointer to the src.

I don't think auto-correcting, even if "only" case, is an option. To be consistent with POSIX platform behavior, checking and bailing out on error seems more reasonable.

I suspect the following could do, though I don't have a system set up to build git itself currently.

To check:

• Should there never be a git command containing a digit, that test can be ripped out.
• Should there be a command containing '-', a check for that obviously needs to be added.

prefix_args_len = wcslen(prefix_args);
+ { /* scope 'i' (C89) to reduce patch complexity */
+   int i;
+   for (i = 0; i < prefix_args_len; ++i) {
+     if (!iswlower(prefix_args[i]) && !iswdigit(prefix_args[i])) {
+       fwprintf(stderr, L"invalid git command: %s\n", prefix_args);
+       r =1;
+       goto bailout;
+     }
+   }
+ }
...
+ bailout:
   free(cmd);

If it turns out to solve the problem, feel free to commit it.

[dscho]

If it turns out to solve the problem, feel free to commit it.

Don't you worry. Should it turn out that I can take on this project, too (because you make no signs of helping me there, apart from a code snippet that is so straight-forward that I already thought of it before you wrote it down), I shall feel free to commit it, indeed. I do not even need encouragement to commit things I implement myself.

😁

[tamlin-mike]

I have given this some thought, and the "patch" it's obviously incomplete as-is.

I can see two solutions:

1. Use the same PATH as CreateProcess is using, and look for a match using SearchPath. If something is found, do a case sensitive (wcscmp/wcsncmp) comparison with the provided command and the found git-xxx.exe, and bail out on mismatch.

2. Convert the user-provided git command ("staTUS") to plain ASCII, and bail out if there is an error or re-mapping on that conversion, or if there are any uppercase char's (this under the assumption there will not be any git commands using uppercase).

Case mapping/switching UCS-2 (Microsoft might claim they are UTF-16, but that's just hogwash at least for the filesystem) is impossible to do reliably.

Option 1 has the benefit of always working, even if there are git commands added with uppercase characters. Downside is an extra kernel call, and general ickyness.

Option 2 feels "cleaner" to me, but relies on an assumption that, while I find it probable, I don't know if it can be relied upon.

Iff it can, then it could be as simple as looping the input command and check for "[A-Z]" (i.e. iswupper), and bail out if anything found.

[kgybels]

I can't reproduce this problem:

C:\git\bug\test>ver

Microsoft Windows [Version 10.0.16299.192]

C:\git\bug\test>git --version --build-options
git version 2.16.2.windows.1
cpu: x86_64
built from commit: e1848984d1004040ec5199e749b5f282ddf4bb09
sizeof-long: 4

C:\git\bug\test>set PATH=%PATH%;C:\Program Files\Git\mingw64\libexec\git-core

C:\git\bug\test>git status
On branch master
nothing to commit, working tree clean

C:\git\bug\test>git-status
On branch master
nothing to commit, working tree clean

C:\git\bug\test>git staTUS
fatal: cannot handle staTUS.exe as a builtin

C:\git\bug\test>git-staTUS
On branch master
nothing to commit, working tree clean

C:\git\bug\test>GIT status
On branch master
nothing to commit, working tree clean

C:\git\bug\test>GIT-status
On branch master
nothing to commit, working tree clean

C:\git\bug\test>GIT-staTUS
On branch master
nothing to commit, working tree clean

There is already some logic in place at git.c#L641-L655 to prevent recursively calling the same executable:

	/*
	 * "git-xxxx" is the same as "git xxxx", but we obviously:
	 *
	 *  - cannot take flags in between the "git" and the "xxxx".
	 *  - cannot execute it externally (since it would just do
	 *    the same thing over again)
	 *
	 * So we just directly call the builtin handler, and die if
	 * that one cannot handle it.
	 */
	if (skip_prefix(cmd, "git-", &cmd)) {
		argv[0] = cmd;
		handle_builtin(argc, argv);
		die("cannot handle %s as a builtin", cmd);
	}

[tamlin-mike]

For precision, I'm using: Microsoft Windows [Version 10.0.16299.248]

@kgybels Can you verify it still works for you if you do not have <gitroot>\mingw64\libexec\git-core in the PATH env. var? If so, this requires digging deeper.

I was using a slightly older version, 2.16.1.windows.4, but the code you refer to has apparently been in place for four years, so that's not the part solving it.

Updating to 2.16.2.windows.1, I can still repro, whether I run "git staTUS" inside a git-managed directory or not. Both tests only performed from cmd.exe.

I do not have <gitroot>\mingw64\libexec\git-core in the path, but even after adding it I get the same "fork bomb" running "git staTUS". I do not see "fatal: cannot handle staTUS.exe as a builtin". I only see CPU consuming and memory consumption rising until a dialog pops up

git.exe - Application Error
---------------------------
The application was unable to start correctly (0xc0000017). Click OK to close the application. 
---------------------------
OK   
---------------------------

0xc0000017 is the NT error STATUS_NO_MEMORY, which in this case seems to relate to a process-local (kmode?) memory limit (system free RAM at the time is, according to Task Manager, still over 10GB).

One possible (even if unlikely) difference is that I have both disabled (and probably cleaned all possible) 8.3 paths from the volume. From an admin command prompt, see:
fsutil 8dot3name query S:
(where S: is my local volume where git resides).

cmd.exe codepage (mode con cp) is 437.

[dscho]

@kgybels the problem occurs only when the Git wrapper is involved, i.e. when %PROGRAMFILES%\Git\cmd is in the PATH (which is the default PATH option).

[dscho]

@kgybels to be precise, I already identified where this would need to be fixed in this comment.

To be quite honest, reading over the code again I am quite surprised that GetModulePath() retains the incorrect case: https://github.com/git-for-windows/MINGW-packages/blob/7637a1c656d24a4e58dd69838b00891a34f5afc7/mingw-w64-git/git-wrapper.c#L505

An alternative would of course be to set an environment variable in the clause handling builtins, to detect recursion. The simplest I could come up with is this patch:

diff --git a/mingw-w64-git/git-wrapper.c b/mingw-w64-git/git-wrapper.c
index 594f803d..07e96966 100644
--- a/mingw-w64-git/git-wrapper.c
+++ b/mingw-w64-git/git-wrapper.c
@@ -557,6 +557,10 @@ int main(void)
 		prefix_args_len = wcslen(buffer);
 	}
 	else if (!wcsnicmp(basename, L"git-", 4)) {
+		LPCWSTR builtin_env = L"GIT_WRAPPER_BUILTIN_NAME";
+		LPWSTR p;
+		int len;
+
 		needs_env_setup = 0;
 
 		/* Call a builtin */
@@ -565,6 +569,26 @@ int main(void)
 		if (!wcsicmp(prefix_args + prefix_args_len - 4, L".exe"))
 			prefix_args_len -= 4;
 
+		len = GetEnvironmentVariableW(builtin_env, NULL, 0);
+		if (len == prefix_args_len + 1) {
+			p = malloc(sizeof(WCHAR) * len);
+			if (!p ||
+			    !GetEnvironmentVariableW(builtin_env, p, len) ||
+			    !wcsnicmp(p, prefix_args, prefix_args_len)) {
+				fwprintf(stderr,
+					 L"Invalid builtin name (incorrect "
+					 "case?): '%.*s'\n",
+					prefix_args_len, prefix_args);
+				exit(1);
+			}
+			free(p);
+		}
+		p = malloc(sizeof(WCHAR) * (prefix_args_len + 1));
+		memcpy(p, prefix_args, sizeof(WCHAR) * prefix_args_len);
+		p[prefix_args_len] = L'\0';
+		SetEnvironmentVariableW(builtin_env, p);
+		free(p);
+
 		/* set the default exe module */
 		wcscpy(exe, exepath);
 		PathAppend(exe, L"git.exe");

[kgybels]

@dscho @tamlin-mike Even without <gitroot>\mingw64\libexec\git-core in the path, I can't reproduce. That's actually the first thing I tried.

Here is the result without <gitroot>\mingw64\libexec\git-core in the path:

C:\git\bug\test>where git
C:\Program Files\Git\cmd\git.exe

C:\git\bug\test>git staTUS
fatal: cannot handle staTUS.exe as a builtin

C:\git\bug\test>git staTUS .
fatal: cannot handle staTUS.exe as a builtin

C:\git\bug\test>git-status
'git-status' is not recognized as an internal or external command,
operable program or batch file.

To check the process call chain in Process Explorer, I replaced git-status.exe with an executable that sleeps for 30 seconds. Here is the result for executing git staTUS .:

Path [Command line]
C:\Program Files\Git\cmd\git.exe [git  staTUS .]
- C:\Program Files\Git\mingw64\bin\git.exe [git.exe  staTUS .]
-- C:\Program Files\Git\mingw64\libexec\git-core\git-staTUS.exe [git-staTUS .]

Normally, git-status.exe is a hardlink to C:\Program Files\Git\mingw64\bin\git.exe, consequently triggering the code I linked to earlier (git.c#L641-L655), and bailing out with fatal: cannot handle staTUS.exe as a builtin.

[kgybels]

Problem only happens with the portable version:

Path [Command line]
C:\PortableGit\cmd\git.exe [git  STATus .]
- C:\PortableGit\mingw32\bin\git.exe [git.exe  STATus .]
-- C:\PortableGit\mingw32\libexec\git-core\git-STATus.exe [git-STATus .]
--- C:\PortableGit\mingw32\libexec\git-core\git.exe ["C:\PortableGit\mingw32\libexec\git-core\git.exe" STATus .]
---- C:\PortableGit\mingw32\libexec\git-core\git-STATus.exe [git-STATus .]
----- C:\PortableGit\mingw32\libexec\git-core\git.exe ["C:\PortableGit\mingw32\libexec\git-core\git.exe" STATus .]
------ ...

[dscho]

Problem only happens with the portable version:

... or when hardlinks are not available (such as on FAT drives).

What do you think about my proposed patch?

[kgybels]

@dscho When not hard linking, instead of doing a copy, the wrapper is used to save disk space?

If the wrapper wouldn't change the command line, then the recursion would be broken by the main git executable.

Instead of the following process call chain:

Path [Command line]
C:\PortableGit\mingw32\libexec\git-core\git-STATus.exe [git-STATus .]
- C:\PortableGit\mingw32\libexec\git-core\git.exe ["C:\PortableGit\mingw32\libexec\git-core\git.exe" STATus .]

It should be like:

Path [Command line]
C:\PortableGit\mingw32\libexec\git-core\git-STATus.exe [git-STATus .]
- C:\PortableGit\mingw32\libexec\git-core\git.exe [git-STATus .]

argv[0] will become git-STATus, and the main git executable will recognize it as trying to execute a builtin, without falling back to calling git-STATus. Would this solution be possible?

We could even make a purpose build executable to use as a hard link stand-in, that would just execute the target executable passing the command line unchanged. This will save even more disk space then using the current wrapper, which serves more purposes than just being a hard link substitute.

Another solution would be to just remove all the git-<builtin> files from git-core. What purpose do they still serve? git-core is not in the path, so calling git-status doesn't work and calling git status will just handle it as a builtin without calling git-status.

@dscho What do you think about my proposed patch?

It feels like a workaround for a problem that shouldn't be there in the first place.

[kgybels]

@dscho I tried making a hard link stand-in executable, just calling CreateProcess for the target executable, forwarding the command line. However, to my surprise it didn't work with git-status.exe replaced with my new executable:

C:\git\bug\test>git statUS .
git: '.' is not a git command. See 'git --help'.

The most similar command is
        d

Even though I could see I was correctly calling C:\PortableGit\mingw32\libexec\git-core\git.exe with command line git-STATus .

I found that argv[0] is always set to the executable path in msc_startup:

	/*
	 * Create a UTF-8 version of w_argv. Also create a "save" copy
	 * to remember all the string pointers because parse_options()
	 * will remove claimed items from the argv that we pass down.
	 */
	ALLOC_ARRAY(my_utf8_argv, argc + 1);
	ALLOC_ARRAY(save, argc + 1);
	save[0] = my_utf8_argv[0] = wcstoutfdup_startup(buffer, _wpgmptr, maxlen);
	for (k = 1; k < argc; k++)
		save[k] = my_utf8_argv[k] = wcstoutfdup_startup(buffer, w_argv[k], maxlen);
	save[k] = my_utf8_argv[k] = NULL;

The same thing happens in mingw_startup.

This is very unfortunate because setting argv[0] to git-<builtin> is currently the only way to prevent git from falling back to calling a helper executable.

[dscho]

Another solution would be to just remove all the git- files from git-core.

That had been proposed, but the core Git maintainer states that a Git without those dashed forms of the builtins is not a Git.

git-core is not in the path

It is as soon as git.exe prepended it to the PATH (which it does).

argv[0] will become git-STATus

If argv[0] becomes a non-absolute path, we're screwed. Try git rebase. It won't find /mingw64/libexec/git-core/git-rebase. Problem.

And we cannot solve that by patching cmd/git.exe, as we call the git.exe in /mingw64/bin/ in Git Bash.

So I am afraid that even if my "ugly" solution does not meet your bar, it is the one that works...

[tamlin-mike]

I never knew this issue would open up such a can of worms. :-)

@dscho

I am quite surprised that GetModulePath() retains the incorrect case

That makes two of us. I would have expected the Windows module "database" to keep the canonical (on-filesystem) name. Your finding suggests it may be loading N instances of the same binary for N different casing (upper/lower).

So I am afraid that even if my "ugly" solution does not meet your bar, it is the one that works...

Between a rock and a hard place, I think even an ugly solution (since it seems it may be impossible to get an elegant solution on Windows) is better than "fork-bomb".

[dscho]

I think @kgybels has a point, and if we can join efforts with the "RUNTIME_PREFIX everywhere" patch series, we probably can avoid my ugly work-around.

[dscho]

@kgybels I integrated @danjacques' and my RUNTIME_PREFIX work into our master branch, as per 6e007ce. So now you should be unblocked. Thank you for your patience!

[dscho]

Oh, and of course I am reopening the issue!

[kgybels]

@dscho I integrated @danjacques' and my RUNTIME_PREFIX work into our master branch, as per 6e007ce. So now you should be unblocked. Thank you for your patience!

You guys already did all the work that needs to be done for git itself, now only need to merge git-for-windows/MINGW-packages#23 to resolve the problem.

[maxxd]

It seems that I am experiencing a problem possibly related to this with Git 2.20.1.windows.1 running on Windows 10 1809 with NTFS. I left a comment on #1855 which appeared to be a possible match as well.

In my case, any call to the executable C:\Program Files\Git\cmd\git.exe or C:\Program Files\Git\bin\git.exe results in this "fork bomb" (calling /mingw64/bin/git via Git Bash works as expected). When executed from PowerShell or cmd.exe the failure is silent, but I get the 0xc0000017 error if I execute the C:\Program Files\Git\cmd\git.exe in the Windows Explorer "Run" dialog or double click the executable from Explorer.

Both bash.exe and sh.exe in the bin directory work when executed from a native Windows context (Explorer, cmd.exe, or PowerShell). In the cmd directory git, git-gui, gitk, and git-lfs all fail in the above "fork bomb" type of manner.

Setting GIT_TRACE 1 has no visible output. Setting GIT_TRACE to a file starting with "/" per the code did not show any trace file created.

Update: I have two systems with mostly the same software and verified the sha1 of the problematic git.exe as identical on both the working and non-working system. I will leave the other system in a broken state to investigate potential differences.

Update 2: If I connect to the system with the "fork bomb" behavior via Remote Desktop git works fine and there is no "fork bomb" behavior. If I log in locally to the physical machine, the "fork bomb" behavior resumes.

[dscho]

@maxxd this sounds like a very different problem you have than the one reported in this ticket. If I had to bet, I would put my money on an anti-malware going haywire.

[maxxd]

@dscho I agree, the common symptom is the child process bomb, but not the same executable. In the case I ran into, it was the wrapper repeatedly being executed, not the actual git executable. Going back to 2.19 resolved the issue I was experiencing, and since this was reported against 2.15/2.16 that is another difference.

Additionally, I also traced the source of the problem that I experienced to a commit about 2 months ago. Specifically, line 66 of this commit of Dec 8, 2018 added the wcsncpy(exepath, realexe + 4, dwlen - 4); which requires manual null termination and didn't have it.

git-for-windows/MINGW-packages@521a655#diff-764b20bf126c245c501c9f87566af95cR66

So I would agree. Unrelated. I'll try the portable version on my systems and see if I can repro this one.

[maxxd]

@dscho I tried and was able to reproduce this with PortableGit-2.16.1-64-bit.7z.exe with a sha1 of 1760b6d7db8dc93a82cdb70418f093e55468e913 on my system.

I was unable to reproduce it with PortableGit-2.20.1-64-bit.7z.exe with a sha1sum of 7c2fc81c3530c898749797787eb3e728b8d0d477 nor was I able to reproduce it with the clean installed version of git 2.20.1, nor my patched version of 2.20.1

So, confirmed. different issue

[dscho]

@maxxd thanks for following up!