Fix termination when windows suspends/sleeps

When TUN/TAP I/O operation is aborted, restart with a SIGHUP instead of terminate. The abort error from TAP is often triggered by system suspend which is fully recoverable on resume. Catastrophic events will get caught later during the restart. This solves the abnormal termination during suspend/resume.

---
 src/openvpn/forward.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 62eb6fc..cef063d 100644
@@ -958,8 +958,9 @@ read_incoming_tun (struct context *c)
   /* Was TUN/TAP I/O operation aborted? */
   if (tuntap_abort(c->c2.buf.len))
   {
-     register_signal(c, SIGTERM, "tun-abort");
-     msg(M_FATAL, "TUN/TAP I/O operation aborted, exiting");
+     register_signal(c, SIGHUP, "tun-abort");
+     c->persist.restart_sleep_seconds = 10;
+     msg(M_INFO, "TUN/TAP I/O operation aborted, restarting");
      perf_pop();
      return;
   }

2.6.2

ACK. Explanation makes sense, logs and testers demonstrate that it indeed fixes a significant problem, and the code is sane :-) - thanks.

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

commit ea66a2b5cdb21422139c421b4d3733e1c1c3937e (master) commit 0d4ba251879c702b9474e26ff73a4f559d922d4f (release/2.3)

Author: Selva Nair
Date:   Wed Nov 4 13:59:38 2015 -0500

     Fix termination when windows suspends/sleeps

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/10438
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Hi,

Uh, and because I forgot to mention it - Samuli's buildbot will build a windows snapshot from git master containing this, and it will be part of 2.3.9 when we release this (no timeline yet).

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Hi,

Thanks.

Speaking of the windows installer, the GUI needs a small change to play nice with this patch. Else the GUI will end up killing openvpn on resume.. Not sure where to send patches for the GUI.

Selva

Hi,

For now you can send the patch to this list. I'll add OpenVPN-GUI to OpenVPN project's GitHub page soon, after which you can just issue a pull request.

Samuli

With commit ea66a2b5c.. openvpn restarts instead of terminate on suspend. This conflicts with the stop/restart logic in the gui during windows power state change events. Here we change the default behaviour so that SIGTERM is not triggered during windows suspend.

Requires the installer to update the registry during an upgrade of exisitng installations, or notify the end user of the change in the default value of disconnect_on_suspend (0).

---
 registry.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index f96827a..16af368 100644
@@ -111,7 +111,7 @@ GetRegistryKeys()
 
   if (!GetRegKey(_T("show_script_window"), o.show_script_window, _T("1"), _countof(o.show_script_window))) return(false);
 
-  if (!GetRegKey(_T("disconnect_on_suspend"), o.disconnect_on_suspend, _T("1"), 
+  if (!GetRegKey(_T("disconnect_on_suspend"), o.disconnect_on_suspend, _T("0"), 
       _countof(o.disconnect_on_suspend))) return(false);
 
   if (!GetRegKey(_T("passphrase_attempts"), o.psw_attempts_string, _T("3"), 

2.6.2

Hi,

What does this patch *do*? Is this the "set up registry key on first start" part (so we set the default differently now)?

But that implies that an upgrade will not change the default, right?

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Hi,

You are right, the current openvpn installer does not rewrite the registry for the GUI, so an update does not change the default and that is not good. I left that to the package maintainer with that comment about the installer and upgrading.

I would have preferred to completely remove the handling of PBT_APMSUSPEND event in main.c but did not wanted to make pervasive changes..

Instead, one could just modify openvpn.nsi with an added line in the GUI install section:

DelRegKeyIfUnchanged HKLM "SOFTWARE\${PACKAGE_NAME}_GUI}\disconnect_on_suspend" "1"

Selva

On further thought, its better to go more bold on this. Here is version 2 which culls the suspend processing.

--8<--

---
 main.c     | 14 +-------------
 registry.c |  3 ---
 2 files changed, 1 insertion(+), 16 deletions(-)

index 08c5663..bc3c41c 100644
@@ -399,19 +399,7 @@ LRESULT CALLBACK WindowProcedure (HWND hwnd, UINT message, WPARAM wParam, LPARAM
     case WM_POWERBROADCAST:
       switch (wParam) {
         case PBT_APMSUSPEND:
-          if (o.disconnect_on_suspend[0] == '1')
-            {
-              /* Suspend running connections */
-              for (i=0; i<o.num_configs; i++)
-                {
-                  if (o.conn[i].state == connected)
-                SuspendOpenVPN(i);
-                }
-
-              /* Wait for all connections to suspend */
-              for (i=0; i<10; i++, Sleep(500))
-                if (CountConnState(suspending) == 0) break;
-            }
+           /* do nothing */
           return FALSE;
 
         case PBT_APMRESUMESUSPEND:

index f96827a..2a75738 100644
@@ -111,9 +111,6 @@ GetRegistryKeys()
 
   if (!GetRegKey(_T("show_script_window"), o.show_script_window, _T("1"), _countof(o.show_script_window))) return(false);
 
-  if (!GetRegKey(_T("disconnect_on_suspend"), o.disconnect_on_suspend, _T("1"), 
-      _countof(o.disconnect_on_suspend))) return(false);
-
   if (!GetRegKey(_T("passphrase_attempts"), o.psw_attempts_string, _T("3"), 
       _countof(o.psw_attempts_string))) return(false);
   o.psw_attempts = _ttoi(o.psw_attempts_string);

2.6.2

Hi,

I like the approach of "the installer changes the default, but if someone *wants* this functionality, they can still enable it by changing the (poorly documented, alas) registry key" better...

Leaving it in but having it disabled-by-default might be useful for different deployment scenarios than you have in mind. It's there for a reason, and much longer than the tap6 driver existed, so it fixed other issues... (like, "reconnect on a laptop after network change", where you might need to wait for a --ping timeout otherwise)

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...