diff mbox series

[Openvpn-devel,v3] Fix console prompts with redirected log

Message ID 20210625010405.224-1-lstipakov@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,v3] Fix console prompts with redirected log | expand

Commit Message

Lev Stipakov June 24, 2021, 3:04 p.m. UTC 
From: Lev Stipakov <lev@openvpn.net>

When openvpn nees to prompt user for a password
(for example, to set management interface password),
the prompt is written to standard error device.

When log is redirected to a file, that prompt is written
to that file and not to the "original" stderr. Moreover, on recent
Insider build (21390.2025) openvpn exits with fatal error

  get_console_input_win32(): unexpected error: No such device or address
  (errno=6)

while attempting to write that prompt.

When redirecting stdout/stderr, we use _dup2() to associate stderr
descriptor with a log file. This call closes file associated
with stderr descriptor, which might explain why it has stopped
working (original stderr is closed and WriteFile() fails) and on
current versions it appears to work "by accident" - not failing
but use redirected stderr instead of original one.

Fix by creating new file descriptor with _dup() for stderr
before redirect and use this descriptor for writing prompts.

While on it, make code a bit more C99-ish by moving variables
declaration from the beginning of the scope to the actual
initialisation.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 v3: rebase

 v2: actually fix the prompt by displaying it in console
instead of writing to log

 src/openvpn/console_builtin.c | 18 +++++++-----------
 src/openvpn/error.c           | 28 +++++++---------------------
 src/openvpn/error.h           |  4 ++--
 3 files changed, 16 insertions(+), 34 deletions(-)

Comments

Gert Doering June 25, 2021, 6:02 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

Thanks for that.  Stared-at-code (looks good and much more "I understand
how this works" than before :-) ).  Test built on ubuntu/mingw, but not
actually tested the resulting binary - but I did test the openvpn.exe
that Lev built with MSVC for amd64, and it is working nicely for all
tested cases ("openvpn.exe --auth-user-pass --log file.txt --config ..."
actually prompts now, which it didn't before - GUI and CLI tested,
with and without auth-user-pass and passphrase prompts)

Your patch has been applied to the master and release/2.5 branch (bugfix).

commit 480e4cc14ff34fac72406ab1dd66290a91cc09f0 (master)
commit 86b247a40b919fa8e0a3de1c367087355bb10074 (release/2.5)
Author: Lev Stipakov
Date:   Fri Jun 25 04:04:05 2021 +0300

     Fix console prompts with redirected log

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20210625010405.224-1-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/search?l=mid&q=20210625010405.224-1-lstipakov@gmail.com
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/console_builtin.c b/src/openvpn/console_builtin.c
index 9bf36347..06b8a77a 100644
--- a/src/openvpn/console_builtin.c
+++ b/src/openvpn/console_builtin.c
@@ -62,23 +62,17 @@ 
 static bool
 get_console_input_win32(const char *prompt, const bool echo, char *input, const int capacity)
 {
-    HANDLE in = INVALID_HANDLE_VALUE;
-    HANDLE err = INVALID_HANDLE_VALUE;
-    DWORD len = 0;
-
     ASSERT(prompt);
     ASSERT(input);
     ASSERT(capacity > 0);
 
     input[0] = '\0';
 
-    in = GetStdHandle(STD_INPUT_HANDLE);
-    err = get_orig_stderr();
-
-    if (in == INVALID_HANDLE_VALUE
-        || err == INVALID_HANDLE_VALUE
+    HANDLE in = GetStdHandle(STD_INPUT_HANDLE);
+    int orig_stderr = get_orig_stderr(); // guaranteed to be always valid
+    if ((in == INVALID_HANDLE_VALUE)
         || win32_service_interrupt(&win32_signal)
-        || !WriteFile(err, prompt, strlen(prompt), &len, NULL))
+        || (_write(orig_stderr, prompt, strlen(prompt)) == -1))
     {
         msg(M_WARN|M_ERRNO, "get_console_input_win32(): unexpected error");
         return false;
@@ -106,6 +100,8 @@  get_console_input_win32(const char *prompt, const bool echo, char *input, const
         }
     }
 
+    DWORD len = 0;
+
     if (is_console)
     {
         winput = malloc(capacity * sizeof(WCHAR));
@@ -128,7 +124,7 @@  get_console_input_win32(const char *prompt, const bool echo, char *input, const
 
     if (!echo)
     {
-        WriteFile(err, "\r\n", 2, &len, NULL);
+        _write(orig_stderr, "\r\n", 2);
     }
     if (is_console)
     {
diff --git a/src/openvpn/error.c b/src/openvpn/error.c
index b94d387c..eb82f9c7 100644
--- a/src/openvpn/error.c
+++ b/src/openvpn/error.c
@@ -491,22 +491,12 @@  close_syslog(void)
 }
 
 #ifdef _WIN32
+static int orig_stderr;
 
-static HANDLE orig_stderr;
-
-HANDLE
-get_orig_stderr(void)
+int get_orig_stderr()
 {
-    if (orig_stderr)
-    {
-        return orig_stderr;
-    }
-    else
-    {
-        return GetStdHandle(STD_ERROR_HANDLE);
-    }
+    return orig_stderr ? orig_stderr : _fileno(stderr);
 }
-
 #endif
 
 void
@@ -550,16 +540,12 @@  redirect_stdout_stderr(const char *file, bool append)
         }
 
         /* save original stderr for password prompts */
-        orig_stderr = GetStdHandle(STD_ERROR_HANDLE);
-
-#if 0 /* seems not be necessary with stdout/stderr redirection below*/
-        /* set up for redirection */
-        if (!SetStdHandle(STD_OUTPUT_HANDLE, log_handle)
-            || !SetStdHandle(STD_ERROR_HANDLE, log_handle))
+        orig_stderr = _dup(_fileno(stderr));
+        if (orig_stderr == -1)
         {
-            msg(M_ERR, "Error: cannot redirect stdout/stderr to --log file: %s", file);
+            msg(M_WARN | M_ERRNO, "Warning: cannot duplicate stderr, password prompts will appear in log file instead of console.");
+            orig_stderr = _fileno(stderr);
         }
-#endif
 
         /* direct stdout/stderr to point to log_handle */
         log_fd = _open_osfhandle((intptr_t)log_handle, _O_TEXT);
diff --git a/src/openvpn/error.h b/src/openvpn/error.h
index f4528ef2..533354b3 100644
--- a/src/openvpn/error.h
+++ b/src/openvpn/error.h
@@ -256,8 +256,8 @@  void close_syslog(void);
 void redirect_stdout_stderr(const char *file, bool append);
 
 #ifdef _WIN32
-/* get original stderr handle, even if redirected by --log/--log-append */
-HANDLE get_orig_stderr(void);
+/* get original stderr fd, even if redirected by --log/--log-append */
+int get_orig_stderr(void);
 
 #endif