Restore SEM_FAILCRITICALERRORS [was: Aren't Windows System Error popups meant to be disabled in Cygwin?]

[Corinna Vinschen]

On Feb  2 18:22, Corinna Vinschen via Cygwin wrote:
> On Feb  2 14:56, David Allsopp via Cygwin wrote:
> > On Fri, 2 Feb 2024 at 14:18, Corinna Vinschen via Cygwin wrote:
> > > On Feb  2 13:35, David Allsopp via Cygwin wrote:
> > > > Not really suggesting it be done this way (it feels more complicated
> > > > than just reverting the change), but in some ways perhaps Cygwin
> > > > should be using GetErrorMode on startup and instead of not inheriting
> > > > it, ensuring that it sets whatever it received? i.e. just before the
> > > > call to CreateProcess for a non-Cygwin binary, Cygwin restores the
> > > > error mode (for that thread only) to the value read at startup, calls
> > > > CreateProcess and then sets the error mode back.
> > >
> > > This sounds like a good ide, but...
> > >
> > > Is it actually a safe bet that the error mode set by SetThreadErrorMode
> > > is then propagated as process error mode to the child process?
> > >
> > > I have to ask that because Microsoft conveniently forgot to document
> > > this scenario in the MSDN docs.
> > 
> > :o) Never knowingly clear, are they! It would seem to be the intent of
> > SetThreadErrorMode that it would behave that way but who knows.
> > 
> > Happy to set up a quick experiment to check that it does work (i.e.
> > the invoked process has GetErrorMode set as expected) and that there's
> > no possible race between two threads in the calling process with
> > differing values (i.e. that having SEM_FAILCRITICALERRORS in one
> > thread and not in another behaves as expected. If it does appear to
> > work consistently, would you be willing to go down this route? Happy
> > to do the patch, although it'd be very helpful to have a couple of
> > pointers: I'm guessing the value would want to be captured just before
> > the one place where SetErrorMode is already called, but in which
> > structure should it then be stashed away to be reused in spawn?
> 
> Wanna try this?  It ignores the case of starting a process
> under another user account for now, but that can be added easily
> if this proves to work as expected.

During dinner it occured to me that I forgot to remove setting the
default error mode via CreateProcess.  So this patch would have
to be applied as well:

diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 8a2db5cf72e2..2e0f0fcf2146 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -401,13 +401,6 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
 
       c_flags |= CREATE_SEPARATE_WOW_VDM | CREATE_UNICODE_ENVIRONMENT;
 
-      /* Add CREATE_DEFAULT_ERROR_MODE flag for non-Cygwin processes so they
-	 get the default error mode instead of inheriting the mode Cygwin
-	 uses.  This allows things like Windows Error Reporting/JIT debugging
-	 to work with processes launched from a Cygwin shell. */
-      if (!real_path.iscygexec ())
-	c_flags |= CREATE_DEFAULT_ERROR_MODE;
-
       /* We're adding the CREATE_BREAKAWAY_FROM_JOB flag here to workaround
 	 issues with the "Program Compatibility Assistant (PCA) Service".
 	 For some reason, when starting long running sessions from mintty(*),

However, it occured to me that this won't work at all.

Consider fork/exec.  In Windows terms, the forked process is a child
process started with CreateProcess.  The forked child is a Cygwin
process, so at DLL init time, it sets

  orig_proc_error_mode = SetErrorMode (SEM_FAILCRITICALERRORS
                                       | SEM_NOGPFAULTERRORBOX);

However, given the parent is always a Cygwin parent, orig_proc_error_mode
will always be SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX.

The following exec starting a non-Cygwin process will set the thread
error mode to the above value.  So any non-Cygwin process started from
your typical Cygwin process like bash, will always have the error mode
set to the same mode as any Cygwin application.

Ultimately this means, the effect is the same as if we had just reverted
commit 21ec498d7f912 ("cygwin: use CREATE_DEFAULT_ERROR_MODE in spawn").


Corinna