From 9892d5813f1223e5075ef74a195d6e2238698240 Mon Sep 17 00:00:00 2001 From: Selva Nair Date: Wed, 22 Jun 2016 21:48:19 -0400 Subject: [PATCH 1/3] NUL terminate messages received from interactive service Signed-off-by: Selva Nair --- openvpn.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/openvpn.c b/openvpn.c index 30b3c22..068b443 100644 --- a/openvpn.c +++ b/openvpn.c @@ -658,10 +658,15 @@ HandleServiceIO (DWORD err, DWORD bytes, LPOVERLAPPED lpo) int len, capacity; len = _countof(s->readbuf); - capacity = len*sizeof(*(s->readbuf)); + capacity = (len-1)*sizeof(*(s->readbuf)); if (bytes > 0) + { + /* messages from the service are not nul terminated */ + int nchars = bytes/sizeof(s->readbuf[0]); + s->readbuf[nchars] = L'\0'; SetEvent (s->hEvent); + } if (err) { _snwprintf(s->readbuf, len, L"0x%08x\nInteractive Service disconnected\n", err); From fcd0efa479737bb32c415554b73887f558a8048e Mon Sep 17 00:00:00 2001 From: Selva Nair Date: Thu, 23 Jun 2016 12:38:20 -0400 Subject: [PATCH 2/3] Terminate any openvpn processes that fail to stop Sometimes gracefully stopping openvpn fails leaving the process running in background. This causes restarting of connections to fail until those processes are manually killed. - Read process ID from interactive service to get process handle when openvpn is started by the service. - Add a last resort method to forcefully terminate openvpn process that fails to exit aftier sending stop signal. Terminate is triggered after a 3 second timeout following Stop. Signed-off-by: Selva Nair --- openvpn-gui-res.h | 3 +++ openvpn.c | 66 +++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/openvpn-gui-res.h b/openvpn-gui-res.h index 5e38139..a1000e8 100644 --- a/openvpn-gui-res.h +++ b/openvpn-gui-res.h @@ -296,4 +296,7 @@ /* Save password related messages */ #define IDS_NFO_DELETE_PASS 2001 +/* Timer IDs */ +#define IDT_STOP_TIMER 2500 /* Timer used to trigger force termination */ + #endif diff --git a/openvpn.c b/openvpn.c index 068b443..ee5c1a1 100644 --- a/openvpn.c +++ b/openvpn.c @@ -54,6 +54,9 @@ extern options_t o; +static BOOL +TerminateOpenVPN(connection_t *c); + const TCHAR *cfgProp = _T("conn"); typedef struct { @@ -719,6 +722,7 @@ static void OnService(connection_t *c, UNUSED char *msg) { DWORD err = 0; + DWORD pid = 0; WCHAR *p, *buf, *next; DWORD len; const WCHAR *prefix = L"IService> "; @@ -735,6 +739,17 @@ OnService(connection_t *c, UNUSED char *msg) } p = buf + 11; + if (!err && swscanf (p, L"0x%08x\nProcess ID", &pid) == 1 && pid != 0) + { + PrintDebug (L"Process ID of openvpn started by IService: %d", pid); + c->hProcess = OpenProcess (PROCESS_TERMINATE|PROCESS_QUERY_INFORMATION, FALSE, pid); + if (!c->hProcess) + PrintDebug (L"Failed to get process handle from pid of openvpn: error = %lu", + GetLastError()); + free (buf); + return; + } + while (iswspace(*p)) ++p; while (p && *p) @@ -796,10 +811,11 @@ Cleanup (connection_t *c) if (c->hProcess) CloseHandle (c->hProcess); - else - CloseServiceIO (&c->iserv); c->hProcess = NULL; + if (c->iserv.hEvent) + CloseServiceIO (&c->iserv); + if (c->exit_event) CloseHandle (c->exit_event); c->exit_event = NULL; @@ -931,6 +947,7 @@ StatusDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) SetMenuStatus(c, disconnecting); SetDlgItemText(c->hwndStatus, ID_TXT_STATUS, LoadLocalizedString(IDS_NFO_STATE_WAIT_TERM)); SetEvent(c->exit_event); + SetTimer(hwndDlg, IDT_STOP_TIMER, 3000, NULL); break; case WM_OVPN_SUSPEND: @@ -941,6 +958,18 @@ StatusDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) SetMenuStatus(c, disconnecting); SetDlgItemText(c->hwndStatus, ID_TXT_STATUS, LoadLocalizedString(IDS_NFO_STATE_WAIT_TERM)); SetEvent(c->exit_event); + SetTimer(hwndDlg, IDT_STOP_TIMER, 3000, NULL); + break; + + case WM_TIMER: + PrintDebug(L"WM_TIMER message with wParam = %lu", wParam); + c = (connection_t *) GetProp(hwndDlg, cfgProp); + if (wParam == IDT_STOP_TIMER) + { + /* openvpn failed to respond to stop signal -- terminate */ + TerminateOpenVPN(c); + KillTimer (hwndDlg, IDT_STOP_TIMER); + } break; } return FALSE; @@ -979,7 +1008,7 @@ ThreadOpenVPNStatus(void *p) PostMessage(c->hwndStatus, WM_CLOSE, 0, 0); /* Start the async read loop for service and set it as the wait event */ - if (!c->hProcess) + if (c->iserv.hEvent) { HandleServiceIO (0, 0, (LPOVERLAPPED) &c->iserv); wait_event = c->iserv.hEvent; @@ -999,9 +1028,9 @@ ThreadOpenVPNStatus(void *p) if ((res = MsgWaitForMultipleObjectsEx (1, &wait_event, INFINITE, QS_ALLINPUT, MWMO_ALERTABLE)) == WAIT_OBJECT_0) { - if (c->hProcess) + if (wait_event == c->hProcess) OnProcess (c, NULL); - else + else if (wait_event == c->iserv.hEvent) OnService (c, NULL); } continue; @@ -1224,6 +1253,33 @@ StopOpenVPN(connection_t *c) PostMessage(c->hwndStatus, WM_OVPN_STOP, 0, 0); } +/* force-kill as a last resort */ +static BOOL +TerminateOpenVPN (connection_t *c) +{ + DWORD exit_code = 0; + BOOL retval = TRUE; + + if (!c->hProcess) + return retval; + if (!GetExitCodeProcess (c->hProcess, &exit_code)) + { + PrintDebug (L"In TerminateOpenVPN: failed to get process status: error = %lu", GetLastError()); + return FALSE; + } + if (exit_code == STILL_ACTIVE) + { + retval = TerminateProcess (c->hProcess, 1); + if (retval) + PrintDebug (L"Openvpn Process for config '%s' terminated", c->config_name); + else + PrintDebug (L"Failed to terminate openvpn Process for config '%s'", c->config_name); + } + else + PrintDebug(L"In TerminateOpenVPN: Process is not active"); + + return retval; +} void SuspendOpenVPN(int config) From ad58766f523ee4c8f22b00ffd905398beb943c15 Mon Sep 17 00:00:00 2001 From: Selva Nair Date: Sat, 8 Oct 2016 12:50:01 -0400 Subject: [PATCH 3/3] Do not start a connection when a previous thread has not fully exited When openvpn exits due to error, the GUI pops up a modal dialog and waits on user to click OK before cleaning up resources and closing the status window. During this phase if the user clicks "connect" from the tray menu, a new thread is started overwriiting several handles in the connection struct. Fix: Refuse to start a connection when previous status thread is still active. Instead, bring the exisiting status window to fore-ground. Also make the modal dialog a child of the status window for better visibility. Signed-off-by: Selva Nair --- openvpn.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/openvpn.c b/openvpn.c index ee5c1a1..3356876 100644 --- a/openvpn.c +++ b/openvpn.c @@ -474,7 +474,8 @@ OnStop(connection_t *c, UNUSED char *msg) SetForegroundWindow(c->hwndStatus); ShowWindow(c->hwndStatus, SW_SHOW); } - ShowLocalizedMsg(IDS_NFO_CONN_TERMINATED, c->config_name); + MessageBox(c->hwndStatus, LoadLocalizedString(IDS_NFO_CONN_TERMINATED, c->config_file), + _T(PACKAGE_NAME), MB_OK); SendMessage(c->hwndStatus, WM_CLOSE, 0, 0); break; @@ -501,7 +502,7 @@ OnStop(connection_t *c, UNUSED char *msg) SetForegroundWindow(c->hwndStatus); ShowWindow(c->hwndStatus, SW_SHOW); } - ShowLocalizedMsg(msg_id, msg_xtra); + MessageBox(c->hwndStatus, LoadLocalizedString(msg_id, msg_xtra), _T(PACKAGE_NAME), MB_OK); SendMessage(c->hwndStatus, WM_CLOSE, 0, 0); break; @@ -1045,6 +1046,7 @@ ThreadOpenVPNStatus(void *p) /* release handles etc.*/ Cleanup (c); + c->hwndStatus = NULL; return 0; } @@ -1089,6 +1091,15 @@ StartOpenVPN(connection_t *c) CLEAR(c->ip); + if (c->hwndStatus) + { + PrintDebug(L"Connection request when previous status window is still open -- ignored"); + WriteStatusLog(c, L"OpenVPN GUI> ", + L"Complete the pending dialog before starting a new connection", false); + SetForegroundWindow(c->hwndStatus); + return FALSE; + } + RunPreconnectScript(c); /* Create thread to show the connection's status dialog */