Re: [Openvpn-devel] [PATCH 2/2] signal_reset(): combine check and reset operations
On Tue, Jul 25, 2023 at 01:42:41PM -0400, Selva Nair wrote:
> On Tue, Jul 25, 2023 at 6:18 AM Frank Lichtenheld
> wrote:
>
> > On Sat, Jan 28, 2023 at 04:59:01PM -0500, selva.n...@gmail.com wrote:
> > > From: Selva Nair
> > >
> > > - "if (sig == X) signal_reset(sig)" now becomes
> > > "signal_reset(sig, X)" so that the check and assignment
> > > can be done in one place where signals are masked.
> > > This is required to avoid change of signal state between
> > > check and reset operations.
> > >
> > > - Avoid resetting the signal except when absolutely necessary
> > > (resetting has the potential of losing signals)
> > >
> > > - In 'pre_init_signal_catch()', when certain low priority signals
> > > are set to SIG_IGN, clear any pending signals of the same
> > > type. Also, reset signal at the end of the SIGUSR1 and
> > > SIGHUP loops where their values are checked instead of later. This
> > > avoids the need for 'signal_reset()' after SIGHUP or in
> > 'init_instance()'
> > > which could cause a signal like SIGTERM to be lost.
> > >
> > [...]
> > > diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c
> > > index cba58276..ad0aa8a2 100644
> > > --- a/src/openvpn/openvpn.c
> > > +++ b/src/openvpn/openvpn.c
> > > @@ -194,7 +194,6 @@ openvpn_main(int argc, char *argv[])
> > > context_clear_all_except_first_time();
> > >
> > > /* static signal info object */
> > > -CLEAR(siginfo_static);
> > > c.sig = _static;
> >
> > Is that actually save? Doesn't that mean that siginfo_static might be
> > used uninitialized?
> >
>
> siginfo_static is a global variable with no explicit initialization, so it
> is guaranteed to
> get initialized to {0} at start up.
Right.
Stared at code, all makes sense to me. Didn't do any testing specific to the
changes.
Acked-by: Frank Lichtenheld
Regards,
--
Frank Lichtenheld
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/2] signal_reset(): combine check and reset operations
On Tue, Jul 25, 2023 at 6:18 AM Frank Lichtenheld
wrote:
> On Sat, Jan 28, 2023 at 04:59:01PM -0500, selva.n...@gmail.com wrote:
> > From: Selva Nair
> >
> > - "if (sig == X) signal_reset(sig)" now becomes
> > "signal_reset(sig, X)" so that the check and assignment
> > can be done in one place where signals are masked.
> > This is required to avoid change of signal state between
> > check and reset operations.
> >
> > - Avoid resetting the signal except when absolutely necessary
> > (resetting has the potential of losing signals)
> >
> > - In 'pre_init_signal_catch()', when certain low priority signals
> > are set to SIG_IGN, clear any pending signals of the same
> > type. Also, reset signal at the end of the SIGUSR1 and
> > SIGHUP loops where their values are checked instead of later. This
> > avoids the need for 'signal_reset()' after SIGHUP or in
> 'init_instance()'
> > which could cause a signal like SIGTERM to be lost.
> >
> [...]
> > diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c
> > index cba58276..ad0aa8a2 100644
> > --- a/src/openvpn/openvpn.c
> > +++ b/src/openvpn/openvpn.c
> > @@ -194,7 +194,6 @@ openvpn_main(int argc, char *argv[])
> > context_clear_all_except_first_time();
> >
> > /* static signal info object */
> > -CLEAR(siginfo_static);
> > c.sig = _static;
>
> Is that actually save? Doesn't that mean that siginfo_static might be
> used uninitialized?
>
siginfo_static is a global variable with no explicit initialization, so it
is guaranteed to
get initialized to {0} at start up.
Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/2] signal_reset(): combine check and reset operations
On Sat, Jan 28, 2023 at 04:59:01PM -0500, selva.n...@gmail.com wrote: > From: Selva Nair > > - "if (sig == X) signal_reset(sig)" now becomes > "signal_reset(sig, X)" so that the check and assignment > can be done in one place where signals are masked. > This is required to avoid change of signal state between > check and reset operations. > > - Avoid resetting the signal except when absolutely necessary > (resetting has the potential of losing signals) > > - In 'pre_init_signal_catch()', when certain low priority signals > are set to SIG_IGN, clear any pending signals of the same > type. Also, reset signal at the end of the SIGUSR1 and > SIGHUP loops where their values are checked instead of later. This > avoids the need for 'signal_reset()' after SIGHUP or in 'init_instance()' > which could cause a signal like SIGTERM to be lost. > [...] > diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c > index cba58276..ad0aa8a2 100644 > --- a/src/openvpn/openvpn.c > +++ b/src/openvpn/openvpn.c > @@ -194,7 +194,6 @@ openvpn_main(int argc, char *argv[]) > context_clear_all_except_first_time(); > > /* static signal info object */ > -CLEAR(siginfo_static); > c.sig = _static; Is that actually save? Doesn't that mean that siginfo_static might be used uninitialized? Regards, -- Frank Lichtenheld ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 2/2] signal_reset(): combine check and reset operations
From: Selva Nair
- "if (sig == X) signal_reset(sig)" now becomes
"signal_reset(sig, X)" so that the check and assignment
can be done in one place where signals are masked.
This is required to avoid change of signal state between
check and reset operations.
- Avoid resetting the signal except when absolutely necessary
(resetting has the potential of losing signals)
- In 'pre_init_signal_catch()', when certain low priority signals
are set to SIG_IGN, clear any pending signals of the same
type. Also, reset signal at the end of the SIGUSR1 and
SIGHUP loops where their values are checked instead of later. This
avoids the need for 'signal_reset()' after SIGHUP or in 'init_instance()'
which could cause a signal like SIGTERM to be lost.
Signed-off-by: Selva Nair
---
src/openvpn/init.c| 3 ---
src/openvpn/multi.c | 5 ++---
src/openvpn/openvpn.c | 5 ++---
src/openvpn/sig.c | 40 +---
src/openvpn/sig.h | 7 ++-
src/openvpn/socket.c | 5 ++---
src/openvpn/win32.c | 2 +-
7 files changed, 46 insertions(+), 21 deletions(-)
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index b500d354..76a7be7b 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -4299,9 +4299,6 @@ init_instance(struct context *c, const struct env_set
*env, const unsigned int f
do_inherit_env(c, env);
}
-/* signals caught here will abort */
-signal_reset(c->sig);
-
if (c->mode == CM_P2P)
{
init_management_callback_p2p(c);
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index f2559016..c52c8f14 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3868,7 +3868,7 @@ multi_push_restart_schedule_exit(struct multi_context *m,
bool next_server)
>deferred_shutdown_signal.wakeup,
compute_wakeup_sigma(>deferred_shutdown_signal.wakeup));
-signal_reset(m->top.sig);
+signal_reset(m->top.sig, 0);
}
/*
@@ -3878,12 +3878,11 @@ multi_push_restart_schedule_exit(struct multi_context
*m, bool next_server)
bool
multi_process_signal(struct multi_context *m)
{
-if (m->top.sig->signal_received == SIGUSR2)
+if (signal_reset(m->top.sig, SIGUSR2) == SIGUSR2)
{
struct status_output *so = status_open(NULL, 0, M_INFO, NULL, 0);
multi_print_status(m, so, m->status_file_version);
status_close(so);
-signal_reset(m->top.sig);
return false;
}
else if (proto_is_dgram(m->top.options.ce.proto)
diff --git a/src/openvpn/openvpn.c b/src/openvpn/openvpn.c
index cba58276..ad0aa8a2 100644
--- a/src/openvpn/openvpn.c
+++ b/src/openvpn/openvpn.c
@@ -194,7 +194,6 @@ openvpn_main(int argc, char *argv[])
context_clear_all_except_first_time();
/* static signal info object */
-CLEAR(siginfo_static);
c.sig = _static;
/* initialize garbage collector scoped to context object */
@@ -333,14 +332,14 @@ openvpn_main(int argc, char *argv[])
/* pass restart status to management subsystem */
signal_restart_status(c.sig);
}
-while (c.sig->signal_received == SIGUSR1);
+while (signal_reset(c.sig, SIGUSR1) == SIGUSR1);
env_set_destroy(c.es);
uninit_options();
gc_reset();
uninit_early();
}
-while (c.sig->signal_received == SIGHUP);
+while (signal_reset(c.sig, SIGHUP) == SIGHUP);
}
context_gc_free();
diff --git a/src/openvpn/sig.c b/src/openvpn/sig.c
index 559ca35d..4eead996 100644
--- a/src/openvpn/sig.c
+++ b/src/openvpn/sig.c
@@ -259,15 +259,37 @@ register_signal(struct signal_info *si, int signum, const
char *signal_text)
}
}
-void
-signal_reset(struct signal_info *si)
+/**
+ * Clear the signal if its current value equals signum. If
+ * signum is zero the signal is cleared independent of its current
+ * value. Returns the current value of the signal.
+ */
+int
+signal_reset(struct signal_info *si, int signum)
{
+int sig_saved = 0;
if (si)
{
-si->signal_received = 0;
-si->signal_text = NULL;
-si->source = SIG_SOURCE_SOFT;
+if (si == _static) /* attempting to alter the global signal */
+{
+block_async_signals();
+}
+
+sig_saved = si->signal_received;
+if (!signum || sig_saved == signum)
+{
+si->signal_received = 0;
+si->signal_text = NULL;
+si->source = SIG_SOURCE_SOFT;
+msg(D_SIGNAL_DEBUG, "signal_reset: signal %s is cleared",
signal_name(signum, true));
+}
+
+if (si == _static)
+{
+unblock_async_signals();
+}
}
+return sig_saved;
}
void
@@ -397,6 +419,10 @@ pre_init_signal_catch(void)
sigaction(SIGUSR2, , NULL);
sigaction(SIGPIPE, , NULL);
#endif /* _WIN32 */
+/* clear
