[Openvpn-devel,1/2] Improve signal handling using POSIX sigaction

Message ID 20230128215901.2207208-1-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,1/2] Improve signal handling using POSIX sigaction | expand

Commit Message

Selva Nair Jan. 28, 2023, 9:59 p.m. UTC
From: Selva Nair <selva.nair@gmail.com>

Currently we use the old signal API which follows system-V or
BSD semantics depending on the platform and/or feature-set macros.
Further, signal has many weaknesses which makes proper masking
(deferring) of signals during update not possible.

Improve this:

- Use sigaction to properly mask signals when modifying.

Notes:

Updating signal_reset() is handled in a follow up patch

SIG_SOURCE_CONNECTION_FAILED is retained in a hackish way. This value
has the same meaning as SIG_SOURCE_SOFT everywhere except where the
signal is printed. Looks cosmetic --- could be eliminated?

In pre_init_signal_catch() we ignore some unix signals, but the same signals
from management are not ignored though both are treated as "HARD" signals.
For example, during auth-user-pass query, "kill -SIGUSR1 <pid>" will be ignored,
but "signal SIGUSR1" from management interface will cause M_FATAL and exit.
This is the current behaviour, but could be improved?

This patch was originally submitted as 5/5 of the signals series. Now this is
1/2 of a new series with signal_reset changes moved to 2/2

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/errlevel.h |   1 +
 src/openvpn/sig.c      | 264 +++++++++++++++++++++++++++++++----------
 src/openvpn/socket.c   |   1 -
 3 files changed, 202 insertions(+), 64 deletions(-)

Comments

Frank Lichtenheld Jan. 31, 2023, 10:48 a.m. UTC | #1
On Sat, Jan 28, 2023 at 04:59:00PM -0500, selva.nair@gmail.com wrote:
> From: Selva Nair <selva.nair@gmail.com>
> 
> Currently we use the old signal API which follows system-V or
> BSD semantics depending on the platform and/or feature-set macros.
> Further, signal has many weaknesses which makes proper masking
> (deferring) of signals during update not possible.
> 
> Improve this:
> 
> - Use sigaction to properly mask signals when modifying.
> 

Acked-By: Frank Lichtenheld <frank@lichtenheld.com>

Stared at code intensively. AFAICT this should not change the
general behavior except to be more generally safe.

Only compile+t_client tested.

Regards,
Selva Nair May 25, 2023, 6:41 p.m. UTC | #2
Hi Gert,

Now that 2.6 appears to have reached a fairly stable state, may I request
you to look into this patch for 2.7 -- this one has an ACK (thanks to
Frank), 2/2 may need a closer look but that one is small.

I dread the prospect of this developing serious merge conflicts and having
to drill down into the details to resolve them. Right now it looks like no
one has yet touched related chunks.

Thanks,

Selva



On Tue, Jan 31, 2023 at 5:48 AM Frank Lichtenheld <frank@lichtenheld.com>
wrote:

> On Sat, Jan 28, 2023 at 04:59:00PM -0500, selva.nair@gmail.com wrote:
> > From: Selva Nair <selva.nair@gmail.com>
> >
> > Currently we use the old signal API which follows system-V or
> > BSD semantics depending on the platform and/or feature-set macros.
> > Further, signal has many weaknesses which makes proper masking
> > (deferring) of signals during update not possible.
> >
> > Improve this:
> >
> > - Use sigaction to properly mask signals when modifying.
> >
>
> Acked-By: Frank Lichtenheld <frank@lichtenheld.com>
>
> Stared at code intensively. AFAICT this should not change the
> general behavior except to be more generally safe.
>
> Only compile+t_client tested.
>
> Regards,
> --
>   Frank Lichtenheld
>
Gert Doering May 29, 2023, 7:07 p.m. UTC | #3
Hi,

On Thu, May 25, 2023 at 02:41:10PM -0400, Selva Nair wrote:
> Now that 2.6 appears to have reached a fairly stable state, may I request
> you to look into this patch for 2.7 -- this one has an ACK (thanks to
> Frank), 2/2 may need a closer look but that one is small.
> 
> I dread the prospect of this developing serious merge conflicts and having
> to drill down into the details to resolve them. Right now it looks like no
> one has yet touched related chunks.

Will not really have much available time next week, but I have heard
the message - will look into this the week after.  If not, please feel
free to send me another reminder.

gert
Selva Nair June 26, 2023, 1:49 p.m. UTC | #4
On Mon, May 29, 2023 at 3:07 PM Gert Doering <gert@greenie.muc.de> wrote:

> Hi,
>
> On Thu, May 25, 2023 at 02:41:10PM -0400, Selva Nair wrote:
> > Now that 2.6 appears to have reached a fairly stable state, may I request
> > you to look into this patch for 2.7 -- this one has an ACK (thanks to
> > Frank), 2/2 may need a closer look but that one is small.
> >
> > I dread the prospect of this developing serious merge conflicts and
> having
> > to drill down into the details to resolve them. Right now it looks like
> no
> > one has yet touched related chunks.
>
> Will not really have much available time next week, but I have heard
> the message - will look into this the week after.  If not, please feel
> free to send me another reminder.
>

Here it is :)

Selva
Gert Doering July 23, 2023, 9:28 a.m. UTC | #5
Sorry for taking so long on this - it hit post-release fatigue...

I did test this by running t_client test (which use signals to kill
running openvpn) across all supported OSes, and server side tests on
Linux, with and without DCO.  Of course this is not really excercising
the signal handling code, but at least it confirms that nothing is
grossly broken.

Stared at the code & the manpages as well.  Following my path through
the twisty maze of passages (especially halt_low_priority_signals())
was not easy, but "it seems to make sense" (this blocks signals, but
there is no new code to unblock - if I'm reading this right, this is
because we are at "exit (single) instance" time, and unblocking signals
will be done at init_instance_handle_signals() on the reconnection).

The actual "change to POSIX ways" part of this is fairly trivial
and easy to understand :-) - though I do wonder why you're using
an extra variable for block_mask -> sa.sa_mask, and not using
sigfillset(&sa.sa_mask) - at least on BSD on Linux, both are sigset_t,
so it should do the same thing in a slightly more direct way.

Your patch has been applied to the master branch.

commit 3abfc0fb5ea522fb5fc45fddbf35c3cc5c8e9ef1
Author: Selva Nair
Date:   Sat Jan 28 16:59:00 2023 -0500

     Improve signal handling using POSIX sigaction

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Message-Id: <20230128215901.2207208-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26087.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Selva Nair July 23, 2023, 2:46 p.m. UTC | #6
On Sun, Jul 23, 2023 at 5:28 AM Gert Doering <gert@greenie.muc.de> wrote:

>
> The actual "change to POSIX ways" part of this is fairly trivial
> and easy to understand :-) - though I do wonder why you're using
> an extra variable for block_mask -> sa.sa_mask, and not using
> sigfillset(&sa.sa_mask) - at least on BSD on Linux, both are sigset_t,
> so it should do the same thing in a slightly more direct way.
>

I can't recall any particular reason for having that intermediate variable.
It does look redundant.

Now I wish someone could review 2/2 of this patch-set. That's where further
potential
loss of signal during signal-reset is avoided.

Selva
Gert Doering July 23, 2023, 3:01 p.m. UTC | #7
Hi,

On Sun, Jul 23, 2023 at 10:46:17AM -0400, Selva Nair wrote:
> Now I wish someone could review 2/2 of this patch-set. That's where further
> potential loss of signal during signal-reset is avoided.

Working on finding a vict^Wvolunteer :-)

(I've briefly looked over 2/2 and didn't find anything "obviously 
problematic", but this needs more brains and life is a bit chaotic
over here right now - sufficient brains to test & merge, not enough
to really understand all the fine details involved here)

gert

Patch

diff --git a/src/openvpn/errlevel.h b/src/openvpn/errlevel.h
index c69ea91d..dedc0790 100644
--- a/src/openvpn/errlevel.h
+++ b/src/openvpn/errlevel.h
@@ -115,6 +115,7 @@ 
 #define D_CLIENT_NAT         LOGLEV(6, 69, M_DEBUG)  /* show client NAT debug info */
 #define D_XKEY               LOGLEV(6, 69, M_DEBUG)  /* show xkey-provider debug info */
 #define D_DCO_DEBUG          LOGLEV(6, 69, M_DEBUG)  /* show DCO related lowlevel debug messages */
+#define D_SIGNAL_DEBUG       LOGLEV(6, 69, M_DEBUG)  /* show signal related debug messages */
 
 #define D_SHOW_KEYS          LOGLEV(7, 70, M_DEBUG)  /* show data channel encryption keys */
 #define D_SHOW_KEY_SOURCE    LOGLEV(7, 70, M_DEBUG)  /* show data channel key source entropy */
diff --git a/src/openvpn/sig.c b/src/openvpn/sig.c
index 0d534601..559ca35d 100644
--- a/src/openvpn/sig.c
+++ b/src/openvpn/sig.c
@@ -6,6 +6,7 @@ 
  *             packet compression.
  *
  *  Copyright (C) 2002-2023 OpenVPN Inc <sales@openvpn.net>
+ *  Copyright (C) 2016-2023 Selva Nair <selva.nair@gmail.com>
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2
@@ -60,6 +61,9 @@  static const struct signame signames[] = {
     { SIGUSR2, 1, "SIGUSR2", "sigusr2" }
 };
 
+/* mask for hard signals from management or windows */
+static unsigned long long ignored_hard_signals_mask;
+
 int
 parse_signal(const char *signame)
 {
@@ -114,24 +118,144 @@  signal_description(const int signum, const char *sigtext)
     }
 }
 
+/**
+ * Block (i.e., defer) all unix signals.
+ * Used while directly modifying the volatile elements of
+ * siginfo_static.
+ */
+static inline void
+block_async_signals(void)
+{
+#ifndef _WIN32
+    sigset_t all;
+    sigfillset(&all); /* all signals */
+    sigprocmask(SIG_BLOCK, &all, NULL);
+#endif
+}
+
+/**
+ * Unblock all unix signals.
+ */
+static inline void
+unblock_async_signals(void)
+{
+#ifndef _WIN32
+    sigset_t none;
+    sigemptyset(&none);
+    sigprocmask(SIG_SETMASK, &none, NULL);
+#endif
+}
+
+/**
+ * Private function for registering a signal in the specified
+ * signal_info struct. This could be the global siginfo_static
+ * or a context specific signinfo struct.
+ *
+ * A signal is allowed to override an already registered
+ * one only if it has a higher priority.
+ * Returns true if the signal is set, false otherwise.
+ *
+ * Do not call any "AS-unsafe" functions such as printf from here
+ * as this may be called from signal_handler().
+ */
+static bool
+try_throw_signal(struct signal_info *si, int signum, int source)
+{
+    bool ret = false;
+    if (signal_priority(signum) >= signal_priority(si->signal_received))
+    {
+        si->signal_received = signum;
+        si->source = source;
+        ret = true;
+    }
+    return ret;
+}
+
+/**
+ * Throw a hard signal. Called from management and when windows
+ * signals are received through ctrl-c, exit event etc.
+ */
 void
 throw_signal(const int signum)
 {
-    if (signal_priority(signum) >= signal_priority(siginfo_static.signal_received))
+    if (ignored_hard_signals_mask & (1LL << signum))
+    {
+        msg(D_SIGNAL_DEBUG, "Signal %s is currently ignored", signal_name(signum, true));
+        return;
+    }
+    block_async_signals();
+
+    if (!try_throw_signal(&siginfo_static, signum, SIG_SOURCE_HARD))
     {
-        siginfo_static.signal_received = signum;
-        siginfo_static.source = SIG_SOURCE_HARD;
+        msg(D_SIGNAL_DEBUG, "Ignoring %s when %s has been received", signal_name(signum, true),
+            signal_name(siginfo_static.signal_received, true));
     }
+    else
+    {
+        msg(D_SIGNAL_DEBUG, "Throw signal (hard): %s ", signal_name(signum, true));
+    }
+
+    unblock_async_signals();
 }
 
+/**
+ * Throw a soft global signal. Used to register internally generated signals
+ * due to errors that require a restart or exit, or restart requests
+ * received from the server. A textual description of the signal may
+ * be provided.
+ */
 void
 throw_signal_soft(const int signum, const char *signal_text)
 {
-    if (signal_priority(signum) >= signal_priority(siginfo_static.signal_received))
+    block_async_signals();
+
+    if (try_throw_signal(&siginfo_static, signum, SIG_SOURCE_SOFT))
     {
-        siginfo_static.signal_received = signum;
-        siginfo_static.source = SIG_SOURCE_SOFT;
         siginfo_static.signal_text = signal_text;
+        msg(D_SIGNAL_DEBUG, "Throw signal (soft): %s (%s)", signal_name(signum, true),
+            signal_text);
+    }
+    else
+    {
+        msg(D_SIGNAL_DEBUG, "Ignoring %s when %s has been received", signal_name(signum, true),
+            signal_name(siginfo_static.signal_received, true));
+    }
+
+    unblock_async_signals();
+}
+
+/**
+ * Register a soft signal in the signal_info struct si respecting priority.
+ * si may be a pointer to the global siginfo_static or a context-specific
+ * signal in a multi-instance or a temporary variable.
+ */
+void
+register_signal(struct signal_info *si, int signum, const char *signal_text)
+{
+    if (si == &siginfo_static) /* attempting to alter the global signal */
+    {
+        block_async_signals();
+    }
+
+    if (try_throw_signal(si, signum, SIG_SOURCE_SOFT))
+    {
+        si->signal_text = signal_text;
+        if (signal_text && strcmp(signal_text, "connection-failed") == 0)
+        {
+            si->source = SIG_SOURCE_CONNECTION_FAILED;
+        }
+        msg(D_SIGNAL_DEBUG, "register signal: %s (%s)", signal_name(signum, true),
+            signal_text);
+    }
+    else
+    {
+        msg(D_SIGNAL_DEBUG, "Ignoring %s when %s has been received", signal_name(signum, true),
+            signal_name(si->signal_received, true));
+    }
+
+    if (si == &siginfo_static)
+    {
+        unblock_async_signals();
     }
 }
 
@@ -239,12 +363,10 @@  signal_restart_status(const struct signal_info *si)
 static void
 signal_handler(const int signum)
 {
-    throw_signal(signum);
-    signal(signum, signal_handler);
+    try_throw_signal(&siginfo_static, signum, SIG_SOURCE_HARD);
 }
 #endif
 
-
 /* set handlers for unix signals */
 
 #define SM_UNDEF     0
@@ -256,13 +378,24 @@  void
 pre_init_signal_catch(void)
 {
 #ifndef _WIN32
+    sigset_t block_mask;
+    struct sigaction sa;
+    CLEAR(sa);
+
+    sigfillset(&block_mask); /* all signals */
+    sa.sa_handler = signal_handler;
+    sa.sa_mask = block_mask;  /* signals blocked inside the handler */
+    sa.sa_flags = SA_RESTART; /* match with the behaviour of signal() on Linux and BSD */
+
     signal_mode = SM_PRE_INIT;
-    signal(SIGINT, signal_handler);
-    signal(SIGTERM, signal_handler);
-    signal(SIGHUP, SIG_IGN);
-    signal(SIGUSR1, SIG_IGN);
-    signal(SIGUSR2, SIG_IGN);
-    signal(SIGPIPE, SIG_IGN);
+    sigaction(SIGINT, &sa, NULL);
+    sigaction(SIGTERM, &sa, NULL);
+
+    sa.sa_handler = SIG_IGN;
+    sigaction(SIGHUP, &sa, NULL);
+    sigaction(SIGUSR1, &sa, NULL);
+    sigaction(SIGUSR2, &sa, NULL);
+    sigaction(SIGPIPE, &sa, NULL);
 #endif /* _WIN32 */
 }
 
@@ -270,14 +403,38 @@  void
 post_init_signal_catch(void)
 {
 #ifndef _WIN32
+    sigset_t block_mask;
+    struct sigaction sa;
+    CLEAR(sa);
+
+    sigfillset(&block_mask); /* all signals */
+    sa.sa_handler = signal_handler;
+    sa.sa_mask = block_mask; /* signals blocked inside the handler */
+    sa.sa_flags = SA_RESTART; /* match with the behaviour of signal() on Linux and BSD */
+
     signal_mode = SM_POST_INIT;
-    signal(SIGINT, signal_handler);
-    signal(SIGTERM, signal_handler);
-    signal(SIGHUP, signal_handler);
-    signal(SIGUSR1, signal_handler);
-    signal(SIGUSR2, signal_handler);
-    signal(SIGPIPE, SIG_IGN);
-#endif
+    sigaction(SIGINT, &sa, NULL);
+    sigaction(SIGTERM, &sa, NULL);
+    sigaction(SIGHUP, &sa, NULL);
+    sigaction(SIGUSR1, &sa, NULL);
+    sigaction(SIGUSR2, &sa, NULL);
+    sa.sa_handler = SIG_IGN;
+    sigaction(SIGPIPE, &sa, NULL);
+#endif /* _WIN32 */
+}
+
+void
+halt_low_priority_signals()
+{
+#ifndef _WIN32
+    struct sigaction sa;
+    CLEAR(sa);
+    sa.sa_handler = SIG_IGN;
+    sigaction(SIGHUP, &sa, NULL);
+    sigaction(SIGUSR1, &sa, NULL);
+    sigaction(SIGUSR2, &sa, NULL);
+#endif /* _WIN32 */
+    ignored_hard_signals_mask = (1LL << SIGHUP) | (1LL << SIGUSR1) | (1LL << SIGUSR2);
 }
 
 /* called after daemonization to retain signal settings */
@@ -341,7 +498,6 @@  print_status(const struct context *c, struct status_output *so)
     gc_free(&gc);
 }
 
-
 /* Small helper function to determine if we should send the exit notification
  * via control channel */
 static inline bool
@@ -371,8 +527,15 @@  process_explicit_exit_notification_init(struct context *c)
     event_timeout_init(&c->c2.explicit_exit_notification_interval, 1, 0);
     reset_coarse_timers(c);
 
-    signal_reset(c->sig);
+    /* Windows exit event will continue trigering SIGTERM -- halt it */
     halt_non_edge_triggered_signals();
+
+    /* Before resetting the signal, ensure hard low priority signals
+     * will be ignored during the exit notification period.
+     */
+    halt_low_priority_signals(); /* Set hard SIGUSR1/SIGHUP/SIGUSR2 to be ignored */
+    signal_reset(c->sig);
+
     c->c2.explicit_exit_notification_time_wait = now;
 
     /* Check if we are in TLS mode and should send the notification via data
@@ -439,33 +602,21 @@  process_sigterm(struct context *c)
 }
 
 /**
- * If a restart signal is received during exit-notification, reset the
- * signal and return true. If its a soft restart signal from the event loop
- * which implies the loop cannot continue, remap to SIGTERM to exit promptly.
+ * If a soft restart signal is received during exit-notification, it
+ * implies the event loop cannot continue: remap to SIGTERM to exit promptly.
+ * Hard restart signals are ignored during exit notification wait.
  */
-static bool
-ignore_restart_signals(struct context *c)
+static void
+remap_restart_signals(struct context *c)
 {
-    bool ret = false;
-    if ( (c->sig->signal_received == SIGUSR1 || c->sig->signal_received == SIGHUP)
-         && event_timeout_defined(&c->c2.explicit_exit_notification_interval) )
+    if ((c->sig->signal_received == SIGUSR1 || c->sig->signal_received == SIGHUP)
+        && event_timeout_defined(&c->c2.explicit_exit_notification_interval)
+        && c->sig->source != SIG_SOURCE_HARD)
     {
-        if (c->sig->source == SIG_SOURCE_HARD)
-        {
-            msg(M_INFO, "Ignoring %s received during exit notification",
-                signal_name(c->sig->signal_received, true));
-            signal_reset(c->sig);
-            ret = true;
-        }
-        else
-        {
-            msg(M_INFO, "Converting soft %s received during exit notification to SIGTERM",
-                signal_name(c->sig->signal_received, true));
-            register_signal(c->sig, SIGTERM, "exit-with-notification");
-            ret = false;
-        }
+        msg(M_INFO, "Converting soft %s received during exit notification to SIGTERM",
+            signal_name(c->sig->signal_received, true));
+        register_signal(c->sig, SIGTERM, "exit-with-notification");
     }
-    return ret;
 }
 
 bool
@@ -473,11 +624,9 @@  process_signal(struct context *c)
 {
     bool ret = true;
 
-    if (ignore_restart_signals(c))
-    {
-        ret = false;
-    }
-    else if (c->sig->signal_received == SIGTERM || c->sig->signal_received == SIGINT)
+    remap_restart_signals(c);
+
+    if (c->sig->signal_received == SIGTERM || c->sig->signal_received == SIGINT)
     {
         ret = process_sigterm(c);
     }
@@ -488,14 +637,3 @@  process_signal(struct context *c)
     }
     return ret;
 }
-
-void
-register_signal(struct signal_info *si, int sig, const char *text)
-{
-    if (signal_priority(sig) >= signal_priority(si->signal_received))
-    {
-        si->signal_received = sig;
-        si->signal_text = text;
-        si->source = SIG_SOURCE_SOFT;
-    }
-}
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index a883ac4a..baafe1e6 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -1588,7 +1588,6 @@  socket_connect(socket_descriptor_t *sd,
         openvpn_close_socket(*sd);
         *sd = SOCKET_UNDEFINED;
         register_signal(sig_info, SIGUSR1, "connection-failed");
-        sig_info->source = SIG_SOURCE_CONNECTION_FAILED;
     }
     else
     {