[Openvpn-devel,v2,4/5] Fix signal handling on Windows
Commit Message
From: Selva Nair <selva.nair@gmail.com>
- In win32_signal_get() re-order the check so that Windows
signals are picked up even if signal_received is non-zero
- When management is not active, management_sleep() becomes sleep()
but it is not interruptible by signals on Windows. Fix this by
periodically checking for signal.
Fixes Trac #311 #639 (windows specific part)
Github: Fixes OpenVPN/openvpn#205 (windows specific part)
Note: if stuck in address resolution, press ctrl-C and wait for getaddrinfo()
to timeout.
v2: WIN32 --> _WIN32
add a chunk in management_sleep that was missed by sloppy conflict-resolution
Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
src/openvpn/manage.c | 16 +++++++-
src/openvpn/win32.c | 98 +++++++++++++++++++++++++++++---------------
src/openvpn/win32.h | 3 ++
3 files changed, 81 insertions(+), 36 deletions(-)
Comments
Hi,
> - else if (n > 0)
> + else
> {
> - sleep(n);
> +#ifdef _WIN32
> + win32_sleep(n);
> +#else
> + if (n > 0)
> + {
> + sleep(n);
My understanding is that we want to have interruptible sleep. In this case,
what is the point of calling win32_sleep(0) ? I understand that
management_sleep(0)
is required to service possible management commands even if we don't
need a delay,
but shouldn't it be a no-op without management?
> +void
> +win32_sleep(const int n)
> +{
> + if (n < 0)
> + {
> + return;
> + }
Why not <= 0 ?
> + /* Sleep() is not interruptible. Use a WAIT_OBJECT to catch signal */
> + if (HANDLE_DEFINED(win32_signal.in.read))
> + {
I would reverse this condition, and do just
Sleep(n*1000);
return;
This way we get rid of the indentation level.
> + time_t expire = 0;
> + update_time();
> + expire = now + n;
I would declare and initialize time_t expire after update_time().
> + DWORD status = WaitForSingleObject(win32_signal.in.read, (expire - now)*1000);
> + if (win32_signal_get(&win32_signal) || status == WAIT_TIMEOUT)
> + {
> + return;
> + }
Shouldn't we call win32_signal_get() only if status == WAIT_OBJECT_0 ?
Under what circumstances are we supposed to do the waiting again? If
we get a signal, we bail out.
If the wait times out, we bail out. If wait fails, we do Sleep
(although at this point we probably have a bigger issue).
-Lev
Hi,
Thanks for the careful review
On Thu, Jan 5, 2023 at 11:20 AM Lev Stipakov <lstipakov@gmail.com> wrote:
> Hi,
>
> > - else if (n > 0)
> > + else
> > {
> > - sleep(n);
> > +#ifdef _WIN32
> > + win32_sleep(n);
> > +#else
> > + if (n > 0)
> > + {
> > + sleep(n);
>
>
> My understanding is that we want to have interruptible sleep. In this case,
> what is the point of calling win32_sleep(0) ?
I understand that
> management_sleep(0)
> is required to service possible management commands even if we don't
> need a delay,
> but shouldn't it be a no-op without management?
>
Right, I'm going beyond the original which does nothing for
management_sleep(0) if management is not active or enabled.
I want to err on the side of checking windows signal rather than missing
it. In case of unix/linux OS signals do get delivered out of band into the
global signal_received. In the case of Windows, it has to be picked up
(except for the obscure ctrl-Break).
Note that management_sleep(0) when management is enabled does call
get_signal()==win32_signal_get and picks up Windows signals, not just
management commands. win32_sleep(0) also calls win32_signal_get() even if
there is no time to pause.
>
> > +void
> > +win32_sleep(const int n)
> > +{
> > + if (n < 0)
> > + {
> > + return;
> > + }
>
> Why not <= 0 ?
>
See above.
>
> > + /* Sleep() is not interruptible. Use a WAIT_OBJECT to catch signal
> */
> > + if (HANDLE_DEFINED(win32_signal.in.read))
> > + {
>
> I would reverse this condition, and do just
>
> Sleep(n*1000);
> return;
> This way we get rid of the indentation level.
>
> > + time_t expire = 0;
> > + update_time();
> > + expire = now + n;
>
> I would declare and initialize time_t expire after update_time().
> > + DWORD status = WaitForSingleObject(win32_signal.in.read,
> (expire - now)*1000);
> > + if (win32_signal_get(&win32_signal) || status ==
> WAIT_TIMEOUT)
> > + {
> > + return;
> > + }
>
> Shouldn't we call win32_signal_get() only if status == WAIT_OBJECT_0 ?
>
To ensure the signal is checked at least once in all cases including n = 0.
Not sure whether status will be WAIT_TIMEOUT or WAIT_OBJECT_0 in case n = 0
and there is a signal to pickup.
>
> Under what circumstances are we supposed to do the waiting again? If
> we get a signal, we bail out.
> If the wait times out, we bail out. If wait fails, we do Sleep
> (although at this point we probably have a bigger issue).
>
Probably never -- there is a WAIT_ABANDONED which may not apply here. But,
sometimes windows functions do exit with error codes not listed in the
docs. As the loop is slowed down by a Sleep(1000), this can't hurt, can it?
The common thread in all of the above is that I'm trying to err on the side
of checking for signals an extra round as opposed to missing it. Can do a
v3 if you think it's warranted.
Thanks,
Selva
Hi
>
>>
>> Under what circumstances are we supposed to do the waiting again? If
>> we get a signal, we bail out.
>> If the wait times out, we bail out. If wait fails, we do Sleep
>> (although at this point we probably have a bigger issue).
>>
>
> Probably never -- there is a WAIT_ABANDONED which may not apply here. But,
> sometimes windows functions do exit with error codes not listed in the
> docs. As the loop is slowed down by a Sleep(1000), this can't hurt, can it?
>
I see your point now :) I had a different logic earlier where it always
slept for a second before trying to wait again, but not so any longer. v3
is coming!
Selva
>
@@ -4045,9 +4045,16 @@ management_sleep(const int n)
{
management_event_loop_n_seconds(management, n);
}
- else if (n > 0)
+ else
{
- sleep(n);
+#ifdef _WIN32
+ win32_sleep(n);
+#else
+ if (n > 0)
+ {
+ sleep(n);
+ }
+#endif
}
}
@@ -4088,13 +4095,18 @@ man_persist_client_stats(struct management *man, struct context *c)
#else /* ifdef ENABLE_MANAGEMENT */
+#include "win32.h"
void
management_sleep(const int n)
{
+#ifdef _WIN32
+ win32_sleep(n);
+#else
if (n > 0)
{
sleep(n);
}
+#endif /* ifdef _WIN32 */
}
#endif /* ENABLE_MANAGEMENT */
@@ -642,50 +642,44 @@ int
win32_signal_get(struct win32_signal *ws)
{
int ret = 0;
- if (siginfo_static.signal_received)
- {
- ret = siginfo_static.signal_received;
- }
- else
+
+ if (ws->mode == WSO_MODE_SERVICE)
{
- if (ws->mode == WSO_MODE_SERVICE)
+ if (win32_service_interrupt(ws))
{
- if (win32_service_interrupt(ws))
- {
- ret = SIGTERM;
- }
+ ret = SIGTERM;
}
- else if (ws->mode == WSO_MODE_CONSOLE)
+ }
+ else if (ws->mode == WSO_MODE_CONSOLE)
+ {
+ switch (win32_keyboard_get(ws))
{
- switch (win32_keyboard_get(ws))
- {
- case 0x3B: /* F1 -> USR1 */
- ret = SIGUSR1;
- break;
+ case 0x3B: /* F1 -> USR1 */
+ ret = SIGUSR1;
+ break;
- case 0x3C: /* F2 -> USR2 */
- ret = SIGUSR2;
- break;
+ case 0x3C: /* F2 -> USR2 */
+ ret = SIGUSR2;
+ break;
- case 0x3D: /* F3 -> HUP */
- ret = SIGHUP;
- break;
+ case 0x3D: /* F3 -> HUP */
+ ret = SIGHUP;
+ break;
- case 0x3E: /* F4 -> TERM */
- ret = SIGTERM;
- break;
+ case 0x3E: /* F4 -> TERM */
+ ret = SIGTERM;
+ break;
- case 0x03: /* CTRL-C -> TERM */
- ret = SIGTERM;
- break;
- }
- }
- if (ret)
- {
- throw_signal(ret); /* this will update signinfo_static.signal received */
+ case 0x03: /* CTRL-C -> TERM */
+ ret = SIGTERM;
+ break;
}
}
- return ret;
+ if (ret)
+ {
+ throw_signal(ret); /* this will update signinfo_static.signal received */
+ }
+ return (siginfo_static.signal_received);
}
void
@@ -1603,4 +1597,40 @@ set_openssl_env_vars()
}
}
+void
+win32_sleep(const int n)
+{
+ if (n < 0)
+ {
+ return;
+ }
+
+ /* Sleep() is not interruptible. Use a WAIT_OBJECT to catch signal */
+ if (HANDLE_DEFINED(win32_signal.in.read))
+ {
+ time_t expire = 0;
+ update_time();
+ expire = now + n;
+ while (expire >= now)
+ {
+ DWORD status = WaitForSingleObject(win32_signal.in.read, (expire - now)*1000);
+ if (win32_signal_get(&win32_signal) || status == WAIT_TIMEOUT)
+ {
+ return;
+ }
+
+ update_time();
+
+ if (status == WAIT_FAILED && expire > now)
+ {
+ Sleep((expire-now)*1000);
+ return;
+ }
+ }
+ }
+ else
+ {
+ Sleep(n*1000);
+ }
+}
#endif /* ifdef _WIN32 */
@@ -330,5 +330,8 @@ openvpn_execve(const struct argv *a, const struct env_set *es, const unsigned in
bool
openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...);
+/* Sleep that can be interrupted by signals and exit event */
+void win32_sleep(const int n);
+
#endif /* ifndef OPENVPN_WIN32_H */
#endif /* ifdef _WIN32 */