Handle Ctrl+C interupts for Mingw Executables #31

naveen521kk · 2021-03-15T17:13:36Z

cherry-picked from

there were conflicts while picking tried my best to fix them say if they were wrong 😉

Fixes #29

dscho · 2021-03-15T18:49:26Z

Could you squash the last commit into the one wit the incorrect merge conflict resolution?

Also, could you download https://github.com/msys2/msys2-runtime/suites/2262965199/artifacts/47141681 and verify that it does, indeed, fix the Ctrl+C problem?

naveen521kk · 2021-03-15T20:13:10Z

Also, could you download https://github.com/msys2/msys2-runtime/suites/2262965199/artifacts/47141681 and verify that it does, indeed, fix the Ctrl+C problem?

Could you help me on how to install it?

naveen521kk · 2021-03-16T05:02:15Z

I think I have to cherry-pick from git-for-windows/msys2-runtime@25c4c01 also

dscho · 2021-03-16T08:10:03Z

Also, could you download https://github.com/msys2/msys2-runtime/suites/2262965199/artifacts/47141681 and verify that it does, indeed, fix the Ctrl+C problem?

Could you help me on how to install it?

Yes. You could simply extract this over an existing MSYS2 installation, but all you really want is to replace the usr/bin/msys-2.0.dll file (you cannot do that from inside MSYS2, though, because that DLL cannot be in use while you replace it).

I think I have to cherry-pick from git-for-windows/msys2-runtime@25c4c01 also

Yes, this is crucial to be able to interrupt processes running in a different CPU architecture than the MSYS2 runtime itself.

BTW I think 2153eb0 is not needed: it tries to export a function that is no longer an exported function, but an inline function instead.

About 42f68af... I dropped it from Git for Windows. The rationale is that the processes that are getting interrupted need to make sure themselves that their child processes are reaped, too.

naveen521kk · 2021-03-16T08:38:48Z

It looks like it is working for me. I did a new install from tar archive and pasted the contents of the zip file on that and opened the mingw64.exe. Then I run pacman -S mingw-w64-x86_64-python
and then python -c "import time;time.sleep(40)", and pressing Ctrl+C printed a traceback exited, which means it was killed correctly.

$ python -c "import time;time.sleep(40)"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
KeyboardInterrupt

I will look into your comments and change things soon.

naveen521kk · 2021-03-16T09:47:16Z

BTW I think 2153eb0 is not needed: it tries to export a function that is no longer an exported function, but an inline function instead.
About 42f68af... I dropped it from Git for Windows. The rationale is that the processes that are getting interrupted need to make sure themselves that their child processes are reaped, too.

I am dropping 2153eb0 and 42f68af

naveen521kk · 2021-03-16T10:15:52Z

Also, it works. But randomly triggers AntiVirus saying it does something malicious and deletes bash.exe. While this doesn't happen with git-for-windows one, I wonder why? Maybe I missed any patches? Or AV ignores the Program Files directory things?

dscho · 2021-03-16T11:38:09Z

Also, it works. But randomly triggers AntiVirus saying it does something malicious and deletes bash.exe. While this doesn't happen with git-for-windows one, I wonder why? Maybe I missed any patches? Or AV ignores the Program Files directory things?

Well, the code does inject a remote thread... So I can see why anti-malware would be triggered, even if it is wrong.

And I have no idea why Git for Windows does not trigger this... I have no insight into how these anti-malware programs work.

naveen521kk · 2021-03-16T11:44:37Z

That shouldn't be a problem for me because I have excluded it from scanning the Msys2 directory.
(All this happened because I did a new install for testing this, or else I wouldn't have told about this.)

dscho · 2021-03-16T11:46:08Z

Hey, but here's an idea: we already use those getprocaddr helpers to look up the address of ExitProcess() and CtrlRoutine(). If we now teach getprocaddr to also inject the remote thread, then the anti-malware might trigger on just those two small programs, and they can be excluded specifically from the anti-malware (and if they're quarantined, we can still fall back to TerminateProcess()).

What do you think, @naveen521kk?

naveen521kk · 2021-03-16T11:55:59Z

Hey, but here's an idea: we already use those getprocaddr helpers to look up the address of ExitProcess() and CtrlRoutine(). If we now teach getprocaddr to also inject the remote thread, then the anti-malware might trigger on just those two small programs, and they can be excluded specifically from the anti-malware (and if they're quarantined, we can still fall back to TerminateProcess()).

Should be better.
But still excluding those two programs seems useless to me. I once had pacman.exe removed when I was doing an update and AV thought it as ransomware and quarantined it. So, full msys2 installation needs to be excluded anyhow.

dscho · 2021-03-16T12:14:20Z

Hey, but here's an idea: we already use those getprocaddr helpers to look up the address of ExitProcess() and CtrlRoutine(). If we now teach getprocaddr to also inject the remote thread, then the anti-malware might trigger on just those two small programs, and they can be excluded specifically from the anti-malware (and if they're quarantined, we can still fall back to TerminateProcess()).

Should be better.
But still excluding those two programs seems useless to me. I once had pacman.exe removed when I was doing an update and AV thought it as ransomware and quarantined it. So, full msys2 installation needs to be excluded anyhow.

I think that is just a bug if anti-malware quarantines pacman.exe.

But with the injected remote thread, I think anti-malware is "less wrong"... So in my mind, if we do it right, and if anti-malware is not overly overzealous, it should be enough to exclude those two helpers (or alternatively, hope that anti-malware developers will analyze the helpers, verify that they are mostly harmless, and add patterns that match those use cases specifically to avoid triggering on them).

naveen521kk · 2021-03-16T12:21:11Z

I think that is just a bug if anti-malware quarantines pacman.exe.

Don't know, it was just once after that it didn't do anything there.

But with the injected remote thread, I think anti-malware is "less wrong"... So in my mind, if we do it right, and if anti-malware is not overly overzealous, it should be enough to exclude those two helpers (or alternatively, hope that anti-malware developers will analyze the helpers, verify that they are mostly harmless, and add patterns that match those use cases specifically to avoid triggering on them).

Right. If they analyze bash.exe that would be useless for them as well, but there are possibilities that the AV could exclude those two helpers.
Maybe why not try?

naveen521kk · 2021-03-17T17:31:29Z

I will try to work on that tomorrow.

naveen521kk · 2021-03-19T19:12:55Z

I will try to work on that tomorrow.

Looks like my knowledge with C and win32 API is so poor to do this. I think I won't be able to do it, sorry.

naveen521kk · 2021-04-19T18:12:07Z

@dscho after a long time, today I gave another attempt. e3eb220

I don't know any way's to debug it, if you know please tell, it seems to not work finally when I downloaded the artifacts and tried it on new install.

naveen521kk · 2021-04-19T19:13:30Z

I was able to confirm by compiling the source of getprocaddr separately that it works.
For example, I tried python -c "import time;time.sleep(1000) in shell and then I run

test CtrlRoutine 0 2440

where test.exe is the compiled executable and 2440 is from tasklist command corresponding to python.exe. Python terminated with

Traceback (most recent call last):
  File "<string>", line 1, in <module>
KeyboardInterrupt

which shows that it works.

naveen521kk · 2021-04-19T19:27:15Z

Here I pushed again, I think it should work?

winsup/cygwin/exceptions.cc

naveen521kk · 2021-04-20T16:38:30Z

Yes finally, this works 🎉

dscho

This looks pretty good already!

Eventually, I would love to break this change apart and fold it into the earlier commits (feel free to mark yourself as co-author via the Co-authored-by: footer) because that will make it easier to maintain the MSYS2-specific patches on top of Cygwin.

winsup/cygwin/include/cygwin/exit_process.h

winsup/utils/getprocaddr.c

winsup/cygwin/include/cygwin/exit_process.h

The idea is that we handle Ctrl+C not by terminating the processes (which would reflect `kill -KILL`, not `kill -INT`) but by imitating what Windows does in a CMD window: we inject a remote thread that runs the `CtrlRoutine` function. This lets the `atexit()` handlers and the ConsoleCtrlEvent handlers do their job. This commit brings the goodness from msys2/msys2-runtime#31 over into MSYS2-packages, which cherry-picked and edited the patches from Git for Windows. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho · 2021-06-18T11:10:55Z

@naveen521kk congratulations, and let me say this: I am very impressed how you kept working on this for three months, with a really nice end result. Thank you!

naveen521kk · 2021-06-18T11:40:35Z

Thank you for reviewing. This helped me to learn C and about Win32 APIs; without you, this will not be possible :).

jeremyd2019 · 2021-06-18T22:44:49Z

on arm, running mintty, running clangarm64 python, hitting ctrl-c kills python (after a significant delay). from windows terminal ctrl-c shows KeyboardInterrupt as expected. this seems to be expected behavior for lack of arm binaries for the libexec binaries

jeremyd2019 · 2021-06-18T22:45:59Z

question: doesn't cygwin provide kill(2)? Why does kill util need patching rather than just the kill syscall within cygwin?

jeremyd2019 · 2021-06-19T02:16:10Z

Does this work on win7? I could only get python to get a keyboard interrupt if run through winpty. Otherwise, sometimes python would die, but leave things in a very broken state that never got back to bash. It appears getprocaddress32 (or 64) hangs around in taskmgr for many seconds, and then both it and python die.

The only reason I tried win7 was to make sure my proposed change to detect arm processes wouldn't blow up there, but I'm seeing the same behavior with that as with the version currently in staging

On Windows 10 I get keyboard interrupt AND python exits when attempting to interrupt time.sleep from interactive python

EDIT: I'm sometimes seeing the 'broken' behavior on Win10 too... It appears the 'forked' bash process that supposedly 'execed' python is hanging around. Killing it returns to the shell.

naveen521kk · 2021-06-21T11:57:28Z

Hmm, I tried to install this version from Pacman(staging repo) today, and I found it wasn't working, there was a long hang and nothing worked, I had to close the Window. I tried the same pip.exe I created previously.
pip.zip

It was working previously and also works on a fresh installation but not on my old one, and that looked weird.

So, to debug I downloaded Process Monitor and started to check whether getprocaddr64.exe was called. Everything looked normal, except the below one

Parent PID: 8528, Command line: C:\msys64\usr\libexec\getprocaddr64.exe CtrlRoutine 0 13267416829, Current directory: H:\mingw\test_programs\likefinal\, Environment: 
	MSYSTEM=MINGW64
	PATH=...
	SYSTEMDRIVE=C:
	SYSTEMROOT=C:\Windows
	WINDIR=C:\Windows

When starting getprocaddr64.exe runtime is calling/passing a significantly large PID 13267416829, will this even happen?
In procmon the PID for pip.exe was 7344 and python.exe was 2228.

Next up, this one

PID: 7720, Command line: C:\msys64\usr\libexec\getprocaddr64.exe ExitProcess 130 132674168

Again, a large PID but different from the previous one. As expected getprocaddr64.exe exited with status 1.
This happened twice and then finally, pip.exe was killed using TerminateProcess still python.exe is alive though and terminal was hanged(maybe the forked process executing python was hanging around ).

I wondering why this happens that too only in the main installation.

naveen521kk · 2021-06-21T12:23:35Z

Hmm, the same number comes up

PID: 2720, Command line: C:\msys64\usr\libexec\getprocaddr64.exe CtrlRoutine 0 13267416829

pip.exe pid -> 8332

Next, I tried placing the build from this PR on the tree and now it still hangs but the numbers changed and I think something is going wrong

PID: 8092, Command line: C:\msys64\usr\libexec\getprocaddr64.exe CtrlRoutine 0 13268512904

Even on second time it was same number.

dscho · 2021-06-21T12:26:09Z

That is really strange. I thought maybe the pid was formatted using the wrong printf() format, but no, it is a DWORD and it is rendered using %ld... Puzzled.

jeremyd2019 · 2021-06-21T17:17:49Z

winsup/cygwin/include/cygwin/exit_process.h

+                  | PROCESS_VM_OPERATION | PROCESS_VM_WRITE | PROCESS_VM_READ,
+              FALSE, pids[i]);
+          if (!process)
+            process = OpenProcess (PROCESS_TERMINATE, FALSE, pids[i]);


Should probably have PROCESS_QUERY_LIMITED_INFORMATION here too, to be able to query PID and exit code. Assuming nobody here cares about anything older than Vista, on XP you need PROCESS_QUERY_INFORMATION.

naveen521kk · 2021-06-21T17:37:51Z

Does this work on win7? I could only get python to get a keyboard interrupt if run through winpty. Otherwise, sometimes python would die, but leave things in a very broken state that never got back to bash. It appears getprocaddress32 (or 64) hangs around in taskmgr for many seconds, and then both it and python die.

Seems related, I was also testing on win7. Though in my case getprocaddr*.exe didn't hang instead it got a wrong argument.

naveen521kk · 2021-06-21T18:35:52Z

Hmm, I just downloaded https://github.com/msys2/msys2-installer/releases/download/nightly-x86_64/msys2-base-x86_64-latest.tar.xz extracted it added the Pacman staging repo and updated runtime. There it is worked perfectly

$ /h/mingw/test_programs/likefinal/pip.exe
start
Traceback (most recent call last):
  File "H:\mingw\test_programs\likefinal\pip-script.py", line 34, in <module>
    time.sleep(4000)
KeyboardInterrupt

So, I think it isn't a problem with code here.
Maybe something messed up with the original installation?

jeremyd2019 · 2021-06-21T18:40:25Z

That doesn't make any sense, msys2-runtime is/should be self-contained

jeremyd2019

Going through this code looking for something that could be causing issues...

jeremyd2019 · 2021-06-21T19:04:40Z

winsup/cygwin/include/cygwin/exit_process.h

+  size_t len = wcslen (wbuf) + wcslen (function_name) + 3 /* exit code */
+               + 1 /* space */ + 10 /* process ID, i.e. DWORD */ + 1 /* NUL */;


I think this is missing 2 spaces (space between wbuf and function_name, and space between function_name and exit_code. Also, is it safe to assume exit_code will always only be 3 digits? It seems so because the callers use exit_code & 0x7f, or else CTRL_C_EVENT/CTRL_BREAK_EVENT which are 0 or 1 resp.

jeremyd2019 · 2021-06-21T19:05:15Z

winsup/cygwin/include/cygwin/exit_process.h

+  PROCESSENTRY32 entry;
+  DWORD pids[16384];
+  int max_len = sizeof (pids) / sizeof (*pids), i, len, ret = 0;
+  pid_t pid = GetProcessId (main_process);


Why is this pid_t and not DWORD? This is a Windows PID, not a cygwin PID.

jeremyd2019 · 2021-06-21T19:05:35Z

winsup/cygwin/include/cygwin/exit_process.h

+  pid_t pid = GetProcessId (main_process);
+  int signo = exit_code & 0x7f;
+
+  pids[0] = (DWORD)pid;


Then this cast would be unnecessary

jeremyd2019 · 2021-06-21T19:13:18Z

winsup/cygwin/include/cygwin/exit_process.h

+      if (orig_len == len || len >= max_len)
+        break;
+    }
+


CloseHandle(snapshot); ?

jeremyd2019 · 2021-06-21T19:19:36Z

I guess now that this is merged I should make my own pull request instead of review comments suggesting code 😁

From my post-merge review at msys2#31 (review)

kill_via_console_helper: use %u instead of %ld for DWORD. It appears that cygwin's swprintf is acting like LP64 instead of Windows LLP64, and expecting a 64-bit long. DWORDs are 32-bit unsigned. Minor fixes for Ctrl-C handling code. From my post-merge review at msys2#31 (review)

kill_via_console_helper: use %u instead of %ld for DWORD. It appears that cygwin's swprintf is acting like LP64 instead of Windows LLP64, and expecting a 64-bit long. DWORDs are 32-bit unsigned. Add check to avoid closing the handle from the input paramter. It may well be that the caller still intends to use it. Minor fixes for Ctrl-C handling code. From my post-merge review at msys2#31 (review)

kill_via_console_helper: use %u instead of %ld for DWORD. It appears that cygwin's swprintf is acting like LP64 instead of Windows LLP64, and expecting a 64-bit long. DWORDs are 32-bit unsigned. Add check to avoid closing the handle from the input paramter. It may well be that the caller still intends to use it. Minor fixes for Ctrl-C handling code. From my post-merge review at #31 (review)

naveen521kk force-pushed the ctrl-c-handle branch from 84e3564 to f867abc Compare March 15, 2021 20:09

naveen521kk force-pushed the ctrl-c-handle branch from f867abc to 1fd1ca2 Compare March 16, 2021 05:29

naveen521kk force-pushed the ctrl-c-handle branch from 1fd1ca2 to 002db58 Compare March 16, 2021 09:47

naveen521kk force-pushed the ctrl-c-handle branch from e3eb220 to 92f5113 Compare April 19, 2021 19:09

naveen521kk force-pushed the ctrl-c-handle branch from 92f5113 to 7ae17f7 Compare April 19, 2021 19:25

naveen521kk commented Apr 20, 2021

View reviewed changes

winsup/cygwin/exceptions.cc Show resolved Hide resolved

naveen521kk force-pushed the ctrl-c-handle branch 4 times, most recently from 935ecea to 336d4c7 Compare April 20, 2021 16:21

dscho reviewed Apr 20, 2021

View reviewed changes

dscho merged commit d8b7c36 into msys2:msys2-3_2_0-release Jun 18, 2021

naveen521kk deleted the ctrl-c-handle branch June 18, 2021 11:03

dscho mentioned this pull request Jun 18, 2021

msys2-runtime: handle Ctrl+C with MINGW executables msys2/MSYS2-packages#2547

Merged

github-actions bot mentioned this pull request Jun 18, 2021

[New msys2-runtime-package version] msys2-runtime: handle Ctrl+C with MINGW executables git-for-windows/git#3279

Closed

jeremyd2019 reviewed Jun 21, 2021

View reviewed changes

jeremyd2019 added a commit to jeremyd2019/msys2-runtime that referenced this pull request Jun 21, 2021

Minor fixes for Ctrl-C handling code.

5a4b277

From my post-merge review at msys2#31 (review)

jeremyd2019 mentioned this pull request Jun 21, 2021

Fixes for Ctrl-C handling code. #48

Merged

github-actions bot mentioned this pull request Jun 22, 2021

[New msys2-runtime version] Fix 64 vs 32-bit type confusion in swprintf. git-for-windows/git#3288

Closed

lazka mentioned this pull request Nov 5, 2023

kill() / signal() doesn't really work. #176

Open

		size_t len = wcslen (wbuf) + wcslen (function_name) + 3 /* exit code */
		+ 1 /* space / + 10 / process ID, i.e. DWORD / + 1 / NUL */;

Handle Ctrl+C interupts for Mingw Executables #31

Handle Ctrl+C interupts for Mingw Executables #31

Conversation

naveen521kk commented Mar 15, 2021 • edited Loading

dscho commented Mar 15, 2021

naveen521kk commented Mar 15, 2021

naveen521kk commented Mar 16, 2021

dscho commented Mar 16, 2021

naveen521kk commented Mar 16, 2021

naveen521kk commented Mar 16, 2021

naveen521kk commented Mar 16, 2021

dscho commented Mar 16, 2021

naveen521kk commented Mar 16, 2021

dscho commented Mar 16, 2021

naveen521kk commented Mar 16, 2021 • edited Loading

dscho commented Mar 16, 2021

naveen521kk commented Mar 16, 2021

naveen521kk commented Mar 17, 2021

naveen521kk commented Mar 19, 2021

naveen521kk commented Apr 19, 2021

naveen521kk commented Apr 19, 2021 • edited Loading

naveen521kk commented Apr 19, 2021

naveen521kk commented Apr 20, 2021

dscho left a comment

Choose a reason for hiding this comment

dscho commented Jun 18, 2021

naveen521kk commented Jun 18, 2021

jeremyd2019 commented Jun 18, 2021

jeremyd2019 commented Jun 18, 2021

jeremyd2019 commented Jun 19, 2021 • edited Loading

naveen521kk commented Jun 21, 2021 • edited Loading

naveen521kk commented Jun 21, 2021

dscho commented Jun 21, 2021

jeremyd2019 Jun 21, 2021

Choose a reason for hiding this comment

naveen521kk commented Jun 21, 2021

naveen521kk commented Jun 21, 2021 • edited Loading

jeremyd2019 commented Jun 21, 2021

jeremyd2019 left a comment

Choose a reason for hiding this comment

jeremyd2019 Jun 21, 2021

Choose a reason for hiding this comment

jeremyd2019 Jun 21, 2021

Choose a reason for hiding this comment

jeremyd2019 Jun 21, 2021

Choose a reason for hiding this comment

jeremyd2019 Jun 21, 2021

Choose a reason for hiding this comment

jeremyd2019 commented Jun 21, 2021

naveen521kk commented Mar 15, 2021 •

edited

Loading

naveen521kk commented Mar 16, 2021 •

edited

Loading

naveen521kk commented Apr 19, 2021 •

edited

Loading

jeremyd2019 commented Jun 19, 2021 •

edited

Loading

naveen521kk commented Jun 21, 2021 •

edited

Loading

naveen521kk commented Jun 21, 2021 •

edited

Loading