Fix interactive service ignoring stop command if openvpn is running

Make the exit event not auto-reset so that the signal propagates to all worker threads and finally to the main thread.

Fixes Trac #666

---
 src/openvpnserv/interactive.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 22239b2..39397d1 100644
@@ -1289,7 +1289,7 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv)
     goto out;
 
   io_event = InitOverlapped (&overlapped);
-  exit_event = CreateEvent (NULL, FALSE, FALSE, NULL);
+  exit_event = CreateEvent (NULL, TRUE, FALSE, NULL);
   if (!exit_event || !io_event)
     {
       error = MsgToEventLog (M_SYSERR, TEXT("Could not create event"));

@@ -1356,6 +1356,7 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv)
             {
               /* exit event signaled */
               CloseHandleEx (&pipe);
+              ResetEvent (exit_event);
               error = NO_ERROR;
               break;
             }

1.7.10.4

A widechar can potentially take more than 2 bytes in UTF-8.

---
 src/openvpnserv/interactive.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

index 39397d1..6a7227b 100644
@@ -1063,17 +1063,16 @@ RunOpenvpn (LPVOID p)
   CloseHandleEx (&stdin_read);
   CloseHandleEx (&svc_pipe);
 
-  DWORD input_size = wcslen (sud.std_input) * 2;
-  if (input_size)
+  DWORD input_size = WideCharToMultiByte (CP_UTF8, 0, sud.std_input, -1, NULL, 0, NULL, NULL);
+  LPSTR input = NULL;
+  if (input_size && (input = malloc (input_size)))
     {
       DWORD written;
-      LPSTR input = malloc (input_size);
       WideCharToMultiByte (CP_UTF8, 0, sud.std_input, -1, input, input_size, NULL, NULL);
       WriteFile (stdin_write, input, strlen (input), &written, NULL);
       free (input);
     }
 
-
   while (TRUE)
     {
       DWORD bytes = PeekNamedPipeAsync (ovpn_pipe, 1, &exit_event);

1.7.10.4

ACK.

https://msdn.microsoft.com/en-us/library/windows/desktop/dd374130(v=vs.85).aspx
explains about WideCharToMultiByte() usage with cbMultiByte==0.

I'm not sure if I understand in which scenarios data is fed to the nascent openvpn.exe on stdin - buf if done at all, we should better do it right :-) - I do wonder, though, if WriteFile() could block here, leading to a dead worker thread... all other pipe operations seem to go through AsyncPipeOp() - but that is orthogonal to this fix)

Your patch has been applied to the master branch.

commit 3654d953eb0bf40fb8c9e1fbaa3de1dd898dcbab

Author: Selva Nair
Date:   Sun Mar 6 00:22:02 2016 -0500

     Use appropriate buffer size for WideCharToMultiByte output in interactive.c

     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/11318
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Hi,

This is used for writing the management password to stdin. The GUI sends an "ascii" password, but a client could potentially use some char set that needs 3 or 4 bytes per char.

And you are right, the write should not block. We don't see any deadlock even when openvpn errros out early, because of buffering, I suppose.. But bettter to make this non-blocking.

Selva

Hi,

Ah! Tanks for the explanation, this is showing my lack of familiarity with using the management interface in a programmatic fashion... (I use it on the server side and manually telnet to it, having the password in a file)

For "short" writes, it shouldn't block, no. On a unix pipe, you'd get blocking if the buffers fill up and the reader is just sitting there and not fetching the data - if the reader *dies*, the server would get an error back (since nobody could ever read it).

Probably not very important, just wondering.

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]...

ACK.

https://msdn.microsoft.com/en-us/library/windows/desktop/ms682396(v=vs.85).aspx
explains the behaviour described in trac#666 (only a single thread is stopping, all the rest does not receive the event due to auto-reset behaviour).

I'm not sure if the ResetEvent() call is actually needed but it's certainly good style to acknowledge in the master thread after all clients are done.

Your patch has been applied to the master branch.

commit 239d09938b300f8eafa12bfb8c43373f0215f7bd

Author: Selva Nair
Date:   Sun Mar 6 00:19:19 2016 -0500

     Fix interactive service ignoring stop command if openvpn is running

     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/11317
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering