[3/4] scoped_ignore_signal: Use sigprocmask+sigtimedwait instead of signal

Message ID 20210615111429.1879286-4-pedro@palves.net
State New
Headers show
Series
  • Introduce scoped_ignore_signal & make it thread safe
Related show

Commit Message

Pedro Alves June 15, 2021, 11:14 a.m.
The problem with using signal(...) to temporarily ignore a signal, is
that that changes the the signal disposition for the whole process.
If multiple threads do it at the same time, you have a race.

Fix this by using sigprocmask + sigtimedwait to implement the ignoring
instead, if available, which I think probably means everywhere except
Windows nowadays.  This way, we only change the signal mask for the
current thread, so there's no race.

Change-Id: Idfe3fb08327ef8cae926f3de9ee81c56a83b1738

gdbsupport/ChangeLog:
yyyy-mm-dd  Pedro Alves  <pedro@palves.net>

	* scoped_ignore_signal.h
	(scoped_ignore_signal::scoped_ignore_signal)
	[HAVE_SIGPROCMASK]: Use sigprocmask to block the signal instead of
	changing the signal disposition for the whole process.
	(scoped_ignore_signal::~scoped_ignore_signal) [HAVE_SIGPROCMASK]:
	Use sigtimedwait and sigprocmask to flush and unblock the signal.
---
 gdbsupport/scoped_ignore_signal.h | 37 +++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

-- 
2.26.2

Comments

Lancelot SIX via Gdb-patches June 26, 2021, 12:35 p.m. | #1
On 2021-06-15 7:14 a.m., Pedro Alves wrote:
> The problem with using signal(...) to temporarily ignore a signal, is

> that that changes the the signal disposition for the whole process.

> If multiple threads do it at the same time, you have a race.

> 

> Fix this by using sigprocmask + sigtimedwait to implement the ignoring

> instead, if available, which I think probably means everywhere except

> Windows nowadays.  This way, we only change the signal mask for the

> current thread, so there's no race.


I tried to build on macOS today and got:


      CXX    compile/compile.o
    In file included from /Users/smarchi/src/binutils-gdb/gdb/compile/compile.c:46:
    /Users/smarchi/src/binutils-gdb/gdb/../gdbsupport/scoped_ignore_signal.h:69:4: error: use of undeclared identifier 'sigtimedwait'
              sigtimedwait (&set, nullptr, &zero_timeout);
              ^

I didn't have time to dig yet.

Simon
John Baldwin June 26, 2021, 1:41 p.m. | #2
On 6/26/21 5:35 AM, Simon Marchi via Gdb-patches wrote:
> On 2021-06-15 7:14 a.m., Pedro Alves wrote:

>> The problem with using signal(...) to temporarily ignore a signal, is

>> that that changes the the signal disposition for the whole process.

>> If multiple threads do it at the same time, you have a race.

>>

>> Fix this by using sigprocmask + sigtimedwait to implement the ignoring

>> instead, if available, which I think probably means everywhere except

>> Windows nowadays.  This way, we only change the signal mask for the

>> current thread, so there's no race.

> 

> I tried to build on macOS today and got:

> 

> 

>        CXX    compile/compile.o

>      In file included from /Users/smarchi/src/binutils-gdb/gdb/compile/compile.c:46:

>      /Users/smarchi/src/binutils-gdb/gdb/../gdbsupport/scoped_ignore_signal.h:69:4: error: use of undeclared identifier 'sigtimedwait'

>                sigtimedwait (&set, nullptr, &zero_timeout);

>                ^

> 

> I didn't have time to dig yet.


It looks like macOS doesn't implement either sigtimedwait() or sigwaitinfo()
which are part of POSIX from 1996 (*sigh*); however, macOS does provide
sigpending() and sigwait(), so perhaps we can do something like:

    if (ConsumePending)
      {
#ifdef HAVE_SIGTIMEDWAIT
        const timespec zero_timeout = {};

        sigtimedwait (&set, nullptr, &zero_timeout);
#else
        sigset_t pending;

        sigpending (&pending);
        if (sigismember (&pending, SIG))
          sigwait (&set, nullptr);
#endif
      }

(Ironically, sigwait() was added to POSIX in the same version of the spec as
sigtimedwait().)

-- 
John Baldwin
Lancelot SIX via Gdb-patches June 26, 2021, 7:10 p.m. | #3
On 2021-06-26 9:41 a.m., John Baldwin wrote:
> On 6/26/21 5:35 AM, Simon Marchi via Gdb-patches wrote:

>> On 2021-06-15 7:14 a.m., Pedro Alves wrote:

>>> The problem with using signal(...) to temporarily ignore a signal, is

>>> that that changes the the signal disposition for the whole process.

>>> If multiple threads do it at the same time, you have a race.

>>>

>>> Fix this by using sigprocmask + sigtimedwait to implement the ignoring

>>> instead, if available, which I think probably means everywhere except

>>> Windows nowadays.  This way, we only change the signal mask for the

>>> current thread, so there's no race.

>>

>> I tried to build on macOS today and got:

>>

>>

>>        CXX    compile/compile.o

>>      In file included from /Users/smarchi/src/binutils-gdb/gdb/compile/compile.c:46:

>>      /Users/smarchi/src/binutils-gdb/gdb/../gdbsupport/scoped_ignore_signal.h:69:4: error: use of undeclared identifier 'sigtimedwait'

>>                sigtimedwait (&set, nullptr, &zero_timeout);

>>                ^

>>

>> I didn't have time to dig yet.

> 

> It looks like macOS doesn't implement either sigtimedwait() or sigwaitinfo()

> which are part of POSIX from 1996 (*sigh*); however, macOS does provide

> sigpending() and sigwait(), so perhaps we can do something like:

> 

>    if (ConsumePending)

>      {

> #ifdef HAVE_SIGTIMEDWAIT

>        const timespec zero_timeout = {};

> 

>        sigtimedwait (&set, nullptr, &zero_timeout);

> #else

>        sigset_t pending;

> 

>        sigpending (&pending);

>        if (sigismember (&pending, SIG))


SIG -> Sig

>          sigwait (&set, nullptr);

> #endif

>      }

> 

> (Ironically, sigwait() was added to POSIX in the same version of the spec as

> sigtimedwait().)

> 


Thanks.  At least that change fixes that particular build issue, but I
can't tell if it has the correct behavior though (I haven't looked at
what sigtimedwait / sigwait do yet...).

Simon
Pedro Alves June 27, 2021, 2:55 p.m. | #4
On 2021-06-26 2:41 p.m., John Baldwin wrote:
> On 6/26/21 5:35 AM, Simon Marchi via Gdb-patches wrote:

>> On 2021-06-15 7:14 a.m., Pedro Alves wrote:

>>> The problem with using signal(...) to temporarily ignore a signal, is

>>> that that changes the the signal disposition for the whole process.

>>> If multiple threads do it at the same time, you have a race.

>>>

>>> Fix this by using sigprocmask + sigtimedwait to implement the ignoring

>>> instead, if available, which I think probably means everywhere except

>>> Windows nowadays.  This way, we only change the signal mask for the

>>> current thread, so there's no race.

>>

>> I tried to build on macOS today and got:

>>

>>

>>        CXX    compile/compile.o

>>      In file included from /Users/smarchi/src/binutils-gdb/gdb/compile/compile.c:46:

>>      /Users/smarchi/src/binutils-gdb/gdb/../gdbsupport/scoped_ignore_signal.h:69:4: error: use of undeclared identifier 'sigtimedwait'

>>                sigtimedwait (&set, nullptr, &zero_timeout);

>>                ^

>>

>> I didn't have time to dig yet.

> 

> It looks like macOS doesn't implement either sigtimedwait() or sigwaitinfo()

> which are part of POSIX from 1996 (*sigh*); however, macOS does provide

> sigpending() and sigwait(), so perhaps we can do something like:

> 

>    if (ConsumePending)

>      {

> #ifdef HAVE_SIGTIMEDWAIT

>        const timespec zero_timeout = {};

> 

>        sigtimedwait (&set, nullptr, &zero_timeout);

> #else

>        sigset_t pending;

> 

>        sigpending (&pending);

>        if (sigismember (&pending, SIG))

>          sigwait (&set, nullptr);

> #endif

>      }

> 


Thanks, that looks right to me.  We're using sigtimewait with zero timeout to consume
a pending signal.  sigpending + sigwait effectively does the same thing.

According to gnulib, sigpending exists everywhere but mingw:

 https://www.gnu.org/software/gnulib/manual/html_node/sigpending.html

and sigtimedwait is missing on "Mac OS X 10.13, OpenBSD 6.7, Minix 3.1.8, Cygwin 2.9, mingw, MSVC 14, Android 5.1.":

  https://www.gnu.org/software/gnulib/manual/html_node/sigtimedwait.html

So the approach you propose should work well.

Given sigpending is everywhere Unix-like, if we'd like, we could instead use the sigpending + sigwait path
unconditionally everywhere, but that's two syscalls instead of one, and the configure check is
trivial (just add sigtimedwait to AC_CHECK_FUNCS in gdbsupport/common.m4), so why not indeed save one
syscall, seems easy enough.

Let me know if you'd rather me do the leg work of writing the actual patch.

Pedro Alves
Lancelot SIX via Gdb-patches June 27, 2021, 7:01 p.m. | #5
> Thanks, that looks right to me.  We're using sigtimewait with zero timeout to consume

> a pending signal.  sigpending + sigwait effectively does the same thing.

> 

> According to gnulib, sigpending exists everywhere but mingw:

> 

>  https://www.gnu.org/software/gnulib/manual/html_node/sigpending.html

> 

> and sigtimedwait is missing on "Mac OS X 10.13, OpenBSD 6.7, Minix 3.1.8, Cygwin 2.9, mingw, MSVC 14, Android 5.1.":

> 

>   https://www.gnu.org/software/gnulib/manual/html_node/sigtimedwait.html

> 

> So the approach you propose should work well.

> 

> Given sigpending is everywhere Unix-like, if we'd like, we could instead use the sigpending + sigwait path

> unconditionally everywhere, but that's two syscalls instead of one, and the configure check is

> trivial (just add sigtimedwait to AC_CHECK_FUNCS in gdbsupport/common.m4), so why not indeed save one

> syscall, seems easy enough.

> 

> Let me know if you'd rather me do the leg work of writing the actual patch.


Thanks, I'll do it since I have access to a macOS system, and therefore
I can at least compile-test it.

Simon

Patch

diff --git a/gdbsupport/scoped_ignore_signal.h b/gdbsupport/scoped_ignore_signal.h
index cccd390529b..55a921cb332 100644
--- a/gdbsupport/scoped_ignore_signal.h
+++ b/gdbsupport/scoped_ignore_signal.h
@@ -22,7 +22,10 @@ 
 
 #include <signal.h>
 
-/* RAII class used to ignore a signal in a scope.  */
+/* RAII class used to ignore a signal in a scope.  If sigprocmask is
+   supported, then the signal is only ignored by the calling thread.
+   Otherwise, the signal disposition is set to SIG_IGN, which affects
+   the whole process.  */
 
 template <int Sig>
 class scoped_ignore_signal
@@ -30,18 +33,48 @@  class scoped_ignore_signal
 public:
   scoped_ignore_signal ()
   {
+#ifdef HAVE_SIGPROCMASK
+    sigset_t set, old_state;
+
+    sigemptyset (&set);
+    sigaddset (&set, Sig);
+    sigprocmask (SIG_BLOCK, &set, &old_state);
+    m_was_blocked = sigismember (&old_state, Sig);
+#else
     m_osig = signal (Sig, SIG_IGN);
+#endif
   }
 
   ~scoped_ignore_signal ()
   {
+#ifdef HAVE_SIGPROCMASK
+    if (!m_was_blocked)
+      {
+	sigset_t set;
+	const timespec zero_timeout = {};
+
+	sigemptyset (&set);
+	sigaddset (&set, Sig);
+
+	/* If we got a pending Sig signal, consume it before
+	   unblocking.  */
+	sigtimedwait (&set, nullptr, &zero_timeout);
+
+	sigprocmask (SIG_UNBLOCK, &set, nullptr);
+      }
+#else
     signal (Sig, m_osig);
+#endif
   }
 
   DISABLE_COPY_AND_ASSIGN (scoped_ignore_signal);
 
 private:
-  sighandler_t m_osig = nullptr;
+#ifdef HAVE_SIGPROCMASK
+  bool m_was_blocked;
+#else
+  sighandler_t m_osig;
+#endif
 };
 
 struct scoped_ignore_signal_nop