From 5ab2a48ba0d40a92de4facbf8467190eaee221e9 Mon Sep 17 00:00:00 2001
From: Selva Nair <selva.nair@gmail.com>
Date: Sun, 30 Jun 2019 11:42:32 -0500
Subject: [PATCH] Promptly close pipe handles passed to child

Parent keeping the handle to write end of child's stdout will
cause ERROR_BROKEN_PIPE not signalled if/when the child exits.

Also add a wrapper for CloseHandle()

Fixes the GUI process hanging in read from child
if the latter unexpectedly dies due to some error.
Trac #1203

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 misc.c    | 10 ++++++++++
 misc.h    |  2 ++
 openvpn.c | 27 +++++++++++++++++----------
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/misc.c b/misc.c
index fad1a48..df814bb 100644
--- a/misc.c
+++ b/misc.c
@@ -499,3 +499,13 @@ wcs_concat2(WCHAR *dest, int len, const WCHAR *src1, const WCHAR *src2, const WC
         n = 0;
     dest[n] = L'\0';
 }
+
+void
+CloseHandleEx(LPHANDLE handle)
+{
+    if (handle && *handle && *handle != INVALID_HANDLE_VALUE)
+    {
+        CloseHandle(*handle);
+        *handle = INVALID_HANDLE_VALUE;
+    }
+}
diff --git a/misc.h b/misc.h
index 81723eb..d56b087 100644
--- a/misc.h
+++ b/misc.h
@@ -44,5 +44,7 @@ BOOL validate_input(const WCHAR *input, const WCHAR *exclude);
 /* Concatenate two wide strings with a separator */
 void wcs_concat2(WCHAR *dest, int len, const WCHAR *src1, const WCHAR *src2, const WCHAR *sep);
 void CloseSemaphore(HANDLE sem);
+/* Close a handle if not null or invalid */
+void CloseHandleEx(LPHANDLE h);
 
 #endif
diff --git a/openvpn.c b/openvpn.c
index 4582441..9b231ab 100644
--- a/openvpn.c
+++ b/openvpn.c
@@ -2022,6 +2022,9 @@ StartOpenVPN(connection_t *c)
             goto out;
         }
 
+        CloseHandleEx(&hStdInRead);
+        CloseHandleEx(&hNul);
+
         /* Pass management password to OpenVPN process */
         c->manage.password[sizeof(c->manage.password) - 1] = '\n';
         WriteFile(hStdInWrite, c->manage.password, sizeof(c->manage.password), &written, NULL);
@@ -2159,8 +2162,8 @@ ReadLineFromStdOut(HANDLE hStdOut, char *line, DWORD size)
 BOOL
 CheckVersion()
 {
-    HANDLE hStdOutRead;
-    HANDLE hStdOutWrite;
+    HANDLE hStdOutRead = NULL;
+    HANDLE hStdOutWrite = NULL;
     BOOL retval = FALSE;
     STARTUPINFO si;
     PROCESS_INFORMATION pi;
@@ -2217,19 +2220,23 @@ CheckVersion()
     si.hStdError = hStdOutWrite;
 
     /* Start OpenVPN to check version */
-    if (!CreateProcess(o.exe_path, cmdline, NULL, NULL, TRUE,
-                       CREATE_NO_WINDOW, NULL, pwd, &si, &pi))
+    bool success = CreateProcess(o.exe_path, cmdline, NULL, NULL, TRUE,
+                       CREATE_NO_WINDOW, NULL, pwd, &si, &pi);
+    if (!success)
     {
         ShowLocalizedMsg(IDS_ERR_CREATE_PROCESS, o.exe_path, cmdline, pwd);
+        goto out;
     }
-    else if (ReadLineFromStdOut(hStdOutRead, line, sizeof(line)))
+
+    CloseHandleEx(&hStdOutWrite);
+    CloseHandleEx(&pi.hThread);
+    CloseHandleEx(&pi.hProcess);
+
+    if (ReadLineFromStdOut(hStdOutRead, line, sizeof(line)))
     {
 #ifdef DEBUG
         PrintDebug(_T("VersionString: %S"), line);
 #endif
-        CloseHandle(pi.hThread);
-        CloseHandle(pi.hProcess);
-
         /* OpenVPN version 2.x */
         char *p = strstr(line, match_version);
         if (p)
@@ -2242,8 +2249,8 @@ CheckVersion()
     }
 
 out:
-    CloseHandle(hStdOutRead);
-    CloseHandle(hStdOutWrite);
+    CloseHandleEx(&hStdOutWrite);
+    CloseHandleEx(&hStdOutRead);
     return retval;
 }