diff mbox series

[Openvpn-devel,3/5] Assign and honour signal priority order

Message ID 20230101215109.1521549-4-selva.nair@gmail.com
State Accepted
Headers show
Series Improve signals and avoid losing high priority ones | expand

Commit Message

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

Signals are ordered as SIGUSR2, SIGUSR1, SIGHUP, SIGTERM, SIGINT
in increasing priority. Lower priority signals are not allowed to
overwrite higher ones.

This should fix Trac #311, #639 -- SIGTER/SIGINT lost during dns resolution.
(except for the Windows-specific bug handled in next commit)

On sending SIGTERM during dns resolution, it still takes several seconds
to terminate as the signal will get processed only after getaddrinfo times out
twice (in phase1 and phase2 inits).

Github: fixes OpenVPN/openvpn#205
Note: one has to still wait for address resolution to time out as getaddrinfo()
is no interruptible. But a single ctrl-C (and some patience) is enough.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/proxy.c  |  5 +----
 src/openvpn/sig.c    | 45 ++++++++++++++++++++++++++++++++------------
 src/openvpn/socket.c |  8 ++++++--
 src/openvpn/socks.c  | 14 ++++----------
 4 files changed, 44 insertions(+), 28 deletions(-)

Comments

Arne Schwabe Jan. 6, 2023, 3:12 p.m. UTC | #1 
Am 01.01.23 um 22:51 schrieb selva.nair@gmail.com:
> From: Selva Nair <selva.nair@gmail.com>
> 
> Signals are ordered as SIGUSR2, SIGUSR1, SIGHUP, SIGTERM, SIGINT
> in increasing priority. Lower priority signals are not allowed to
> overwrite higher ones.
> 
> This should fix Trac #311, #639 -- SIGTER/SIGINT lost during dns resolution.
> (except for the Windows-specific bug handled in next commit)
> 
> On sending SIGTERM during dns resolution, it still takes several seconds
> to terminate as the signal will get processed only after getaddrinfo times out
> twice (in phase1 and phase2 inits).
> 
> Github: fixes OpenVPN/openvpn#205
> Note: one has to still wait for address resolution to time out as getaddrinfo()
> is no interruptible. But a single ctrl-C (and some patience) is enough.

Thanks. This looks like a good approach. I know there is some other 
similar bug somewhere else that has been forever on my todo list of my 
Android app and also the problem with signal ordering.

Acked-By: Arne Schwabe <arne@rfc2549.org>

While reviewing the patch, I noticed that 
SIG_SOURCE_SOFT/SIG_SOURCE_HARD seems to be purely informational and 
have no real other meaning.
Gert Doering Jan. 8, 2023, 6 p.m. UTC | #2 
I've stared at the code for quite a bit, and this looks good, and
cleans up quite a few of the odd corner cases ("do not set signal
if some other signal is there already") that have grown over time,
like the one in proxy.c.

I have not manually tested this to see if I could break it, or reproduce
one of the "known issues" - I might go back and do so, but today I am
lacking time.  I *have* fed this to the full set of client/server tests,
FreeBSD, Linux, Linux with DCO, which do excercise the SIGUSR1/SIGTERM
on the p2mp server a bit and SIGUSR1/SIGTERM on the client, and all
is nicely well-behaved.

Your patch has been applied to the master and release/2.6 branch.

commit 3a7257925791a086c1ac88362a8eb422be518b14 (master)
commit b3b1436955b9db8e557fc58b7e37ba3a881109a6 (release/2.6)
Author: Selva Nair
Date:   Sun Jan 1 16:51:07 2023 -0500

     Assign and honour signal priority order

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20230101215109.1521549-4-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25871.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/proxy.c b/src/openvpn/proxy.c
index 91121f25..120ba85e 100644
--- a/src/openvpn/proxy.c
+++ b/src/openvpn/proxy.c
@@ -1080,10 +1080,7 @@  done:
     return ret;
 
 error:
-    if (!sig_info->signal_received)
-    {
-        register_signal(sig_info, SIGUSR1, "HTTP proxy error"); /* SOFT-SIGUSR1 -- HTTP proxy error */
-    }
+    register_signal(sig_info, SIGUSR1, "HTTP proxy error"); /* SOFT-SIGUSR1 -- HTTP proxy error */
     gc_free(&gc);
     return ret;
 }
diff --git a/src/openvpn/sig.c b/src/openvpn/sig.c
index e462b93e..d6b18cb1 100644
--- a/src/openvpn/sig.c
+++ b/src/openvpn/sig.c
@@ -47,16 +47,17 @@  struct signal_info siginfo_static; /* GLOBAL */
 
 struct signame {
     int value;
+    int priority;
     const char *upper;
     const char *lower;
 };
 
 static const struct signame signames[] = {
-    { SIGINT,  "SIGINT",  "sigint"},
-    { SIGTERM, "SIGTERM", "sigterm" },
-    { SIGHUP,  "SIGHUP",  "sighup" },
-    { SIGUSR1, "SIGUSR1", "sigusr1" },
-    { SIGUSR2, "SIGUSR2", "sigusr2" }
+    { SIGINT, 5, "SIGINT",  "sigint"},
+    { SIGTERM, 4, "SIGTERM", "sigterm" },
+    { SIGHUP, 3, "SIGHUP",  "sighup" },
+    { SIGUSR1, 2, "SIGUSR1", "sigusr1" },
+    { SIGUSR2, 1, "SIGUSR2", "sigusr2" }
 };
 
 int
@@ -73,6 +74,19 @@  parse_signal(const char *signame)
     return -1;
 }
 
+static int
+signal_priority(int sig)
+{
+    for (size_t i = 0; i < SIZE(signames); ++i)
+    {
+        if (sig == signames[i].value)
+        {
+            return signames[i].priority;
+        }
+    }
+    return -1;
+}
+
 const char *
 signal_name(const int sig, const bool upper)
 {
@@ -103,16 +117,22 @@  signal_description(const int signum, const char *sigtext)
 void
 throw_signal(const int signum)
 {
-    siginfo_static.signal_received = signum;
-    siginfo_static.source = SIG_SOURCE_HARD;
+    if (signal_priority(signum) >= signal_priority(siginfo_static.signal_received))
+    {
+        siginfo_static.signal_received = signum;
+        siginfo_static.source = SIG_SOURCE_HARD;
+    }
 }
 
 void
 throw_signal_soft(const int signum, const char *signal_text)
 {
-    siginfo_static.signal_received = signum;
-    siginfo_static.source = SIG_SOURCE_SOFT;
-    siginfo_static.signal_text = signal_text;
+    if (signal_priority(signum) >= signal_priority(siginfo_static.signal_received))
+    {
+        siginfo_static.signal_received = signum;
+        siginfo_static.source = SIG_SOURCE_SOFT;
+        siginfo_static.signal_text = signal_text;
+    }
 }
 
 void
@@ -472,9 +492,10 @@  process_signal(struct context *c)
 void
 register_signal(struct signal_info *si, int sig, const char *text)
 {
-    if (si->signal_received != SIGTERM)
+    if (signal_priority(sig) >= signal_priority(si->signal_received))
     {
         si->signal_received = sig;
+        si->signal_text = text;
+        si->source = SIG_SOURCE_SOFT;
     }
-    si->signal_text = text;
 }
diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index faaa2748..59d89352 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -2277,8 +2277,12 @@  link_socket_init_phase2(struct context *c)
 done:
     if (sig_save.signal_received)
     {
-        /* This can potentially lose a saved high priority signal -- to be fixed */
-        if (!sig_info->signal_received)
+        /* Always restore the saved signal -- register/throw_signal will handle priority */
+        if (sig_save.source == SIG_SOURCE_HARD && sig_info == &siginfo_static)
+        {
+            throw_signal(sig_save.signal_received);
+        }
+        else
         {
             register_signal(sig_info, sig_save.signal_received, sig_save.signal_text);
         }
diff --git a/src/openvpn/socks.c b/src/openvpn/socks.c
index b2ca3744..8f2ae226 100644
--- a/src/openvpn/socks.c
+++ b/src/openvpn/socks.c
@@ -499,11 +499,8 @@  establish_socks_proxy_passthru(struct socks_proxy_info *p,
     return;
 
 error:
-    if (!sig_info->signal_received)
-    {
-        /* SOFT-SIGUSR1 -- socks error */
-        register_signal(sig_info, SIGUSR1, "socks-error");
-    }
+    /* SOFT-SIGUSR1 -- socks error */
+    register_signal(sig_info, SIGUSR1, "socks-error");
     return;
 }
 
@@ -543,11 +540,8 @@  establish_socks_proxy_udpassoc(struct socks_proxy_info *p,
     return;
 
 error:
-    if (!sig_info->signal_received)
-    {
-        /* SOFT-SIGUSR1 -- socks error */
-        register_signal(sig_info, SIGUSR1, "socks-error");
-    }
+    /* SOFT-SIGUSR1 -- socks error */
+    register_signal(sig_info, SIGUSR1, "socks-error");
     return;
 }