Re: [PATCH 1/5] Cygwin: Make 'ulimit -c' control writing a coredump

[Jon Turney <jon.turney@dronecode.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

On 12/01/2024 14:09, Jon Turney wrote:
> Pre-format a command to be executed on a fatal error to run 'dumper'
> (using an absolute path).
> 
> Factor out executing a pre-formatted command, so we can use that for
> invoking the JIT debugger in try_to_debug() (if error_start is present
> in the CYGWIN env var) and to invoke dumper when a fatal error occurs.
> 

So, there is a small problem with this change: because dumper itself 
terminates the dumped process, it doesn't go on to exit with the 
signal+128 exit status.

(In fact, it seems to exit with status 0 when terminated by an attached 
debugger terminating, which isn't great)

That's relatively easy to fix: just use the '-n' option to dumper so it 
detaches before exiting, to prevent that terminating the dumped process, 
but then we run into the difficulties of reliably detecting that dumper 
has attached and done it's work, so it's safe for us to exit.

Attached patch does that, and documents the expectations on the 
error_start command a bit more clearly.

Even then this is clearly not totally bullet-proof. Maybe the right 
thing to do is add a suitable timeout here, so even if we fail to notice 
the DebugActiveProcess() (or there's a custom JIT debugger which just 
writes the fact a process crashed to a logfile or something), we'll exit 
eventually?

[-- Attachment #2: 0001-Cygwin-Don-t-terminate-via-dumper.patch --]
[-- Type: text/plain, Size: 2715 bytes --]

From 85120a1697294cd93ff68f6b1840145251ce4185 Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Tue, 16 Jan 2024 16:12:51 +0000
Subject: [PATCH] Cygwin: Don't terminate via dumper

A process which is exiting due to a core dumping signal doesn't
propagate the correct exist status after dumping core, because 'dumper'
itself forcibly terminates the process.

Use 'dumper -n' to avoid killing the dumped process, so we continue to
the end of signal_exit() , to exit with the 128+signal exit status.

Busy-wait in exec_prepared_command() in an attempt to reliably notice
the dumper attaching, so we don't get stuck there.

Also: document these important facts for custom uses of error_start.
---
 winsup/cygwin/exceptions.cc | 7 +++----
 winsup/doc/cygwinenv.xml    | 6 ++++++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 8b1c5493e..0e1a804ca 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -149,7 +149,7 @@ dumper_init (void)
 
   /* Calculate the length of the command, allowing for an appended DWORD PID and
      terminating null */
-  int cmd_len = 1 + wcslen(dll_dir) + 11 + 2 + 1 + wcslen(global_progname) + 1 + 10 + 1;
+  int cmd_len = 1 + wcslen(dll_dir) + 11 + 5 + 1 + wcslen(global_progname) + 1 + 10 + 1;
   if (cmd_len > 32767)
     {
       /* If this comes to more than the 32,767 characters CreateProcess() can
@@ -163,7 +163,7 @@ dumper_init (void)
   cp = wcpcpy (cp, L"\"");
   cp = wcpcpy (cp, dll_dir);
   cp = wcpcpy (cp, L"\\dumper.exe");
-  cp = wcpcpy (cp, L"\" ");
+  cp = wcpcpy (cp, L"\" -n ");
   cp = wcpcpy (cp, L"\"");
   cp = wcpcpy (cp, global_progname);
   wcscat (cp, L"\"");
@@ -570,9 +570,8 @@ int exec_prepared_command (PWCHAR command)
     system_printf ("Failed to start, %E");
   else
     {
-      SetThreadPriority (GetCurrentThread (), THREAD_PRIORITY_IDLE);
       while (!being_debugged ())
-	Sleep (1);
+	Sleep (0);
       Sleep (2000);
     }
 
diff --git a/winsup/doc/cygwinenv.xml b/winsup/doc/cygwinenv.xml
index d97f2b77d..05672c404 100644
--- a/winsup/doc/cygwinenv.xml
+++ b/winsup/doc/cygwinenv.xml
@@ -46,6 +46,12 @@ to the command as arguments.
   Note: This has no effect if a debugger is already attached when the fatal
   error occurs.
 </para>
+<para>
+  Note: The command invoked must either (i) attach to the errored process with
+  <function>DebugActiveProcess()</function>, or (ii) forcibly terminate the
+  errored process (with <function>TerminateProcess()</function> or similar), as
+  otherwise the errored process will wait forever for a debugger to attach.
+</para>
 </listitem>
 
 <listitem>
-- 
2.43.0