[Openvpn-devel,3/5] Assign and honour signal priority order
Commit Message
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
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.
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
@@ -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;
}
@@ -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;
}
@@ -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);
}
@@ -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;
}