Re: [Mono-dev] [PATCH] Incorrect signal handling for Sys V signal handler
> I was thinking about using a helper function that does "the right thing"[tm], > using sigaction if available, with a fallback to sigset and signal. A unified > function that may be used in other parts of the code is probably not > possible, since sigaction allows a number of flags that influence the signal > disposition and signal handling. I'm not sure whether a default set of flags > works for all use cases (e.g. syscall restarting yes/no, alternate signal > stack). Since signals are modified in a number of source files, a central > place to handle signals is probably the best way to go. Perhaps we need to re-evaluate the use of signal there in the first place, and use sigaction consistently. That is what I was alluding to before. ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-dev] [PATCH] Incorrect signal handling for Sys V signal handler
Hi,
On Thursday 27 January 2011 19:11:13 Miguel de Icaza wrote:
> Hello,
>
> I have other concerns about this use of signal. Perhaps it
> should be using sigaction with the proper flags to configure how we
> want signals to be delivered instead.
>
> Miguel
>
> On Thu, Jan 27, 2011 at 12:52 PM, Jonathan Pryor wrote:
> > I can't speak to the rest of the patch, but the mono/support patch can't
> > go in as-is, as mono/support/signal.c is also built for Windows (it's
> > part of MPH_C_SOURCE in mono/support/Makefile.am, which is included in
> > the HOST_WIN32 build).
> >
> > MSVCRT.DLL DOES contain signal(3); it does NOT contain sigset(3), and
> > thus this would break the Windows build.
> >
> > Furthermore, OSX doesn't provide sigset(3) either, so this would break
> > the OSX build as well.
> >
> > You should patch configure.in to check for sigset, and wrap the sigset
> > calls with HAVE_SIGSET, otherwise keep the existing signal calls.
You're right, checking for sigset should be done. I assumed it is part of
POSIX. The applied patch adds the check to configure.in and selects sigset or
signal, depending on the check's result.
Concerning sigaction:
I was thinking about using a helper function that does "the right thing"[tm],
using sigaction if available, with a fallback to sigset and signal. A unified
function that may be used in other parts of the code is probably not
possible, since sigaction allows a number of flags that influence the signal
disposition and signal handling. I'm not sure whether a default set of flags
works for all use cases (e.g. syscall restarting yes/no, alternate signal
stack). Since signals are modified in a number of source files, a central
place to handle signals is probably the best way to go.
With best regards,
Burkhard Linke
diff --git a/configure.in b/configure.in
index 8c1519b..a578800 100644
--- a/configure.in
+++ b/configure.in
@@ -1318,6 +1318,7 @@ if test x$target_win32 = xno; then
dnl ***
AC_CHECK_HEADERS(signal.h)
AC_CHECK_FUNCS(sigaction)
+ AC_CHECK_FUNCS(sigset)
dnl ***
dnl *** Checks for working __thread ***
diff --git a/mono/io-layer/daemon-messages.c b/mono/io-layer/daemon-messages.c
index 812d435..c81247d 100644
--- a/mono/io-layer/daemon-messages.c
+++ b/mono/io-layer/daemon-messages.c
@@ -85,7 +85,11 @@ static void _wapi_daemon_request_response_internal (int fd,
}
while (ret==-1 && errno==EINTR);
#else
- old_sigpipe = signal (SIGPIPE, SIG_IGN);
+#ifdef HAVE_SIGSET
+ old_sigpipe = sigset (SIGPIPE, SIG_IGN);
+#else
+old_sigpipe = signal (SIGPIPE, SIG_IGN);
+#endif /* HAVE_SIGSET */
do {
ret=sendmsg (fd, msg, 0);
}
@@ -113,7 +117,11 @@ static void _wapi_daemon_request_response_internal (int fd,
ret=recv (fd, resp, sizeof(WapiHandleResponse), 0);
}
while (ret==-1 && errno==EINTR);
+#ifdef HAVE_SIGSET
+ sigset (SIGPIPE, old_sigpipe);
+#else
signal (SIGPIPE, old_sigpipe);
+#endif /* HAVE_SIGSET */
#endif
if(ret==-1) {
diff --git a/mono/metadata/threadpool.c b/mono/metadata/threadpool.c
index 105ecf3..e1d1c36 100644
--- a/mono/metadata/threadpool.c
+++ b/mono/metadata/threadpool.c
@@ -1238,6 +1238,9 @@ signal_handler (int signo)
g_print ("\n-IO-\n");
print_pool_info (tp);
MONO_SEM_POST (&tp->lock);
+
+ /* set signal handler again due to SYS V signal behaviour */
+ signal (SIGALRM, signal_handler);
alarm (2);
}
#endif
@@ -1352,7 +1355,11 @@ mono_thread_pool_init ()
g_assert (async_io_tp.pc_nthreads);
tp_inited = 2;
#ifdef DEBUG
+#ifdef HAVE_SIGSET
+ sigset (SIGALRM, signal_handler);
+#else
signal (SIGALRM, signal_handler);
+#endif
alarm (2);
#endif
}
diff --git a/support/signal.c b/support/signal.c
index abd7638..05d2161 100644
--- a/support/signal.c
+++ b/support/signal.c
@@ -186,8 +186,13 @@ default_handler (int signum)
}
}
}
+#ifndef HAVE_SIGSET
+ /* signal handler disposition is reset under SYS V */
+ signal (signum, default_handler);
+#endif
}
+
static pthread_mutex_t signals_mutex = PTHREAD_MUTEX_INITIALIZER;
void*
@@ -217,7 +222,11 @@ Mono_Unix_UnixSignal_install (int sig)
for (i = 0; i < NUM_SIGNALS; ++i) {
if (h == NULL && signals [i].signum == 0) {
h = &signals [i];
+#if defined (HAVE_SIGSET)
+ h->handler = sigset (sig, default_handler);
+#else
h->handler = signal (sig, default_handler);
+#endif /*defined (HAVE_SIGSET) */
if (h->handler == SIG_ERR) {
h->handler = NULL;
h = NULL;
@@ -280,7 +289,11 @@ Mono_Unix_UnixSignal_uninstall (void* info)
else {
/* last UnixSignal -- we can unregister */
if (h->have_handler && count_handlers (h->signum) == 1) {
- mph_sighandler_t p = signal (h->signum, h->handler);
+#if defined (HAVE_SIGSET)
+ mph_sighandler_t p = sigset (h->signum, h->handler);
+#else
+ mph_sighandler_t p= signal (h->signum, h->handler);
+#endif /*defined (HAVE_SIGSET) */
if (p != SIG_ERR)
r = 0;
h->handler = NULL;
Re: [Mono-dev] [PATCH] Incorrect signal handling for Sys V signal handler
Hello, I have other concerns about this use of signal. Perhaps it should be using sigaction with the proper flags to configure how we want signals to be delivered instead. Miguel On Thu, Jan 27, 2011 at 12:52 PM, Jonathan Pryor wrote: > I can't speak to the rest of the patch, but the mono/support patch can't go > in as-is, as mono/support/signal.c is also built for Windows (it's part of > MPH_C_SOURCE in mono/support/Makefile.am, which is included in the HOST_WIN32 > build). > > MSVCRT.DLL DOES contain signal(3); it does NOT contain sigset(3), and thus > this would break the Windows build. > > Furthermore, OSX doesn't provide sigset(3) either, so this would break the > OSX build as well. > > You should patch configure.in to check for sigset, and wrap the sigset calls > with HAVE_SIGSET, otherwise keep the existing signal calls. > > - Jon > > On Jan 27, 2011, at 10:06 AM, Burkhard Linke wrote: > >> Hi, >> >> signal handlers registered with signal(3) behave in a somewhat different way >> for Sys V systems, e.g. Solaris. The manpage states that during the execution >> of the signal handler, the signal disposition is set to SIG_DFL. This raises >> two problems: >> >> a) a possible race condition, if the same signal occurs during the execution >> of its signal handler >> >> b) handling the signal a second time, since the manpage (and the >> implementation under Solaris..) does not reinstall the signal handler. >> >> The attached test program uses Mono.Unix.Signal to catch signals. Executing >> it >> under Solaris/x86 running Mono 2.8.1, Mono 2.6.7 and the git master gives the >> following output: >> >> running as process 29499 >> sending first HUP >> sending second HUP >> Hangup >> >> The second SIGHUP is not catched by the signal handler, terminating the >> application (which is the default for SIGHUP). >> >> The applied patch solves the problem by replacing signal(3) with sigset(3). >> sigset's semantic differs from signal's one; Instead of setting the signal to >> default handling, it adds the signal to the process/threads signal masks, >> executed the signal handler and restores the signal mask. Both problems >> mentioned above are solved. Test program output after applying the patch to >> git master: >> >> running as process 172 >> sending first HUP >> sending second HUP >> sending third HUP >> terminating >> >> >> With best regards, >> Burkhard Linke >> ___ >> Mono-devel-list mailing list >> Mono-devel-list@lists.ximian.com >> http://lists.ximian.com/mailman/listinfo/mono-devel-list > > ___ > Mono-devel-list mailing list > Mono-devel-list@lists.ximian.com > http://lists.ximian.com/mailman/listinfo/mono-devel-list > ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
Re: [Mono-dev] [PATCH] Incorrect signal handling for Sys V signal handler
I can't speak to the rest of the patch, but the mono/support patch can't go in as-is, as mono/support/signal.c is also built for Windows (it's part of MPH_C_SOURCE in mono/support/Makefile.am, which is included in the HOST_WIN32 build). MSVCRT.DLL DOES contain signal(3); it does NOT contain sigset(3), and thus this would break the Windows build. Furthermore, OSX doesn't provide sigset(3) either, so this would break the OSX build as well. You should patch configure.in to check for sigset, and wrap the sigset calls with HAVE_SIGSET, otherwise keep the existing signal calls. - Jon On Jan 27, 2011, at 10:06 AM, Burkhard Linke wrote: > Hi, > > signal handlers registered with signal(3) behave in a somewhat different way > for Sys V systems, e.g. Solaris. The manpage states that during the execution > of the signal handler, the signal disposition is set to SIG_DFL. This raises > two problems: > > a) a possible race condition, if the same signal occurs during the execution > of its signal handler > > b) handling the signal a second time, since the manpage (and the > implementation under Solaris..) does not reinstall the signal handler. > > The attached test program uses Mono.Unix.Signal to catch signals. Executing > it > under Solaris/x86 running Mono 2.8.1, Mono 2.6.7 and the git master gives the > following output: > > running as process 29499 > sending first HUP > sending second HUP > Hangup > > The second SIGHUP is not catched by the signal handler, terminating the > application (which is the default for SIGHUP). > > The applied patch solves the problem by replacing signal(3) with sigset(3). > sigset's semantic differs from signal's one; Instead of setting the signal to > default handling, it adds the signal to the process/threads signal masks, > executed the signal handler and restores the signal mask. Both problems > mentioned above are solved. Test program output after applying the patch to > git master: > > running as process 172 > sending first HUP > sending second HUP > sending third HUP > terminating > > > With best regards, > Burkhard Linke > ___ > Mono-devel-list mailing list > Mono-devel-list@lists.ximian.com > http://lists.ximian.com/mailman/listinfo/mono-devel-list ___ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list
[Mono-dev] [PATCH] Incorrect signal handling for Sys V signal handler
Hi,
signal handlers registered with signal(3) behave in a somewhat different way
for Sys V systems, e.g. Solaris. The manpage states that during the execution
of the signal handler, the signal disposition is set to SIG_DFL. This raises
two problems:
a) a possible race condition, if the same signal occurs during the execution
of its signal handler
b) handling the signal a second time, since the manpage (and the
implementation under Solaris..) does not reinstall the signal handler.
The attached test program uses Mono.Unix.Signal to catch signals. Executing it
under Solaris/x86 running Mono 2.8.1, Mono 2.6.7 and the git master gives the
following output:
running as process 29499
sending first HUP
sending second HUP
Hangup
The second SIGHUP is not catched by the signal handler, terminating the
application (which is the default for SIGHUP).
The applied patch solves the problem by replacing signal(3) with sigset(3).
sigset's semantic differs from signal's one; Instead of setting the signal to
default handling, it adds the signal to the process/threads signal masks,
executed the signal handler and restores the signal mask. Both problems
mentioned above are solved. Test program output after applying the patch to
git master:
running as process 172
sending first HUP
sending second HUP
sending third HUP
terminating
With best regards,
Burkhard Linke
diff --git a/mono/io-layer/daemon-messages.c b/mono/io-layer/daemon-messages.c
index 812d435..0c8ce44 100644
--- a/mono/io-layer/daemon-messages.c
+++ b/mono/io-layer/daemon-messages.c
@@ -85,7 +85,7 @@ static void _wapi_daemon_request_response_internal (int fd,
}
while (ret==-1 && errno==EINTR);
#else
- old_sigpipe = signal (SIGPIPE, SIG_IGN);
+ old_sigpipe = sigset (SIGPIPE, SIG_IGN);
do {
ret=sendmsg (fd, msg, 0);
}
@@ -113,7 +113,7 @@ static void _wapi_daemon_request_response_internal (int fd,
ret=recv (fd, resp, sizeof(WapiHandleResponse), 0);
}
while (ret==-1 && errno==EINTR);
- signal (SIGPIPE, old_sigpipe);
+ sigset (SIGPIPE, old_sigpipe);
#endif
if(ret==-1) {
diff --git a/mono/metadata/threadpool.c b/mono/metadata/threadpool.c
index 105ecf3..f1056fe 100644
--- a/mono/metadata/threadpool.c
+++ b/mono/metadata/threadpool.c
@@ -1352,7 +1352,7 @@ mono_thread_pool_init ()
g_assert (async_io_tp.pc_nthreads);
tp_inited = 2;
#ifdef DEBUG
- signal (SIGALRM, signal_handler);
+ sigset (SIGALRM, signal_handler);
alarm (2);
#endif
}
diff --git a/support/signal.c b/support/signal.c
index abd7638..32450d8 100644
--- a/support/signal.c
+++ b/support/signal.c
@@ -217,7 +217,7 @@ Mono_Unix_UnixSignal_install (int sig)
for (i = 0; i < NUM_SIGNALS; ++i) {
if (h == NULL && signals [i].signum == 0) {
h = &signals [i];
- h->handler = signal (sig, default_handler);
+ h->handler = sigset (sig, default_handler);
if (h->handler == SIG_ERR) {
h->handler = NULL;
h = NULL;
@@ -280,7 +280,7 @@ Mono_Unix_UnixSignal_uninstall (void* info)
else {
/* last UnixSignal -- we can unregister */
if (h->have_handler && count_handlers (h->signum) == 1) {
- mph_sighandler_t p = signal (h->signum, h->handler);
+ mph_sighandler_t p = sigset (h->signum, h->handler);
if (p != SIG_ERR)
r = 0;
h->handler = NULL;
using System;
using System.Threading;
using Mono.Unix;
using Mono.Unix.Native;
namespace SignalTest
{
class MainClass
{
public static void Main (string[] args)
{
// get our PID
int pid = Mono.Unix.UnixProcess.GetCurrentProcessId ();
System.Console.Error.WriteLine ("running as process {0}", pid);
// install signal handler for SIGUP
UnixSignal sighup = new UnixSignal (Signum.SIGHUP);
// handle signals in a thread
new Thread (delegate() {
while (true) {
sighup.WaitOne ();
System.Console.Error.WriteLine ("got HUP signals");
}
}).Start ();
System.Console.Error.WriteLine ("sending first HUP");
Mono.Unix.UnixProcess.GetCurrentProcess ().Signal (Signum.SIGHUP);
System.Console.Error.WriteLine ("sending second HUP");
Mono.Unix.UnixProcess.GetCurrentProcess ().Signal (Signum.SIGHUP);
System.Console.Error.WriteLine ("sending third HUP");
Mono.Unix.UnixProcess.GetCurrentProcess ().Signal (Signum.SIGHUP);
System.Console.Error.WriteLine ("terminating");
System.Environment.Exit (0);
}
}
}
___
Mono-devel-list mailing list
Mono-devel-list@lists.ximian.com
http://lists.ximian.com/mailman/listinfo/mono-devel-list
