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.From a798750d271e20402a0a5efc4ac073f5948ad5b7 Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.tur...@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
