Re: [PATCH 00/11] More testsuite fixes

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

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

On 13/07/2023 12:38, Jon Turney wrote:
> 
> cancel11: some funkiness I can't work out, causing the save/restoring signal handlers around system() to not
> work correctly

So, the test here: is the SIGINT handle restored correctly if the thread 
executing system() is cancelled. This test fails, because it's not.

It seems like that scenario was explicitly considered when this test was 
added in https://cygwin.com/pipermail/cygwin-patches/2003q1/003378.html

I think maybe this is a regression introduced in 
https://cygwin.com/cgit/newlib-cygwin/commit/?id=3cb9da14617c58c2821c80d48f0bd80a2deb5fdf

child_info_spawn::worker calls waitpid() which ultimately calls 
cygwait() which notices the thread's cancel event is signalled and acts 
as a cancellation point.

Attached is a patch which adds back the restoration of signal handlers 
on thread cancellation.

I can't find any hints in the mailing lists around 2013-04 about what 
problem that change is fixing, but given the commentary, this might be 
reintroducing another problem, though.

[-- Attachment #2: 0001-Cygwin-Restore-pthread-cleanup-of-signal-handlers-du.patch --]
[-- Type: text/plain, Size: 1641 bytes --]

From a798750d271e20402a0a5efc4ac073f5948ad5b7 Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Sun, 16 Jul 2023 14:46:00 +0100
Subject: [PATCH] Cygwin: Restore pthread cleanup of signal handlers during
 system()

Removed in 3cb9da14 which describes it as 'ill-advised' (additional
context doesn't appear to be available).

We can't neatly tuck the pthread_cleanup_push/pop inside the object, as
they are implemented as macros which must appear in the same lexical
scope.
---
 winsup/cygwin/spawn.cc | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 84dd74e28..3696ac9b5 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -257,11 +257,15 @@ struct system_call_handle
   }
   ~system_call_handle ()
   {
-    if (is_system_call ())
+  }
+  static void cleanup (void *arg)
+  {
+# define this_ ((system_call_handle *) arg)
+    if (this_->is_system_call ())
       {
-	signal (SIGINT, oldint);
-	signal (SIGQUIT, oldquit);
-	sigprocmask (SIG_SETMASK, &oldmask, NULL);
+	signal (SIGINT, this_->oldint);
+	signal (SIGQUIT, this_->oldquit);
+	sigprocmask (SIG_SETMASK, &(this_->oldmask), NULL);
       }
   }
 # undef cleanup
@@ -912,8 +916,10 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
 	case _P_WAIT:
 	case _P_SYSTEM:
 	  system_call.arm ();
+	  pthread_cleanup_push (system_call_handle::cleanup, &system_call);
 	  if (waitpid (cygpid, &res, 0) != cygpid)
 	    res = -1;
+	  pthread_cleanup_pop (1);
 	  term_spawn_worker.cleanup ();
 	  break;
 	case _P_DETACH:
-- 
2.39.0