diff options
author | Selva Nair | 2016-05-22 14:39:32 -0400 |
---|---|---|
committer | Gert Doering | 2016-05-22 22:34:40 +0200 |
commit | 600dd9a16fc61ff6e595f500fba5daf14248b739 (patch) | |
tree | 3f9cca69a1a57a8e89a6d42575de41c7c2e258e6 /src/openvpnserv | |
parent | 698f0dab76741f4ce8c1a98236786d59eca338ef (diff) | |
download | openvpn-600dd9a16fc61ff6e595f500fba5daf14248b739.zip openvpn-600dd9a16fc61ff6e595f500fba5daf14248b739.tar.gz |
Fix handling of out of memory error in interactive service
Currently realloc failure in UpdateWaitHandles() is handled by
triggering exit_event and waiting for all active worker threads
to terminate. However, at this point the wait handles array
will contain an invalid value (handle of the latest thread that
is terminated), causing a cycle of WAIT_FAILED <-> continue and
trashing of the eventlog.
Fix:
- Update the wait handles again after removing the last thread:
this should not fail as no extra memory is needed. Do not set
the exit event; existing connections are not terminated.
- In case of WAIT_FAILED, break out of the while loop and exit
instead of continue. This usually happens when one or more
handles are invalid, which is hard to recover from.
Other changes:
- Use minimal initial allocation size so that the realloc code path
gets exercised (2 or more connections will cause realloc).
- Use a temp variable to check the return value of realloc().
- Initialize handles array pointer to NULL.
v2 changes:
- Increased initial allocation to 10 (warn: now 10 or more connections
needed to exercise the realloc code path).
- Moved up the declaration of "LPHANDLE tmp" to please stone-age MSVC.
Tested using a dummy realloc that returns NULL.
Signed-off-by: Selva Nair <selva.nair@gmail.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <1463942372-26958-1-git-send-email-selva.nair@gmail.com>
URL: http://article.gmane.org/gmane.network.openvpn.devel/11708
Signed-off-by: Gert Doering <gert@greenie.muc.de>
Diffstat (limited to 'src/openvpnserv')
-rw-r--r-- | src/openvpnserv/interactive.c | 34 |
1 files changed, 24 insertions, 10 deletions
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index df30ad7..2453f17 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -1460,6 +1460,7 @@ UpdateWaitHandles (LPHANDLE *handles_ptr, LPDWORD count, if (handles == NULL) { handles = malloc (size * sizeof (HANDLE)); + *handles_ptr = handles; if (handles == NULL) return ERROR_OUTOFMEMORY; } @@ -1473,16 +1474,22 @@ UpdateWaitHandles (LPHANDLE *handles_ptr, LPDWORD count, { if (pos == size) { + LPHANDLE tmp; size += 10; - handles = realloc (handles, size * sizeof (HANDLE)); - if (handles == NULL) - return ERROR_OUTOFMEMORY; + tmp = realloc (handles, size * sizeof (HANDLE)); + if (tmp == NULL) + { + size -= 10; + *count = pos; + return ERROR_OUTOFMEMORY; + } + handles = tmp; + *handles_ptr = handles; } handles[pos++] = threads->data; threads = threads->next; } - *handles_ptr = handles; *count = pos; return NO_ERROR; } @@ -1502,8 +1509,9 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv) OVERLAPPED overlapped; DWORD error = NO_ERROR; list_item_t *threads = NULL; - PHANDLE handles; + PHANDLE handles = NULL; DWORD handle_count; + BOOL CmpHandle (LPVOID item, LPVOID hnd) { return item == hnd; } service = RegisterServiceCtrlHandlerEx (interactive_service.name, ServiceCtrlInteractive, &status); if (!service) @@ -1566,14 +1574,18 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv) HANDLE thread = CreateThread (NULL, 0, RunOpenvpn, pipe, CREATE_SUSPENDED, NULL); if (thread) { - error = AddListItem (&threads, thread) || - UpdateWaitHandles (&handles, &handle_count, io_event, exit_event, threads); + error = AddListItem (&threads, thread); + if (!error) + error = UpdateWaitHandles (&handles, &handle_count, io_event, exit_event, threads); if (error) { + ReturnError (pipe, error, L"Insufficient resources to service new clients", 1, &exit_event); + /* Update wait handles again after removing the last worker thread */ + RemoveListItem (&threads, CmpHandle, thread); + UpdateWaitHandles (&handles, &handle_count, io_event, exit_event, threads); TerminateThread (thread, 1); CloseHandleEx (&thread); CloseHandleEx (&pipe); - SetEvent (exit_event); } else ResumeThread (thread); @@ -1590,7 +1602,10 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv) if (error == WAIT_FAILED) { MsgToEventLog (M_SYSERR, TEXT("WaitForMultipleObjects failed")); - continue; + SetEvent (exit_event); + /* Give some time for worker threads to exit and then terminate */ + Sleep (1000); + break; } if (!threads) { @@ -1602,7 +1617,6 @@ ServiceStartInteractive (DWORD dwArgc, LPTSTR *lpszArgv) } /* Worker thread ended */ - BOOL CmpHandle (LPVOID item, LPVOID hnd) { return item == hnd; } HANDLE thread = RemoveListItem (&threads, CmpHandle, handles[error]); UpdateWaitHandles (&handles, &handle_count, io_event, exit_event, threads); CloseHandleEx (&thread); |