diff mbox series

[Openvpn-devel] Correctly handle Unicode names for exit event

Message ID 20230516024232.2680491-1-selva.nair@gmail.com
State Accepted
Headers
Series [Openvpn-devel] Correctly handle Unicode names for exit event |

Commit Message

Selva Nair May 16, 2023, 2:42 a.m. UTC 
  From: Selva Nair <selva.nair@gmail.com>

Currently we use the ANSI version of CreateEvent causing name of the
exit event to be interpreted differently depending on the code page
in effect. Internally all strings parsed from command line and config
file are stored as UTF8-encoded Uniode. When passed to Windows API calls,
these should be converted to UTF16 and wide character version of the API
should be used.

CreateEvent calls for unnamed events are left unchanged as there is no
text-encoding dependence in those cases.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/win32.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Lev Stipakov May 16, 2023, 10:53 a.m. UTC | #1 
Hi,

My understanding is that this is related to
https://github.com/OpenVPN/openvpn-gui/issues/626#issuecomment-1546934297
since normally we use string representation of HANDLE value
as an event name.

Looks good to me. Also compiled and tested.

Acked-by: Lev Stipakov <lstipakov@gmail.com>

ti 16. toukok. 2023 klo 5.57 selva.nair@gmail.com kirjoitti:
>
> From: Selva Nair <selva.nair@gmail.com>
>
> Currently we use the ANSI version of CreateEvent causing name of the
> exit event to be interpreted differently depending on the code page
> in effect. Internally all strings parsed from command line and config
> file are stored as UTF8-encoded Uniode. When passed to Windows API calls,
> these should be converted to UTF16 and wide character version of the API
> should be used.
>
> CreateEvent calls for unnamed events are left unchanged as there is no
> text-encoding dependence in those cases.
>
> Signed-off-by: Selva Nair <selva.nair@gmail.com>
> ---
>  src/openvpn/win32.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
> index 1ae3723f..25da54ab 100644
> --- a/src/openvpn/win32.c
> +++ b/src/openvpn/win32.c
> @@ -509,19 +509,19 @@ win32_signal_open(struct win32_signal *ws,
>          && !HANDLE_DEFINED(ws->in.read) && exit_event_name)
>      {
>          struct security_attributes sa;
> +        struct gc_arena gc = gc_new();
> +        const wchar_t *exit_event_nameW = wide_string(exit_event_name, &gc);
>
>          if (!init_security_attributes_allow_all(&sa))
>          {
>              msg(M_ERR, "Error: win32_signal_open: init SA failed");
>          }
>
> -        ws->in.read = CreateEvent(&sa.sa,
> -                                  TRUE,
> -                                  exit_event_initial_state ? TRUE : FALSE,
> -                                  exit_event_name);
> +        ws->in.read = CreateEventW(&sa.sa, TRUE, exit_event_initial_state ? TRUE : FALSE,
> +                                   exit_event_nameW);
>          if (ws->in.read == NULL)
>          {
> -            msg(M_WARN|M_ERRNO, "NOTE: CreateEvent '%s' failed", exit_event_name);
> +            msg(M_WARN|M_ERRNO, "NOTE: CreateEventW '%s' failed", exit_event_name);
>          }
>          else
>          {
> @@ -534,6 +534,7 @@ win32_signal_open(struct win32_signal *ws,
>                  ws->mode = WSO_MODE_SERVICE;
>              }
>          }
> +        gc_free(&gc);
>      }
>      /* set the ctrl handler in both console and service modes */
>      if (!SetConsoleCtrlHandler((PHANDLER_ROUTINE) win_ctrl_handler, true))
> --
> 2.34.1
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Gert Doering May 16, 2023, 11:21 a.m. UTC | #2 
Explanation makes sense, though it's only relevant for "outside openvpn GUI"
use cases.  But I look forward to a submission of "openvpn windows CLI" :-)

Test compiled on MinGW.  Does our GH windows test setup use the event
mechanism?  Or "classic signals"?

Your patch has been applied to the master and release/2.6 branch.

commit 80b073b649fca54f5021f6b4ae45a1e74a07faea (master)
commit 232a0fac1ede6724688cbd4f5d5f2e619f647e16 (release/2.6)
Author: Selva Nair
Date:   Mon May 15 22:42:32 2023 -0400

     Correctly handle Unicode names for exit event

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Message-Id: <20230516024232.2680491-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26666.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Frank Lichtenheld May 16, 2023, 12:33 p.m. UTC | #3 
On Tue, May 16, 2023 at 01:21:34PM +0200, Gert Doering wrote:
> Explanation makes sense, though it's only relevant for "outside openvpn GUI"
> use cases.  But I look forward to a submission of "openvpn windows CLI" :-)
> 
> Test compiled on MinGW.  Does our GH windows test setup use the event
> mechanism?  Or "classic signals"?

Neither, it uses the GUI CLI :)
See https://github.com/OpenVPN/openvpn-windows-test/blob/master/Start-LocalTest.ps1#L342

Regards,
diff mbox series

Patch

diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index 1ae3723f..25da54ab 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -509,19 +509,19 @@  win32_signal_open(struct win32_signal *ws,
         && !HANDLE_DEFINED(ws->in.read) && exit_event_name)
     {
         struct security_attributes sa;
+        struct gc_arena gc = gc_new();
+        const wchar_t *exit_event_nameW = wide_string(exit_event_name, &gc);
 
         if (!init_security_attributes_allow_all(&sa))
         {
             msg(M_ERR, "Error: win32_signal_open: init SA failed");
         }
 
-        ws->in.read = CreateEvent(&sa.sa,
-                                  TRUE,
-                                  exit_event_initial_state ? TRUE : FALSE,
-                                  exit_event_name);
+        ws->in.read = CreateEventW(&sa.sa, TRUE, exit_event_initial_state ? TRUE : FALSE,
+                                   exit_event_nameW);
         if (ws->in.read == NULL)
         {
-            msg(M_WARN|M_ERRNO, "NOTE: CreateEvent '%s' failed", exit_event_name);
+            msg(M_WARN|M_ERRNO, "NOTE: CreateEventW '%s' failed", exit_event_name);
         }
         else
         {
@@ -534,6 +534,7 @@  win32_signal_open(struct win32_signal *ws,
                 ws->mode = WSO_MODE_SERVICE;
             }
         }
+        gc_free(&gc);
     }
     /* set the ctrl handler in both console and service modes */
     if (!SetConsoleCtrlHandler((PHANDLER_ROUTINE) win_ctrl_handler, true))