Git Bash: support running on ARM64 without ARM64 binaries again by dscho · Pull Request #46 · git-for-windows/MINGW-packages

dscho · 2021-02-12T12:31:31Z

I have no way to test this. @dennisameling, could you?

By mistake, a camelCased function name was introduced. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dennisameling · 2021-02-12T13:48:23Z

mingw-w64-git/git-wrapper.c

+				WCHAR msystem_bin2[64];
+
+				/* Maybe we are on ARM64 but don't have the binaries? */
+				swprintf(msystem_bin2, sizeof(msystem_bin2), L"mingw%d\\bin", (int) sizeof(void *) * 8);


Shouldn't this check for the arm64\bin folder instead? If that doesn't exist, we're running on ARM64 but don't have the binaries, correct?

Yes, and we're already in the clause where we determined that arm64/bin does not exist. So we're falling back, looking for mingw<bitness>/bin here.

dennisameling · 2021-02-12T13:49:01Z

Sure, will test later today!

dscho · 2021-02-12T13:49:51Z

Sure, will test later today!

Thank you!

dscho · 2021-02-12T13:54:42Z

Sure, will test later today!

Thank you!

BTW I should have said this earlier: I am trying a slightly patched git-artifacts.yml to produce the corresponding installer/portable artifacts: https://github.com/dscho/git/runs/1887544878?check_suite_focus=true. Maybe that makes it quicker for you to test?

dennisameling · 2021-02-12T20:29:33Z

Holy moly - just tested 32-bit Git and can confirm the fork bomb. Not good!

Just about to test this PR now

dennisameling · 2021-02-12T21:56:20Z

Tried with the generated binaries from your CI run (https://github.com/dscho/git/actions/runs/561141041), but the fork bomb is still there unfortunately. Tried building locally based on this PR just to be sure, but then ran into https://github.com/git-for-windows/git-sdk-32/issues/7😞 tried fixing it for an hour by installing/removing all kinds of pacman packages, to no avail

dscho · 2021-02-12T22:19:19Z

can confirm the fork bomb.

BTW which git.exe is this, is it cmd/git.exe that fork-bombs, or is it mingw32/bin/git.exe? From the size 3.3MB I suspect that it might actually be mingw32/bin/git.exe?

dennisameling · 2021-02-12T22:28:46Z

It's C:\Program Files (x86)\Git\bin\git.exe if I open the file location:

dennisameling · 2021-02-12T22:32:02Z

Getting a warning while compiling git-wrapper though:

$ make -C src/git -f ../mingw-w64-git.mak git-wrapper.exe
make: Entering directory '/usr/src/MINGW-packages/mingw-w64-git/src/git'
    * new prefix flags
    CC git-wrapper.o
../git-wrapper.c: In function 'is_running_on_arm64':
../git-wrapper.c:129:3: warning: initialization of 'BOOL (*)(void *, USHORT *, USHORT *)' {aka 'int (*)(void *, short unsigned int *, short unsigned int *)'} from incompatible pointer type 'FARPROC' {aka 'long long int (*)()'} [-Wincompatible-pointer-types]
  129 |   GetProcAddress(GetModuleHandle(L"kernel32"), "IsWow64Process2");
      |   ^~~~~~~~~~~~~~
    RC git.res
    LINK git-wrapper.exe
make: Leaving directory '/usr/src/MINGW-packages/mingw-w64-git/src/git'

dscho · 2021-02-12T22:34:49Z

Okay, so it probably does not detect the correct top-level directory. Maybe patch the git-wrapper.c for testing

Getting a warning while compiling git-wrapper though:

$ make -C src/git -f ../mingw-w64-git.mak git-wrapper.exe
make: Entering directory '/usr/src/MINGW-packages/mingw-w64-git/src/git'
    * new prefix flags
    CC git-wrapper.o
../git-wrapper.c: In function 'is_running_on_arm64':
../git-wrapper.c:129:3: warning: initialization of 'BOOL (*)(void *, USHORT *, USHORT *)' {aka 'int (*)(void *, short unsigned int *, short unsigned int *)'} from incompatible pointer type 'FARPROC' {aka 'long long int (*)()'} [-Wincompatible-pointer-types]
  129 |   GetProcAddress(GetModuleHandle(L"kernel32"), "IsWow64Process2");
      |   ^~~~~~~~~~~~~~
    RC git.res
    LINK git-wrapper.exe
make: Leaving directory '/usr/src/MINGW-packages/mingw-w64-git/src/git'

Yeah, I stumbled over this, too, but it is harmless. I thought that casting to (void (*)(void)) should fix it, but it didn't. Anyway, that's not my main worry now.

The main worry is what is going wrong, and I could imagine that it makes most sense to patch git-wrapper.exe to not actually call CreateProcessW() but instead to print the path to stderr and then exit.

And then you could even compile it using gcc -g -o git-wrapper.exe git-wrapper.c, copy it into a portable Git, and use gdb -args /path/to/bin/git.exe version to debug it.

dennisameling · 2021-02-12T22:57:02Z

Can't we simply do a _waccess check for arm64/bin in the is_running_on_arm64() function? If it isn't found then it'll simply return false/0 and the system will behave as MINGW32 etc. - I'm currently trying to test that theory but as a beginner in C, it's not as easy as it sounds 😅

dscho · 2021-02-12T22:58:40Z

Can't we simply do a _waccess check for arm64/bin in the is_running_on_arm64() function?

We don't know the absolute path, then. Remember, we could call this via absolute path, or via PATH from anywhere.

dscho · 2021-02-12T23:00:01Z

That's why I implemented it at that point: 2ecdd7e

dscho · 2021-02-12T23:06:08Z

BTW we'll want to add this in an extra commit, I think:

diff --git a/mingw-w64-git/git-wrapper.c b/mingw-w64-git/git-wrapper.c
index 130fc1adc..68bc73bd2 100644
--- a/mingw-w64-git/git-wrapper.c
+++ b/mingw-w64-git/git-wrapper.c
@@ -712,7 +712,7 @@ int main(void)
 		is_git_command = 1, full_path = 1, skip_arguments = 0,
 		allocate_console = 0, show_console = -1,
 		append_quote_to_cmdline = 0;
-	WCHAR exepath[MAX_PATH], exe[MAX_PATH], top_level_path[MAX_PATH];
+	WCHAR exepath[MAX_PATH], exe_bup[MAX_PATH], exe[MAX_PATH], top_level_path[MAX_PATH];
 	LPWSTR cmd = NULL, exep = exe, prefix_args = NULL, basename;
 	LPWSTR working_directory = NULL;
 
@@ -729,6 +729,7 @@ int main(void)
 	/* get the installation location */
 	/* GetModuleFileName(NULL, exepath, MAX_PATH); */
 	find_exe_realpath(exepath, MAX_PATH);
+	wcscpy(exe_bup, exepath);
 	if (!PathRemoveFileSpec(exepath)) {
 		fwprintf(stderr, L"Invalid executable path: %s\n", exepath);
 		ExitProcess(1);
@@ -830,6 +831,11 @@ int main(void)
 		}
 	}
 
+	if (!wcscmp(exe_bup, exep)) {
+		fwprintf(stderr, L"BUG (fork bomb): %s\n", exep);
+		ExitProcess(1);
+	}
+
 	{
 		STARTUPINFO si;
 		PROCESS_INFORMATION pi;

dennisameling · 2021-02-12T23:09:04Z

Just added some fprintf's for debugging - it doesn't even reach the code path /* Maybe we are on ARM64 but don't have the binaries? */:

Output is only

Starting initialize_top_level_path...
In while loop
Starting initialize_top_level_path...
In while loop
Starting initialize_top_level_path...
In while loop
....

So the strip_count apparently is 0 or higher

dscho · 2021-02-12T23:15:14Z

Aargh, you're right! We're calling initialize_top_level_path() only with strip_count == 1 when the basename is git.exe. Hrm. Let me think.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

There has been a report that `bin/git.exe` spawns itself over and over again. This is clearly a bug, and to avoid it from making a user's machine sluggish, let's error out early in that case. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

It was reported that the x86 version of Git for Windows v2.30.1 fork bombs on ARM64. The reason is that we are looking for the ARM64 binaries, but they are not bundled in the x86 version. Let's fall be careful before enabling ARM64 mode. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

git-wrapper.c changed, after all. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho · 2021-02-12T23:38:11Z

I think I've got something. If we call is_running_on_arm64() only inside initialize_top_level_path(), and do that by combining it with the check for /arm64/bin, I think we're golden. I pushed a new version; Could I bother you to test again?

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dennisameling · 2021-02-12T23:42:21Z

Nice!! It works without the arm64 folder present now 🚀

Let me just double-check the case when the arm64 is present, give me a few mins...

dennisameling · 2021-02-12T23:50:01Z

Bingo! Also works when the arm64 binaries are present 🎉 Thanks!!

dennisameling

Thanks!

dscho · 2021-02-12T23:52:22Z

Awesome!

I also kicked off a build here: https://github.com/dscho/git/actions/runs/562531927, and intend to let the (private...) reporter test before merging.

dscho · 2021-02-13T00:34:38Z

let the (private...) reporter test before merging.

That test also succeeded.

dscho added 3 commits February 12, 2021 13:09

git-wrapper: use snake case consistently

570642a

By mistake, a camelCased function name was introduced. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

git-wrapper: fix typo

caded3b

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

git-wrapper: fix indentation

bc6b266

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho mentioned this pull request Feb 12, 2021

Add arm64 support to git-wrapper #44

Merged

dscho added a commit to dscho/git that referenced this pull request Feb 12, 2021

TEST: build git-artifacts for git-for-windows/MINGW-packages#46

709ef88

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho added a commit to dscho/git that referenced this pull request Feb 12, 2021

TEST: build git-artifacts for git-for-windows/MINGW-packages#46

0dd1b43

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho added a commit to dscho/git that referenced this pull request Feb 12, 2021

TEST: build git-artifacts for git-for-windows/MINGW-packages#46

2ae2e9c

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

git-wrapper: use C comments, not C++ ones

21f6b5b

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho force-pushed the safer-arm64-support-for-git branch from c2c0990 to 36a87e9 Compare February 12, 2021 13:08

dscho added a commit to dscho/git that referenced this pull request Feb 12, 2021

TEST: build git-artifacts for git-for-windows/MINGW-packages#46

c586795

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dennisameling reviewed Feb 12, 2021

View reviewed changes

dscho added 4 commits February 13, 2021 00:34

git-wrapper: avoid compiler warning when casting a function pointer

41d73fb

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

git-wrapper: avoid fork bombs

473e1ab

There has been a report that `bin/git.exe` spawns itself over and over again. This is clearly a bug, and to avoid it from making a user's machine sluggish, let's error out early in that case. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

git-wrapper: cache the answer to "is this ARM64?"

0b10be1

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

git: update checksum

dd83c3c

git-wrapper.c changed, after all. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho force-pushed the safer-arm64-support-for-git branch from 36a87e9 to dd83c3c Compare February 12, 2021 23:38

dscho added a commit to dscho/git that referenced this pull request Feb 12, 2021

TEST: build git-artifacts for git-for-windows/MINGW-packages#46

4f196d3

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dennisameling approved these changes Feb 12, 2021

View reviewed changes

dscho merged commit 02a34b5 into git-for-windows:main Feb 13, 2021

dscho deleted the safer-arm64-support-for-git branch February 13, 2021 00:35

dscho mentioned this pull request Feb 23, 2021

Git for Windows 2.30.1 cannot run in windows Arm system git-for-windows/git#3052

Closed

dennisameling mentioned this pull request Mar 6, 2021

cmd\git.exe crashing when ARM64 folder is present in Git installation (git-wrapper) git-for-windows/git#3083

Closed

1 task

Conversation

dscho commented Feb 12, 2021

Uh oh!

dennisameling Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

dscho Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

dennisameling commented Feb 12, 2021

Uh oh!

dscho commented Feb 12, 2021

Uh oh!

dscho commented Feb 12, 2021

Uh oh!

dennisameling commented Feb 12, 2021

Uh oh!

dennisameling commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dscho commented Feb 12, 2021

Uh oh!

dennisameling commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dennisameling commented Feb 12, 2021

Uh oh!

dscho commented Feb 12, 2021

Uh oh!

dennisameling commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dscho commented Feb 12, 2021

Uh oh!

dscho commented Feb 12, 2021

Uh oh!

dscho commented Feb 12, 2021

Uh oh!

dennisameling commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dscho commented Feb 12, 2021

Uh oh!

dscho commented Feb 12, 2021

Uh oh!

dennisameling commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dennisameling commented Feb 12, 2021

Uh oh!

dennisameling left a comment

Choose a reason for hiding this comment

Uh oh!

dscho commented Feb 12, 2021

Uh oh!

dscho commented Feb 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dennisameling commented Feb 12, 2021 •

edited

Loading

dennisameling commented Feb 12, 2021 •

edited

Loading

dennisameling commented Feb 12, 2021 •

edited

Loading

dennisameling commented Feb 12, 2021 •

edited

Loading

dennisameling commented Feb 12, 2021 •

edited

Loading