irssi-import / bugs.irssi.org

bugs.irssi.org archive
https://github.com/irssi/irssi/issues
0 stars 0 forks source link

Should avoid overriding custom SIGTERM handler unless user explicitly wants to ignore the signal #879

Open irssibot opened 11 years ago

irssibot commented 11 years ago

The problem: although an signal handler was installed to simulate "/quit" when SIGTERM is received, code executed later during startup always got rid of the handler and thus /quit was never simulated on SIGTERM.

As a result, the client always lost connections to servers in an uncontrolled fashion when SIGTERM occured, probably resulting in "Connection reset by peer" and similar quit messages. One "natural" source of SIGTERM is when the machine running irssi is rebooted without manually shutting down irssi first with the /quit command.

The situation inherits from core_init() which first calls settings_init() which calls init_configfile() to set up the sigterm handler. However, core_init() later calls read_settings() which on non-WIN32 systems sets up a bunch of signals (including SIGTERM) to either SIG_IGN or SIG_DFL depending on user preferences.

The attached patch changes read_settings() to retain the custom handler instead of using SIG_DFL for SIGTERM. Therefore the user can still choose to ignore SIGTERM, but when not [ignoring] the SIGTERM handler from init_configfile() will be used. The other signals are set up as before. It has also been implemented to work as expected when dynamically changing the "ignore_signals" setting.

The patch thus enables irssi to shut down cleanly even when receiving SIGTERM. This includes invoking any (perl) scripts that have hooked onto the "quit" command using command_bind('quit', ...);

irssibot commented 11 years ago

patch

commit 7605b97cab3ec09cfc398a178cdff4d4c4da55fe
Author: Jonas Berlin <irssi@outerspace.dyndns.org>
Date:   Wed Oct 24 22:59:35 2012

    Avoid overriding our SIGTERM handler unless user explicitly wants
    irssi to ignore the signal (non-WIN32 systems only).

    The problem: although an signal handler was installed to simulate
    "/quit" when SIGTERM is received, code executed later during startup
    always got rid of the handler and thus /quit was never simulated on
    SIGTERM.

    As a result, the client always lost connections to servers in an
    uncontrolled fashion when SIGTERM occured, probably resulting in
    "Connection reset by peer" and similar quit messages. One "natural"
    source of SIGTERM is when the machine running irssi is rebooted
    without manually shutting down irssi first with the /quit command.

    The situation inherits from core_init() which first calls
    settings_init() which calls init_configfile() to set up the sigterm
    handler. However, core_init() later calls read_settings() which on
    non-WIN32 systems sets up a bunch of signals (including SIGTERM) to
    either SIG_IGN or SIG_DFL depending on user preferences.

    This patch changes read_settings() to retain the custom handler
    instead of using SIG_DFL for SIGTERM. Therefore the user can still
    choose to ignore SIGTERM, but when not the SIGTERM handler from
    init_configfile() will be used. The other signals are set up as
    before. It has also been implemented to work as expected when
    dynamically changing the "ignore_signals" setting.

    The patch thus enables irssi to shut down cleanly even when receiving
    SIGTERM. This includes invoking any (perl) scripts that have hooked
    onto the "quit" command using command_bind('quit', ...);

diff --git src/core/core.c src/core/core.c
index b9debbb..844a6ef 100644
--- src/core/core.c
+++ src/core/core.c
@@ -92,11 +92,17 @@ static void read_settings(void)
        "int", "quit", "term",
        "alrm", "usr1", "usr2"
    };
+   static struct sigaction default_sigterm_handler;

    const char *ignores;
    struct sigaction act;
         int n;

+   // remember default sigterm handler on first call
+   if (!default_sigterm_handler.sa_handler && !default_sigterm_handler.sa_sigaction) {
+      sigaction(SIGTERM, NULL, &default_sigterm_handler);
+   }
+
    ignores = settings_get_str("ignore_signals");

    sigemptyset (&act.sa_mask);
@@ -109,7 +115,13 @@ static void read_settings(void)
    for (n = 0; n < sizeof(signals)/sizeof(signals[0]); n++) {
        act.sa_handler = find_substr(ignores, signames[n]) ?
            SIG_IGN : SIG_DFL;
-       sigaction(signals[n], &act, NULL);
+
+       /* For SIGTERM replace SIG_DFL with the signal handler we installed earlier in settings.c */
+       if (signals[n] == SIGTERM && act.sa_handler == SIG_DFL) {
+           sigaction(SIGTERM, &default_sigterm_handler, NULL);
+       } else {
+           sigaction(signals[n], &act, NULL);
+       }
    }

 #ifdef HAVE_SYS_RESOURCE_H
irssibot commented 11 years ago

In some sense it is good that sig_term() in src/core/settings.c is not used, since emitting "command quit" is definitely not async-signal-safe. A signal may happen at any time, including when data structures are being manipulated. This may result in hangs or crashes.

If you want to do this, use the glib main loop (gmain*). If glib does not offer a way to monitor signals directly, use a pipe-to-self, non-blocking write a byte to the pipe in the signal handler and add the other side of the pipe to the main loop. This will ensure the signal is only handled when the program is ready for it.

irssibot commented 11 years ago

Hi,

Thanks for your feedback. I totally agree.

GLib does offer signal monitoring but only from version 2.30 onwards. As it seemed from configure.in that irssi depends on 2.6.0, I decided to go with plan B (attached) i.e. pipe-to-self.

The attached patch should be applied in addition to the previous patch since they resolve disparate parts of the problem.

I took the liberty to write some "deinit" code for the added constructs. I reused the existing irssi helper functions I could find. I also tried to follow glib recommendations on making the code work under Windows, however I did not have the possibility to verify proper operation of the patch outside of my Linux system.

irssibot commented 11 years ago

make-signal-handler-use-a-pipe.patch

commit 135a01f6e805663d11bd45bbd426ee6d8d5713b4
Author: Jonas Berlin <irssi@outerspace.dyndns.org>
Date:   Fri Oct 26 22:28:25 2012

    In order avoid running risky code inside a unix signal handler,
    replace old sig_term() with indirection by waking up the GLib mainloop
    through writing to a pipe.

    This way the old sig_term() functionality gets called from inside the
    mainloop when the process is known to be in a well-defined state as
    opposed to what can be assumed inside a unix signal handler.

    The new signal handler is generic and forwards the signal number
    through the pipe so the construct can be reused for any number of
    signals if needed later.

diff --git src/core/settings.c src/core/settings.c
index 9f527eb..a05a7da 100644
--- src/core/settings.c
+++ src/core/settings.c
@@ -19,6 +19,7 @@
 */

 #include "module.h"
+#include "network.h"
 #include "signals.h"
 #include "commands.h"
 #include "levels.h"
@@ -48,6 +49,10 @@ static time_t config_last_mtime;
 static long config_last_size;
 static unsigned int config_last_checksum;

+static int signal_wakeup_pipe[2] = { -1, -1 };
+static GIOChannel *signal_wakeup_channel;
+static guint signal_wakeup_event_tag;
+
 static SETTINGS_REC *settings_get(const char *key, SettingType type)
 {
    SETTINGS_REC *rec;
@@ -570,16 +575,29 @@ GSList *settings_get_sorted(void)
    return list;
 }

-void sig_term(int n)
+void signal_handler(int signal)
 {
-   /* if we get SIGTERM after this, just die instead of coming back here. */
-   signal(SIGTERM, SIG_DFL);
+   gchar signalNo = (gchar) signal;
+   // write failures silently ignored
+   write(signal_wakeup_pipe[1], &signalNo, 1);
+}

-   /* quit from all servers too.. */
-   signal_emit("command quit", 1, "");
+void signal_wakeup_pipe_handler(void *data, GIOChannel *source, int condition) {
+   gchar signalNo;
+   gsize r;
+   GIOStatus status;

-   /* and die */
-   raise(SIGTERM);
+   status = g_io_channel_read_chars(signal_wakeup_channel, &signalNo, 1, &r, NULL);
+   if (status != G_IO_STATUS_NORMAL || r < 1) {
+       // ignore error conditions
+       return;
+   }
+   switch (signalNo) {
+       case SIGTERM:
+           /* Act as if the user typed "/quit" in the client */
+           signal_emit("command quit", 1, "");
+           break;
+   }
 }

 /* Yes, this is my own stupid checksum generator, some "real" algorithm
@@ -675,10 +693,20 @@ static CONFIG_REC *parse_configfile(const char *fname)
    return config;
 }

+static int set_nonblock_flag (int fd)
+{
+   int flags = fcntl (fd, F_GETFL, 0);
+   if (flags == -1)
+       return -1;
+   flags |= O_NONBLOCK;
+   return fcntl (fd, F_SETFL, flags);
+}
+
 static void init_configfile(void)
 {
    struct stat statbuf;
    char *str;
+   struct sigaction act;

    if (stat(get_irssi_dir(), &statbuf) != 0) {
        /* ~/.irssi not found, create it. */
@@ -702,9 +730,49 @@ static void init_configfile(void)
                 g_free(str);
    }

-   signal(SIGTERM, sig_term);
+   if (pipe(signal_wakeup_pipe)) {
+       g_warning("Failed to create signal wakeup pipe - SIGTERM not attached");
+       signal_wakeup_pipe[0] = 0;
+       return;
+   }
+   if (set_nonblock_flag(signal_wakeup_pipe[0]) || set_nonblock_flag(signal_wakeup_pipe[1])) {
+       g_warning("Failed to configure signal wakeup pipe - SIGTERM not attached");
+       return;
+   }
+
+   signal_wakeup_channel = g_io_channel_new(signal_wakeup_pipe[0]);
+   signal_wakeup_event_tag = g_input_add(signal_wakeup_channel, G_INPUT_READ, signal_wakeup_pipe_handler, NULL);
+
+   sigemptyset (&act.sa_mask);
+   act.sa_flags = SA_RESETHAND;
+   act.sa_handler = signal_handler;
+   sigaction(SIGTERM, &act, NULL);
+}
+
+static void deinit_configfile(void)
+{
+   struct sigaction act;
+
+   sigemptyset (&act.sa_mask);
+   act.sa_flags = 0;
+   act.sa_handler = SIG_DFL;
+   sigaction(SIGTERM, &act, NULL);
+
+   if (signal_wakeup_event_tag) {
+       g_source_remove(signal_wakeup_event_tag);
+   }
+   if (signal_wakeup_channel) {
+       g_io_channel_shutdown(signal_wakeup_channel, FALSE, NULL);
+       g_io_channel_unref(signal_wakeup_channel);
+   }
+
+   if (signal_wakeup_pipe[0]) {
+       close(signal_wakeup_pipe[0]);
+       close(signal_wakeup_pipe[1]);
+   }
 }

+
 int settings_reread(const char *fname)
 {
    CONFIG_REC *tempconfig;
@@ -818,6 +886,8 @@ void settings_deinit(void)
    signal_remove("irssi init finished", (SIGNAL_FUNC) sig_init_finished);
    signal_remove("gui exit", (SIGNAL_FUNC) sig_autosave);

+   deinit_configfile();
+
    g_slist_foreach(last_invalid_modules, (GFunc) g_free, NULL);
    g_slist_free(last_invalid_modules);
irssibot commented 11 years ago

Updated version of second patch; renamed variable for clarity

irssibot commented 11 years ago

make-signal-handler-use-a-pipe-2.patch

commit fdc8bf89fe4b19983d5c5a37090b3b35460286cc
Author: Jonas Berlin <irssi@outerspace.dyndns.org>
Date:   Fri Oct 26 22:28:25 2012

    In order avoid running risky code inside a unix signal handler,
    replace old sig_term() with indirection by waking up the GLib mainloop
    through writing to a pipe.

    This way the old sig_term() functionality gets called from inside the
    mainloop when the process is known to be in a well-defined state as
    opposed to what can be assumed inside a unix signal handler.

    The new signal handler is generic and forwards the signal number
    through the pipe so the construct can be reused for any number of
    signals if needed later.

diff --git src/core/settings.c src/core/settings.c
index 9f527eb..1255397 100644
--- src/core/settings.c
+++ src/core/settings.c
@@ -19,6 +19,7 @@
 */

 #include "module.h"
+#include "network.h"
 #include "signals.h"
 #include "commands.h"
 #include "levels.h"
@@ -48,6 +49,10 @@ static time_t config_last_mtime;
 static long config_last_size;
 static unsigned int config_last_checksum;

+static int signal_wakeup_pipe[2] = { -1, -1 };
+static GIOChannel *signal_wakeup_read_channel;
+static guint signal_wakeup_event_tag;
+
 static SETTINGS_REC *settings_get(const char *key, SettingType type)
 {
    SETTINGS_REC *rec;
@@ -570,16 +575,29 @@ GSList *settings_get_sorted(void)
    return list;
 }

-void sig_term(int n)
+void signal_handler(int signal)
 {
-   /* if we get SIGTERM after this, just die instead of coming back here. */
-   signal(SIGTERM, SIG_DFL);
+   gchar signalNo = (gchar) signal;
+   // write failures silently ignored
+   write(signal_wakeup_pipe[1], &signalNo, 1);
+}

-   /* quit from all servers too.. */
-   signal_emit("command quit", 1, "");
+void signal_wakeup_pipe_handler(void *data, GIOChannel *source, int condition) {
+   gchar signalNo;
+   gsize r;
+   GIOStatus status;

-   /* and die */
-   raise(SIGTERM);
+   status = g_io_channel_read_chars(signal_wakeup_read_channel, &signalNo, 1, &r, NULL);
+   if (status != G_IO_STATUS_NORMAL || r < 1) {
+       // ignore error conditions
+       return;
+   }
+   switch (signalNo) {
+       case SIGTERM:
+           /* Act as if the user typed "/quit" in the client */
+           signal_emit("command quit", 1, "");
+           break;
+   }
 }

 /* Yes, this is my own stupid checksum generator, some "real" algorithm
@@ -675,10 +693,20 @@ static CONFIG_REC *parse_configfile(const char *fname)
    return config;
 }

+static int set_nonblock_flag (int fd)
+{
+   int flags = fcntl (fd, F_GETFL, 0);
+   if (flags == -1)
+       return -1;
+   flags |= O_NONBLOCK;
+   return fcntl (fd, F_SETFL, flags);
+}
+
 static void init_configfile(void)
 {
    struct stat statbuf;
    char *str;
+   struct sigaction act;

    if (stat(get_irssi_dir(), &statbuf) != 0) {
        /* ~/.irssi not found, create it. */
@@ -702,9 +730,49 @@ static void init_configfile(void)
                 g_free(str);
    }

-   signal(SIGTERM, sig_term);
+   if (pipe(signal_wakeup_pipe)) {
+       g_warning("Failed to create signal wakeup pipe - SIGTERM not attached");
+       signal_wakeup_pipe[0] = 0;
+       return;
+   }
+   if (set_nonblock_flag(signal_wakeup_pipe[0]) || set_nonblock_flag(signal_wakeup_pipe[1])) {
+       g_warning("Failed to configure signal wakeup pipe - SIGTERM not attached");
+       return;
+   }
+
+   signal_wakeup_read_channel = g_io_channel_new(signal_wakeup_pipe[0]);
+   signal_wakeup_event_tag = g_input_add(signal_wakeup_read_channel, G_INPUT_READ, signal_wakeup_pipe_handler, NULL);
+
+   sigemptyset (&act.sa_mask);
+   act.sa_flags = SA_RESETHAND;
+   act.sa_handler = signal_handler;
+   sigaction(SIGTERM, &act, NULL);
+}
+
+static void deinit_configfile(void)
+{
+   struct sigaction act;
+
+   sigemptyset (&act.sa_mask);
+   act.sa_flags = 0;
+   act.sa_handler = SIG_DFL;
+   sigaction(SIGTERM, &act, NULL);
+
+   if (signal_wakeup_event_tag) {
+       g_source_remove(signal_wakeup_event_tag);
+   }
+   if (signal_wakeup_read_channel) {
+       g_io_channel_shutdown(signal_wakeup_read_channel, FALSE, NULL);
+       g_io_channel_unref(signal_wakeup_read_channel);
+   }
+
+   if (signal_wakeup_pipe[0]) {
+       close(signal_wakeup_pipe[0]);
+       close(signal_wakeup_pipe[1]);
+   }
 }

+
 int settings_reread(const char *fname)
 {
    CONFIG_REC *tempconfig;
@@ -818,6 +886,8 @@ void settings_deinit(void)
    signal_remove("irssi init finished", (SIGNAL_FUNC) sig_init_finished);
    signal_remove("gui exit", (SIGNAL_FUNC) sig_autosave);

+   deinit_configfile();
+
    g_slist_foreach(last_invalid_modules, (GFunc) g_free, NULL);
    g_slist_free(last_invalid_modules);
irssibot commented 11 years ago

../core/libcore.a(settings.o): In function init_configfile': settings.c:(.text+0x68e): undefined reference tog_io_channel_new' collect2: ld returned 1 exit status make[3]: *** [irssi] Error 1