Introduce openvpn_swprintf() with nul termination guarantee

PATCH Superseded Lev Stipakov 5 participants 14 messages Oct 4, 2018 – Oct 24, 2018

lore

Share & tools

lore

Lev Stipakov

author

2018-10-04 11:34 UTC #0 lore raw

PATCH 1/1 Introduce openvpn_swprintf() with nul termination guarantee

From: Lev Stipakov <[email protected]...>

Every call to swprintf is followed by line which adds nul terminator. This patch introduces openvpn_swprintf() which guarantees nul termination for size > 0.

Same approach as for snprintf / openvpn_snprintf.

Signed-off-by Lev Stipakov <[email protected]...>

---
 src/openvpn/buffer.c          | 17 +++++++++++++++++
 src/openvpn/buffer.h          | 11 +++++++++++
 src/openvpn/tun.c             |  3 +--
 src/openvpnserv/common.c      | 15 +++++++++++++++
 src/openvpnserv/interactive.c | 18 ++++++------------
 src/openvpnserv/validate.c    |  3 +--
 6 files changed, 51 insertions(+), 16 deletions(-)

src/openvpn/buffer.c +17 -0

index 7630ff7..52747cd 100644
@@ -37,6 +37,8 @@
 
 #include "memdbg.h"
 
+#include <wchar.h>
+
 size_t
 array_mult_safe(const size_t m1, const size_t m2, const size_t extra)
 {

@@ -308,6 +310,21 @@ openvpn_snprintf(char *str, size_t size, const char *format, ...)
     return (len >= 0 && len < size);
 }
 
+bool
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)
+{
+    va_list arglist;
+    int len = -1;
+    if (size > 0)
+    {
+        va_start(arglist, format);
+        len = vswprintf(str, size, format, arglist);
+        va_end(arglist);
+        str[size - 1] = L'\0';
+    }
+    return (len >= 0 && len < size);
+}
+
 /*
  * write a string to the end of a buffer that was
  * truncated by buf_printf

src/openvpn/buffer.h +11 -0

index cb9e9df..b1ee46d 100644
@@ -448,6 +448,17 @@ __attribute__ ((format(__printf__, 3, 4)))
 #endif
 ;
 
+
+/*
+ * Like swprintf but guarantees null termination for size > 0
+ */
+bool
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...);
+/*
+ * Unlike in openvpn_snprintf, we cannot use format attributes since
+ * GCC doesn't support wprintf as archetype.
+ */
+
 /*
  * remove/add trailing characters
  */

src/openvpn/tun.c +1 -2

index 50f158c..c5ef37a 100644
@@ -4480,8 +4480,7 @@ get_adapter_index_method_1(const char *guid)
     DWORD index;
     ULONG aindex;
     wchar_t wbuf[256];
-    swprintf(wbuf, SIZE(wbuf), L"\\DEVICE\\TCPIP_%S", guid);
-    wbuf [SIZE(wbuf) - 1] = 0;
+    openvpn_swprintf(wbuf, SIZE(wbuf), L"\\DEVICE\\TCPIP_%S", guid);
     if (GetAdapterIndex(wbuf, &aindex) != NO_ERROR)
     {
         index = TUN_ADAPTER_INDEX_INVALID;

src/openvpnserv/common.c +15 -0

index dc47666..b47046f 100644
@@ -56,6 +56,21 @@ openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...)
     return len;
 }
 
+BOOL
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)
+{
+    va_list arglist;
+    int len = -1;
+    if (size > 0)
+    {
+        va_start(arglist, format);
+        len = vswprintf(str, size, format, arglist);
+        va_end(arglist);
+        str[size - 1] = L'\0';
+    }
+    return (len >= 0 && len < size);
+}
+
 static DWORD
 GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size, LPCTSTR default_value)
 {

src/openvpnserv/interactive.c +6 ~~-12~~

index 9d459a6..bb2c411 100644
@@ -276,8 +276,7 @@ ReturnProcessId(HANDLE pipe, DWORD pid, DWORD count, LPHANDLE events)
      * Same format as error messages (3 line string) with error = 0 in
      * 0x%08x format, PID on line 2 and a description "Process ID" on line 3
      */
-    swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg);
-    buf[_countof(buf) - 1] = '\0';
+    openvpn_swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg);
 
     WritePipeAsync(pipe, buf, (DWORD)(wcslen(buf) * 2), count, events);
 }

@@ -402,9 +401,8 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
 
         if (!CheckOption(workdir, 2, argv_tmp, &settings))
         {
-            swprintf(buf, _countof(buf), msg1, argv[0], workdir,
+            openvpn_swprintf(buf, _countof(buf), msg1, argv[0], workdir,
                      settings.ovpn_admin_group);
-            buf[_countof(buf) - 1] = L'\0';
             ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
         }
         goto out;

@@ -421,16 +419,14 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
         {
             if (wcscmp(L"--config", argv[i]) == 0 && argc-i > 1)
             {
-                swprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
+                openvpn_swprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
                          settings.ovpn_admin_group);
-                buf[_countof(buf) - 1] = L'\0';
                 ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
             }
             else
             {
-                swprintf(buf, _countof(buf), msg2, argv[i],
+                openvpn_swprintf(buf, _countof(buf), msg2, argv[i],
                          settings.ovpn_admin_group);
-                buf[_countof(buf) - 1] = L'\0';
                 ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
             }
             goto out;

@@ -954,8 +950,7 @@ RegisterDNS(LPVOID unused)
 
     if (GetSystemDirectory(sys_path, MAX_PATH))
     {
-        swprintf(ipcfg, MAX_PATH, L"%s\\%s", sys_path, L"ipconfig.exe");
-        ipcfg[MAX_PATH-1] = L'\0';
+        openvpn_swprintf(ipcfg, MAX_PATH, L"%s\\%s", sys_path, L"ipconfig.exe");
     }
 
     if (WaitForMultipleObjects(2, wait_handles, FALSE, timeout) == WAIT_OBJECT_0)

@@ -1594,9 +1589,8 @@ RunOpenvpn(LPVOID p)
     else if (exit_code != 0)
     {
         WCHAR buf[256];
-        swprintf(buf, _countof(buf),
+        openvpn_swprintf(buf, _countof(buf),
                  L"OpenVPN exited with error: exit code = %lu", exit_code);
-        buf[_countof(buf) - 1] =  L'\0';
         ReturnError(pipe, ERROR_OPENVPN_STARTUP, buf, 1, &exit_event);
     }
     Undo(&undo_lists);

src/openvpnserv/validate.c +1 -2

index 91c6a2b..8d1855d 100644
@@ -68,8 +68,7 @@ CheckConfigPath(const WCHAR *workdir, const WCHAR *fname, const settings_t *s)
     /* convert fname to full path */
     if (PathIsRelativeW(fname) )
     {
-        swprintf(tmp, _countof(tmp), L"%s\\%s", workdir, fname);
-        tmp[_countof(tmp)-1] = L'\0';
+        openvpn_swprintf(tmp, _countof(tmp), L"%s\\%s", workdir, fname);
         config_file = tmp;
     }
     else

Signature · 1 line

2.7.4

Selva Nair

2018-10-18 21:31 UTC #1 lore raw

Hi,

Quoted 7 lines · Lev Stipakov

From: Lev Stipakov <[email protected]...>
Every call to swprintf is followed by line which adds nul terminator. This patch introduces openvpn_swprintf() which guarantees nul termination for size > 0.
Same approach as for snprintf / openvpn_snprintf.

Looks good and useful especially for the interactive service code. swprintf is rarely used in OpenVPN core (where strings are kept utf8 as far as possible) but having the wrapper is good there too.

Some opinionated comments below

Quoted 25 lines

Signed-off-by Lev Stipakov <[email protected]...>

---
 src/openvpn/buffer.c          | 17 +++++++++++++++++
 src/openvpn/buffer.h          | 11 +++++++++++
 src/openvpn/tun.c             |  3 +--
 src/openvpnserv/common.c      | 15 +++++++++++++++
 src/openvpnserv/interactive.c | 18 ++++++------------
 src/openvpnserv/validate.c    |  3 +--
 6 files changed, 51 insertions(+), 16 deletions(-)

src/openvpn/buffer.c +2 -0

index 7630ff7..52747cd 100644
@@ -37,6 +37,8 @@
 
 #include "memdbg.h"
 
+#include <wchar.h>
+
 size_t
 array_mult_safe(const size_t m1, const size_t m2, const size_t extra)
 {

src/openvpn/buffer.c openvpn_snprintf(char *str, size_t size, const char *format, ...) · line 314

Quoted 5 lines

src/openvpn/buffer.c +2 -0

@@ -308,6 +310,21 @@ openvpn_snprintf(char *str, size_t size, const char *format, ...)
     return (len >= 0 && len < size);
 }
 
+bool
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)

These const after *, and const on size serves little purpose and, IMO, makes the code
hard to read. One has to mentally delete all but one to ensure that the important one
(const whar_t *format) is in place. Same in common.c

But it may be just me as I see such usages all over the code. And I recall feeling
compelled to follow that style sometimes for consistency.. So, just saying :)

Quoted 148 lines

src/openvpn/buffer.c +13 -0

+{
+    va_list arglist;
+    int len = -1;
+    if (size > 0)
+    {
+        va_start(arglist, format);
+        len = vswprintf(str, size, format, arglist);
+        va_end(arglist);
+        str[size - 1] = L'\0';
+    }
+    return (len >= 0 && len < size);
+}
+
 /*
  * write a string to the end of a buffer that was
  * truncated by buf_printf

src/openvpn/buffer.h +11 -0

index cb9e9df..b1ee46d 100644
@@ -448,6 +448,17 @@ __attribute__ ((format(__printf__, 3, 4)))
 #endif
 ;
 
+
+/*
+ * Like swprintf but guarantees null termination for size > 0
+ */
+bool
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...);
+/*
+ * Unlike in openvpn_snprintf, we cannot use format attributes since
+ * GCC doesn't support wprintf as archetype.
+ */
+
 /*
  * remove/add trailing characters
  */

src/openvpn/tun.c +1 -2

index 50f158c..c5ef37a 100644
@@ -4480,8 +4480,7 @@ get_adapter_index_method_1(const char *guid)
     DWORD index;
     ULONG aindex;
     wchar_t wbuf[256];
-    swprintf(wbuf, SIZE(wbuf), L"\\DEVICE\\TCPIP_%S", guid);
-    wbuf [SIZE(wbuf) - 1] = 0;
+    openvpn_swprintf(wbuf, SIZE(wbuf), L"\\DEVICE\\TCPIP_%S", guid);
     if (GetAdapterIndex(wbuf, &aindex) != NO_ERROR)
     {
         index = TUN_ADAPTER_INDEX_INVALID;

src/openvpnserv/common.c +15 -0

index dc47666..b47046f 100644
@@ -56,6 +56,21 @@ openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...)
     return len;
 }
 
+BOOL
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)
+{
+    va_list arglist;
+    int len = -1;
+    if (size > 0)
+    {
+        va_start(arglist, format);
+        len = vswprintf(str, size, format, arglist);
+        va_end(arglist);
+        str[size - 1] = L'\0';
+    }
+    return (len >= 0 && len < size);
+}
+
 static DWORD
 GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size, LPCTSTR default_value)
 {

src/openvpnserv/interactive.c +6 ~~-12~~

index 9d459a6..bb2c411 100644
@@ -276,8 +276,7 @@ ReturnProcessId(HANDLE pipe, DWORD pid, DWORD count, LPHANDLE events)
      * Same format as error messages (3 line string) with error = 0 in
      * 0x%08x format, PID on line 2 and a description "Process ID" on line 3
      */
-    swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg);
-    buf[_countof(buf) - 1] = '\0';
+    openvpn_swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg);
 
     WritePipeAsync(pipe, buf, (DWORD)(wcslen(buf) * 2), count, events);
 }

@@ -402,9 +401,8 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
 
         if (!CheckOption(workdir, 2, argv_tmp, &settings))
         {
-            swprintf(buf, _countof(buf), msg1, argv[0], workdir,
+            openvpn_swprintf(buf, _countof(buf), msg1, argv[0], workdir,
                      settings.ovpn_admin_group);
-            buf[_countof(buf) - 1] = L'\0';
             ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
         }
         goto out;

@@ -421,16 +419,14 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
         {
             if (wcscmp(L"--config", argv[i]) == 0 && argc-i > 1)
             {
-                swprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
+                openvpn_swprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
                          settings.ovpn_admin_group);
-                buf[_countof(buf) - 1] = L'\0';
                 ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
             }
             else
             {
-                swprintf(buf, _countof(buf), msg2, argv[i],
+                openvpn_swprintf(buf, _countof(buf), msg2, argv[i],
                          settings.ovpn_admin_group);
-                buf[_countof(buf) - 1] = L'\0';
                 ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
             }
             goto out;

@@ -954,8 +950,7 @@ RegisterDNS(LPVOID unused)
 
     if (GetSystemDirectory(sys_path, MAX_PATH))
     {
-        swprintf(ipcfg, MAX_PATH, L"%s\\%s", sys_path, L"ipconfig.exe");
-        ipcfg[MAX_PATH-1] = L'\0';
+        openvpn_swprintf(ipcfg, MAX_PATH, L"%s\\%s", sys_path, L"ipconfig.exe");
     }
 
     if (WaitForMultipleObjects(2, wait_handles, FALSE, timeout) == WAIT_OBJECT_0)

@@ -1594,9 +1589,8 @@ RunOpenvpn(LPVOID p)
     else if (exit_code != 0)
     {
         WCHAR buf[256];
-        swprintf(buf, _countof(buf),
+        openvpn_swprintf(buf, _countof(buf),
                  L"OpenVPN exited with error: exit code = %lu", exit_code);
-        buf[_countof(buf) - 1] =  L'\0';
         ReturnError(pipe, ERROR_OPENVPN_STARTUP, buf, 1, &exit_event);
     }
     Undo(&undo_lists);

src/openvpnserv/validate.c +0 -0

index 91c6a2b..8d1855d 100644

src/openvpnserv/validate.c CheckConfigPath(const WCHAR *workdir, const WCHAR *fname, const settings_t *s) · line 74

Quoted 9 lines

src/openvpnserv/validate.c +1 -2

@@ -68,8 +68,7 @@ CheckConfigPath(const WCHAR *workdir, const WCHAR *fname, const settings_t *s)
     /* convert fname to full path */
     if (PathIsRelativeW(fname) )
     {
-        swprintf(tmp, _countof(tmp), L"%s\\%s", workdir, fname);
-        tmp[_countof(tmp)-1] = L'\0';
+        openvpn_swprintf(tmp, _countof(tmp), L"%s\\%s", workdir, fname);
         config_file = tmp;
     }
     else

This wrapper was sorely missed in the service code.

Despite my moaning about the const overload, I want to ACK this. But the patch doesn't apply on top of master -- likely because of the commits that changed int to BOOL and refactored GetSystemDirectory in the service code.

Could you please rebase and send again?

Thanks

Lev Stipakov

author

2018-10-21 10:19 UTC #2 lore raw

PATCH v2 1/1 Introduce openvpn_swprintf() with nul termination guarantee

From: Lev Stipakov <[email protected]...>

Every call to swprintf is followed by line which adds nul terminator. This patch introduces openvpn_swprintf() which guarantees nul termination for size > 0.

Same approach as for snprintf / openvpn_snprintf.

Signed-off-by Lev Stipakov <[email protected]...>

---
v2:
- rebase on top of master

 src/openvpn/buffer.c          | 17 +++++++++++++++++
 src/openvpn/buffer.h          | 11 +++++++++++
 src/openvpn/tun.c             |  3 +--
 src/openvpnserv/common.c      | 15 +++++++++++++++
 src/openvpnserv/interactive.c | 18 ++++++------------
 src/openvpnserv/validate.c    |  3 +--
 6 files changed, 51 insertions(+), 16 deletions(-)

src/openvpn/buffer.c +17 -0

index 80fbb75..8ca189f 100644
@@ -37,6 +37,8 @@
 
 #include "memdbg.h"
 
+#include <wchar.h>
+
 size_t
 array_mult_safe(const size_t m1, const size_t m2, const size_t extra)
 {

@@ -308,6 +310,21 @@ openvpn_snprintf(char *str, size_t size, const char *format, ...)
     return (len >= 0 && len < size);
 }
 
+bool
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)
+{
+    va_list arglist;
+    int len = -1;
+    if (size > 0)
+    {
+        va_start(arglist, format);
+        len = vswprintf(str, size, format, arglist);
+        va_end(arglist);
+        str[size - 1] = L'\0';
+    }
+    return (len >= 0 && len < size);
+}
+
 /*
  * write a string to the end of a buffer that was
  * truncated by buf_printf

src/openvpn/buffer.h +11 -0

index aab249f..c5b78a0 100644
@@ -448,6 +448,17 @@ __attribute__ ((format(__printf__, 3, 4)))
 #endif
 ;
 
+
+/*
+ * Like swprintf but guarantees null termination for size > 0
+ */
+bool
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...);
+/*
+ * Unlike in openvpn_snprintf, we cannot use format attributes since
+ * GCC doesn't support wprintf as archetype.
+ */
+
 /*
  * remove/add trailing characters
  */

src/openvpn/tun.c +1 -2

index 948fd17..c091401 100644
@@ -4472,8 +4472,7 @@ get_adapter_index_method_1(const char *guid)
     DWORD index;
     ULONG aindex;
     wchar_t wbuf[256];
-    swprintf(wbuf, SIZE(wbuf), L"\\DEVICE\\TCPIP_%S", guid);
-    wbuf [SIZE(wbuf) - 1] = 0;
+    openvpn_swprintf(wbuf, SIZE(wbuf), L"\\DEVICE\\TCPIP_%S", guid);
     if (GetAdapterIndex(wbuf, &aindex) != NO_ERROR)
     {
         index = TUN_ADAPTER_INDEX_INVALID;

src/openvpnserv/common.c +15 -0

index 33a8097..fd51776 100644
@@ -57,6 +57,21 @@ openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...)
     return res;
 }
 
+BOOL
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)
+{
+    va_list arglist;
+    int len = -1;
+    if (size > 0)
+    {
+        va_start(arglist, format);
+        len = vswprintf(str, size, format, arglist);
+        va_end(arglist);
+        str[size - 1] = L'\0';
+    }
+    return (len >= 0 && len < size);
+}
+
 static DWORD
 GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size, LPCTSTR default_value)
 {

src/openvpnserv/interactive.c +6 ~~-12~~

index 4a571f5..b065c39 100644
@@ -276,8 +276,7 @@ ReturnProcessId(HANDLE pipe, DWORD pid, DWORD count, LPHANDLE events)
      * Same format as error messages (3 line string) with error = 0 in
      * 0x%08x format, PID on line 2 and a description "Process ID" on line 3
      */
-    swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg);
-    buf[_countof(buf) - 1] = '\0';
+    openvpn_swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg);
 
     WritePipeAsync(pipe, buf, (DWORD)(wcslen(buf) * 2), count, events);
 }

@@ -402,9 +401,8 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
 
         if (!CheckOption(workdir, 2, argv_tmp, &settings))
         {
-            swprintf(buf, _countof(buf), msg1, argv[0], workdir,
+            openvpn_swprintf(buf, _countof(buf), msg1, argv[0], workdir,
                      settings.ovpn_admin_group);
-            buf[_countof(buf) - 1] = L'\0';
             ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
         }
         goto out;

@@ -421,16 +419,14 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
         {
             if (wcscmp(L"--config", argv[i]) == 0 && argc-i > 1)
             {
-                swprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
+                openvpn_swprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
                          settings.ovpn_admin_group);
-                buf[_countof(buf) - 1] = L'\0';
                 ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
             }
             else
             {
-                swprintf(buf, _countof(buf), msg2, argv[i],
+                openvpn_swprintf(buf, _countof(buf), msg2, argv[i],
                          settings.ovpn_admin_group);
-                buf[_countof(buf) - 1] = L'\0';
                 ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
             }
             goto out;

@@ -950,8 +946,7 @@ RegisterDNS(LPVOID unused)
 
     HANDLE wait_handles[2] = {rdns_semaphore, exit_event};
 
-    swprintf(ipcfg, _countof(ipcfg), L"%s\\%s", get_win_sys_path(), L"ipconfig.exe");
-    ipcfg[_countof(ipcfg) - 1] = L'\0';
+    openvpn_swprintf(ipcfg, MAX_PATH, L"%s\\%s", get_win_sys_path(), L"ipconfig.exe");
 
     if (WaitForMultipleObjects(2, wait_handles, FALSE, timeout) == WAIT_OBJECT_0)
     {

@@ -1628,9 +1623,8 @@ RunOpenvpn(LPVOID p)
     else if (exit_code != 0)
     {
         WCHAR buf[256];
-        swprintf(buf, _countof(buf),
+        openvpn_swprintf(buf, _countof(buf),
                  L"OpenVPN exited with error: exit code = %lu", exit_code);
-        buf[_countof(buf) - 1] =  L'\0';
         ReturnError(pipe, ERROR_OPENVPN_STARTUP, buf, 1, &exit_event);
     }
     Undo(&undo_lists);

src/openvpnserv/validate.c +1 -2

index c576ac1..5506e90 100644
@@ -68,8 +68,7 @@ CheckConfigPath(const WCHAR *workdir, const WCHAR *fname, const settings_t *s)
     /* convert fname to full path */
     if (PathIsRelativeW(fname) )
     {
-        swprintf(tmp, _countof(tmp), L"%s\\%s", workdir, fname);
-        tmp[_countof(tmp)-1] = L'\0';
+        openvpn_swprintf(tmp, _countof(tmp), L"%s\\%s", workdir, fname);
         config_file = tmp;
     }
     else

Signature · 1 line

2.7.4

Selva Nair

2018-10-21 16:31 UTC #3 lore raw

Hi,

Quoted 6 lines · Lev Stipakov

From: Lev Stipakov <[email protected]...>
Every call to swprintf is followed by line which adds nul terminator. This patch introduces openvpn_swprintf() which guarantees nul termination for size > 0.

Quoted 6 lines

Same approach as for snprintf / openvpn_snprintf.
Signed-off-by Lev Stipakov <[email protected]...>
---
v2:
- rebase on top of master

Thanks. I like to build and do a quick run test except for very trivial patches.

And, building did expose a small issue:

Quoted 8 lines

 src/openvpn/buffer.c          | 17 +++++++++++++++++
 src/openvpn/buffer.h          | 11 +++++++++++
 src/openvpn/tun.c             |  3 +--
 src/openvpnserv/common.c      | 15 +++++++++++++++
 src/openvpnserv/interactive.c | 18 ++++++------------
 src/openvpnserv/validate.c    |  3 +--
 6 files changed, 51 insertions(+), 16 deletions(-)

openvpn_swprintf in common.c has to be declared in one of the headers (service.h?) too. Missing as of now.

Should have noticed in the last round...

Selva

Lev Stipakov

author

2018-10-22 10:19 UTC #4 lore raw

PATCH v3 1/1 Introduce openvpn_swprintf() with nul termination guarantee

From: Lev Stipakov <[email protected]...>

Every call to swprintf is followed by line which adds nul terminator. This patch introduces openvpn_swprintf() which guarantees nul termination for size > 0.

Same approach as for snprintf / openvpn_snprintf.

Signed-off-by Lev Stipakov <[email protected]...>

---
v3:
- add missing function declaration
- fix indentation

v2:
- rebase on top of master

 src/openvpn/buffer.c          | 17 +++++++++++++++++
 src/openvpn/buffer.h          | 11 +++++++++++
 src/openvpn/tun.c             |  3 +--
 src/openvpnserv/common.c      | 15 +++++++++++++++
 src/openvpnserv/interactive.c | 26 ++++++++++----------------
 src/openvpnserv/service.h     |  2 ++
 src/openvpnserv/validate.c    |  3 +--
 7 files changed, 57 insertions(+), 20 deletions(-)

src/openvpn/buffer.c +17 -0

index 80fbb75..8ca189f 100644
@@ -37,6 +37,8 @@
 
 #include "memdbg.h"
 
+#include <wchar.h>
+
 size_t
 array_mult_safe(const size_t m1, const size_t m2, const size_t extra)
 {

@@ -308,6 +310,21 @@ openvpn_snprintf(char *str, size_t size, const char *format, ...)
     return (len >= 0 && len < size);
 }
 
+bool
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)
+{
+    va_list arglist;
+    int len = -1;
+    if (size > 0)
+    {
+        va_start(arglist, format);
+        len = vswprintf(str, size, format, arglist);
+        va_end(arglist);
+        str[size - 1] = L'\0';
+    }
+    return (len >= 0 && len < size);
+}
+
 /*
  * write a string to the end of a buffer that was
  * truncated by buf_printf

src/openvpn/buffer.h +11 -0

index aab249f..c5b78a0 100644
@@ -448,6 +448,17 @@ __attribute__ ((format(__printf__, 3, 4)))
 #endif
 ;
 
+
+/*
+ * Like swprintf but guarantees null termination for size > 0
+ */
+bool
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...);
+/*
+ * Unlike in openvpn_snprintf, we cannot use format attributes since
+ * GCC doesn't support wprintf as archetype.
+ */
+
 /*
  * remove/add trailing characters
  */

src/openvpn/tun.c +1 -2

index 948fd17..c091401 100644
@@ -4472,8 +4472,7 @@ get_adapter_index_method_1(const char *guid)
     DWORD index;
     ULONG aindex;
     wchar_t wbuf[256];
-    swprintf(wbuf, SIZE(wbuf), L"\\DEVICE\\TCPIP_%S", guid);
-    wbuf [SIZE(wbuf) - 1] = 0;
+    openvpn_swprintf(wbuf, SIZE(wbuf), L"\\DEVICE\\TCPIP_%S", guid);
     if (GetAdapterIndex(wbuf, &aindex) != NO_ERROR)
     {
         index = TUN_ADAPTER_INDEX_INVALID;

src/openvpnserv/common.c +15 -0

index 33a8097..fd51776 100644
@@ -57,6 +57,21 @@ openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...)
     return res;
 }
 
+BOOL
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)
+{
+    va_list arglist;
+    int len = -1;
+    if (size > 0)
+    {
+        va_start(arglist, format);
+        len = vswprintf(str, size, format, arglist);
+        va_end(arglist);
+        str[size - 1] = L'\0';
+    }
+    return (len >= 0 && len < size);
+}
+
 static DWORD
 GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size, LPCTSTR default_value)
 {

src/openvpnserv/interactive.c +10 ~~-16~~

index 4a571f5..2dc152c 100644
@@ -276,8 +276,7 @@ ReturnProcessId(HANDLE pipe, DWORD pid, DWORD count, LPHANDLE events)
      * Same format as error messages (3 line string) with error = 0 in
      * 0x%08x format, PID on line 2 and a description "Process ID" on line 3
      */
-    swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg);
-    buf[_countof(buf) - 1] = '\0';
+    openvpn_swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg);
 
     WritePipeAsync(pipe, buf, (DWORD)(wcslen(buf) * 2), count, events);
 }

@@ -402,9 +401,8 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
 
         if (!CheckOption(workdir, 2, argv_tmp, &settings))
         {
-            swprintf(buf, _countof(buf), msg1, argv[0], workdir,
-                     settings.ovpn_admin_group);
-            buf[_countof(buf) - 1] = L'\0';
+            openvpn_swprintf(buf, _countof(buf), msg1, argv[0], workdir,
+                             settings.ovpn_admin_group);
             ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
         }
         goto out;

@@ -421,16 +419,14 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
         {
             if (wcscmp(L"--config", argv[i]) == 0 && argc-i > 1)
             {
-                swprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
-                         settings.ovpn_admin_group);
-                buf[_countof(buf) - 1] = L'\0';
+                openvpn_swprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
+                                 settings.ovpn_admin_group);
                 ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
             }
             else
             {
-                swprintf(buf, _countof(buf), msg2, argv[i],
-                         settings.ovpn_admin_group);
-                buf[_countof(buf) - 1] = L'\0';
+                openvpn_swprintf(buf, _countof(buf), msg2, argv[i],
+                                 settings.ovpn_admin_group);
                 ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
             }
             goto out;

@@ -950,8 +946,7 @@ RegisterDNS(LPVOID unused)
 
     HANDLE wait_handles[2] = {rdns_semaphore, exit_event};
 
-    swprintf(ipcfg, _countof(ipcfg), L"%s\\%s", get_win_sys_path(), L"ipconfig.exe");
-    ipcfg[_countof(ipcfg) - 1] = L'\0';
+    openvpn_swprintf(ipcfg, MAX_PATH, L"%s\\%s", get_win_sys_path(), L"ipconfig.exe");
 
     if (WaitForMultipleObjects(2, wait_handles, FALSE, timeout) == WAIT_OBJECT_0)
     {

@@ -1628,9 +1623,8 @@ RunOpenvpn(LPVOID p)
     else if (exit_code != 0)
     {
         WCHAR buf[256];
-        swprintf(buf, _countof(buf),
-                 L"OpenVPN exited with error: exit code = %lu", exit_code);
-        buf[_countof(buf) - 1] =  L'\0';
+        openvpn_swprintf(buf, _countof(buf),
+                         L"OpenVPN exited with error: exit code = %lu", exit_code);
         ReturnError(pipe, ERROR_OPENVPN_STARTUP, buf, 1, &exit_event);
     }
     Undo(&undo_lists);

src/openvpnserv/service.h +2 -0

index d7b7b3f..cb7020f 100644
@@ -86,6 +86,8 @@ BOOL openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist
 
 BOOL openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...);
 
+BOOL openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...);
+
 DWORD GetOpenvpnSettings(settings_t *s);
 
 BOOL ReportStatusToSCMgr(SERVICE_STATUS_HANDLE service, SERVICE_STATUS *status);

src/openvpnserv/validate.c +1 -2

index c576ac1..5506e90 100644
@@ -68,8 +68,7 @@ CheckConfigPath(const WCHAR *workdir, const WCHAR *fname, const settings_t *s)
     /* convert fname to full path */
     if (PathIsRelativeW(fname) )
     {
-        swprintf(tmp, _countof(tmp), L"%s\\%s", workdir, fname);
-        tmp[_countof(tmp)-1] = L'\0';
+        openvpn_swprintf(tmp, _countof(tmp), L"%s\\%s", workdir, fname);
         config_file = tmp;
     }
     else

Signature · 1 line

2.7.4

Selva Nair

Acked-by on v3 1/1

2018-10-22 16:41 UTC #5 lore raw

Hi,

Quoted 210 lines · Lev Stipakov

From: Lev Stipakov <[email protected]...>

Every call to swprintf is followed by line which adds nul terminator. This patch introduces openvpn_swprintf() which guarantees nul termination for size > 0.

Same approach as for snprintf / openvpn_snprintf.

Signed-off-by Lev Stipakov <[email protected]...>

---
v3:
- add missing function declaration
- fix indentation

v2:
- rebase on top of master

 src/openvpn/buffer.c          | 17 +++++++++++++++++
 src/openvpn/buffer.h          | 11 +++++++++++
 src/openvpn/tun.c             |  3 +--
 src/openvpnserv/common.c      | 15 +++++++++++++++
 src/openvpnserv/interactive.c | 26 ++++++++++----------------
 src/openvpnserv/service.h     |  2 ++
 src/openvpnserv/validate.c    |  3 +--
 7 files changed, 57 insertions(+), 20 deletions(-)

src/openvpn/buffer.c +17 -0

index 80fbb75..8ca189f 100644
@@ -37,6 +37,8 @@
 
 #include "memdbg.h"
 
+#include <wchar.h>
+
 size_t
 array_mult_safe(const size_t m1, const size_t m2, const size_t extra)
 {

@@ -308,6 +310,21 @@ openvpn_snprintf(char *str, size_t size, const char *format, ...)
     return (len >= 0 && len < size);
 }
 
+bool
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)
+{
+    va_list arglist;
+    int len = -1;
+    if (size > 0)
+    {
+        va_start(arglist, format);
+        len = vswprintf(str, size, format, arglist);
+        va_end(arglist);
+        str[size - 1] = L'\0';
+    }
+    return (len >= 0 && len < size);
+}
+
 /*
  * write a string to the end of a buffer that was
  * truncated by buf_printf

src/openvpn/buffer.h +11 -0

index aab249f..c5b78a0 100644
@@ -448,6 +448,17 @@ __attribute__ ((format(__printf__, 3, 4)))
 #endif
 ;
 
+
+/*
+ * Like swprintf but guarantees null termination for size > 0
+ */
+bool
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...);
+/*
+ * Unlike in openvpn_snprintf, we cannot use format attributes since
+ * GCC doesn't support wprintf as archetype.
+ */
+
 /*
  * remove/add trailing characters
  */

src/openvpn/tun.c +1 -2

index 948fd17..c091401 100644
@@ -4472,8 +4472,7 @@ get_adapter_index_method_1(const char *guid)
     DWORD index;
     ULONG aindex;
     wchar_t wbuf[256];
-    swprintf(wbuf, SIZE(wbuf), L"\\DEVICE\\TCPIP_%S", guid);
-    wbuf [SIZE(wbuf) - 1] = 0;
+    openvpn_swprintf(wbuf, SIZE(wbuf), L"\\DEVICE\\TCPIP_%S", guid);
     if (GetAdapterIndex(wbuf, &aindex) != NO_ERROR)
     {
         index = TUN_ADAPTER_INDEX_INVALID;

src/openvpnserv/common.c +15 -0

index 33a8097..fd51776 100644
@@ -57,6 +57,21 @@ openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...)
     return res;
 }
 
+BOOL
+openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)
+{
+    va_list arglist;
+    int len = -1;
+    if (size > 0)
+    {
+        va_start(arglist, format);
+        len = vswprintf(str, size, format, arglist);
+        va_end(arglist);
+        str[size - 1] = L'\0';
+    }
+    return (len >= 0 && len < size);
+}
+
 static DWORD
 GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size, LPCTSTR default_value)
 {

src/openvpnserv/interactive.c +10 ~~-16~~

index 4a571f5..2dc152c 100644
@@ -276,8 +276,7 @@ ReturnProcessId(HANDLE pipe, DWORD pid, DWORD count, LPHANDLE events)
      * Same format as error messages (3 line string) with error = 0 in
      * 0x%08x format, PID on line 2 and a description "Process ID" on line 3
      */
-    swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg);
-    buf[_countof(buf) - 1] = '\0';
+    openvpn_swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg);
 
     WritePipeAsync(pipe, buf, (DWORD)(wcslen(buf) * 2), count, events);
 }

@@ -402,9 +401,8 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
 
         if (!CheckOption(workdir, 2, argv_tmp, &settings))
         {
-            swprintf(buf, _countof(buf), msg1, argv[0], workdir,
-                     settings.ovpn_admin_group);
-            buf[_countof(buf) - 1] = L'\0';
+            openvpn_swprintf(buf, _countof(buf), msg1, argv[0], workdir,
+                             settings.ovpn_admin_group);
             ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
         }
         goto out;

@@ -421,16 +419,14 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
         {
             if (wcscmp(L"--config", argv[i]) == 0 && argc-i > 1)
             {
-                swprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
-                         settings.ovpn_admin_group);
-                buf[_countof(buf) - 1] = L'\0';
+                openvpn_swprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
+                                 settings.ovpn_admin_group);
                 ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
             }
             else
             {
-                swprintf(buf, _countof(buf), msg2, argv[i],
-                         settings.ovpn_admin_group);
-                buf[_countof(buf) - 1] = L'\0';
+                openvpn_swprintf(buf, _countof(buf), msg2, argv[i],
+                                 settings.ovpn_admin_group);
                 ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
             }
             goto out;

@@ -950,8 +946,7 @@ RegisterDNS(LPVOID unused)
 
     HANDLE wait_handles[2] = {rdns_semaphore, exit_event};
 
-    swprintf(ipcfg, _countof(ipcfg), L"%s\\%s", get_win_sys_path(), L"ipconfig.exe");
-    ipcfg[_countof(ipcfg) - 1] = L'\0';
+    openvpn_swprintf(ipcfg, MAX_PATH, L"%s\\%s", get_win_sys_path(), L"ipconfig.exe");
 
     if (WaitForMultipleObjects(2, wait_handles, FALSE, timeout) == WAIT_OBJECT_0)
     {

@@ -1628,9 +1623,8 @@ RunOpenvpn(LPVOID p)
     else if (exit_code != 0)
     {
         WCHAR buf[256];
-        swprintf(buf, _countof(buf),
-                 L"OpenVPN exited with error: exit code = %lu", exit_code);
-        buf[_countof(buf) - 1] =  L'\0';
+        openvpn_swprintf(buf, _countof(buf),
+                         L"OpenVPN exited with error: exit code = %lu", exit_code);
         ReturnError(pipe, ERROR_OPENVPN_STARTUP, buf, 1, &exit_event);
     }
     Undo(&undo_lists);

src/openvpnserv/service.h +2 -0

index d7b7b3f..cb7020f 100644
@@ -86,6 +86,8 @@ BOOL openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist
 
 BOOL openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...);
 
+BOOL openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...);
+
 DWORD GetOpenvpnSettings(settings_t *s);
 
 BOOL ReportStatusToSCMgr(SERVICE_STATUS_HANDLE service, SERVICE_STATUS *status);

src/openvpnserv/validate.c +0 -0

index c576ac1..5506e90 100644

src/openvpnserv/validate.c CheckConfigPath(const WCHAR *workdir, const WCHAR *fname, const settings_t *s) · line 74

Quoted 9 lines

src/openvpnserv/validate.c +1 -2

@@ -68,8 +68,7 @@ CheckConfigPath(const WCHAR *workdir, const WCHAR *fname, const settings_t *s)
     /* convert fname to full path */
     if (PathIsRelativeW(fname) )
     {
-        swprintf(tmp, _countof(tmp), L"%s\\%s", workdir, fname);
-        tmp[_countof(tmp)-1] = L'\0';
+        openvpn_swprintf(tmp, _countof(tmp), L"%s\\%s", workdir, fname);
         config_file = tmp;
     }
     else

Acked-by Selva Nair <[email protected]...>

Gert Doering

2018-10-22 18:37 UTC #6 lore raw

In reply to Lev Stipakov

Your patch has been applied to the master branch.

I'm not sure I totally like the change to buffer.c/tun.c - for the single instance where this is called inside openvpn/tun.c, on WIN32, we now carry around a new function in buffer.c which is compiled everywhere... possibly breaking older systems that have C99 compliant C compilers but no full C99 library - like my oldest OpenBSD buildslave (our mantra on that is "if it breaks for good reasons, it will be retired, but we shouldn't carelessly break things").

So if we really want to keep this, it should be #ifdef _WIN32 (until we find good uses for wide-sprintf() on other platforms).

For the service, which is windows-only code, this change is clearly reasonable and beneficial.

commit 43a5a4f3b4e411419639c195fee8a76495fdc88e

Author: Lev Stipakov
Date:   Mon Oct 22 13:19:31 2018 +0300

     Introduce openvpn_swprintf() with nul termination guarantee

     Signed-off-by: Lev Stipakov <[email protected]...>
     Acked-by: Selva Nair <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17786.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Selva Nair

2018-10-23 14:10 UTC #7 lore raw

Quoted 10 lines · Gert Doering

Your patch has been applied to the master branch.
I'm not sure I totally like the change to buffer.c/tun.c - for the single instance where this is called inside openvpn/tun.c, on WIN32, we now carry around a new function in buffer.c which is compiled everywhere... possibly breaking older systems that have C99 compliant C compilers but no full C99 library - like my oldest OpenBSD buildslave (our mantra on that is "if it breaks for good reasons, it will be retired, but we shouldn't carelessly break things").

Good point. We should never need (v)swprintf on non-windows platforms as keeping strings in utf8 should be the preferred way.

Enclosing this inside #ifdef _WIN32 sounds good. My bad to be too much fixated on Windows during review..

Selva

Lev Stipakov

author

2018-10-23 14:51 UTC #8 lore raw

PATCH 1/1 Wrap openvpn_swprintf into Windows define

From: Lev Stipakov <[email protected]...>

Commit 43a5a4f3b4e411419639c195fee8a76495fdc88e added vswprintf() call which turned to me missing in OpenBSD 4.9.

Since that call is inside openvpn_swprintf() function which is only used by Windows, wrap that function info #ifdef _WIN32.

Signed-off-by Lev Stipakov <[email protected]...>

---
 src/openvpn/buffer.c | 3 +++
 src/openvpn/buffer.h | 5 +++++
 2 files changed, 8 insertions(+)

src/openvpn/buffer.c +3 -0

index 8ca189f..acf76ae 100644
@@ -310,6 +310,7 @@ openvpn_snprintf(char *str, size_t size, const char *format, ...)
     return (len >= 0 && len < size);
 }
 
+#ifdef _WIN32
 bool
 openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const format, ...)
 {

@@ -318,12 +319,14 @@ openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const for
     if (size > 0)
     {
         va_start(arglist, format);
+        /* vswprintf is missing in OpenBSD 4.2 */
         len = vswprintf(str, size, format, arglist);
         va_end(arglist);
         str[size - 1] = L'\0';
     }
     return (len >= 0 && len < size);
 }
+#endif
 
 /*
  * write a string to the end of a buffer that was

src/openvpn/buffer.h +5 -0

index c5b78a0..c48c925 100644
@@ -449,7 +449,11 @@ __attribute__ ((format(__printf__, 3, 4)))
 ;
 
 
+#ifdef _WIN32
 /*
+ * This is under #ifdef because only Windows-specific code
+ * uses this function and its implementation is broken in OpenBSD 4.9
+ *
  * Like swprintf but guarantees null termination for size > 0
  */
 bool

@@ -458,6 +462,7 @@ openvpn_swprintf(wchar_t *const str, const size_t size, const wchar_t *const for
  * Unlike in openvpn_snprintf, we cannot use format attributes since
  * GCC doesn't support wprintf as archetype.
  */
+#endif
 
 /*
  * remove/add trailing characters

Signature · 1 line

2.7.4

Antonio Quartulli

Acked-by on v1 1/1

2018-10-24 10:22 UTC #9 lore raw

Re: [Openvpn-devel] [PATCH] Wrap openvpn_swprintf into Windows define

Hi,

Quoted 9 lines · Lev Stipakov

From: Lev Stipakov <[email protected]...>
Commit 43a5a4f3b4e411419639c195fee8a76495fdc88e added vswprintf() call which turned to me missing in OpenBSD 4.9.
Since that call is inside openvpn_swprintf() function which is only used by Windows, wrap that function info #ifdef _WIN32.
Signed-off-by Lev Stipakov <[email protected]...>

This patch introduces another ifdef, but it is enclosing a whole function to prevent it from being compiled on non-windows platforms, therefore I consider it "acceptable" for the time being.

This patch fixes building openvpn on OpenBSD <= 4.9 (maybe this info should be added to the commit message).

Other than that:

Acked-by Antonio Quartulli <[email protected]...>

Signature · 1 line

Antonio Quartulli

Selva Nair

2018-10-24 12:39 UTC #10 lore raw

Re: [Openvpn-devel] [PATCH] Wrap openvpn_swprintf into Windows define

Quoted 12 lines · Antonio Quartulli

Hi,
Quoted 8 lines · Lev Stipakov
From: Lev Stipakov <[email protected]...>
Commit 43a5a4f3b4e411419639c195fee8a76495fdc88e added vswprintf() call which turned to me missing in OpenBSD 4.9.
Since that call is inside openvpn_swprintf() function which is only used by Windows, wrap that function info #ifdef _WIN32.

Quoted 8 lines

Quoted 1 line
Signed-off-by Lev Stipakov <[email protected]...>
This patch introduces another ifdef, but it is enclosing a whole function to prevent it from being compiled on non-windows platforms, therefore I consider it "acceptable" for the time being.
This patch fixes building openvpn on OpenBSD <= 4.9 (maybe this info should be added to the commit message).

Also there is a misplaced and mismatched comment added to buffer.c that says

+ /* vswprintf is missing in OpenBSD 4.2 */

         len = vswprintf(str, size, format, arglist);

Makes little sense in that context and why 4.2..
That one could be removed.

Otherwise an ACK from me too.

Selva

David Sommerseth

2018-10-24 13:00 UTC #11 lore raw

Re: [Openvpn-devel] [PATCH] Wrap openvpn_swprintf into Windows define

Quoted 22 lines · Selva Nair

Quoted 12 lines · Antonio Quartulli
Hi,
Quoted 8 lines · Lev Stipakov
From: Lev Stipakov <[email protected]...>
Commit 43a5a4f3b4e411419639c195fee8a76495fdc88e added vswprintf() call which turned to me missing in OpenBSD 4.9.
Since that call is inside openvpn_swprintf() function which is only used by Windows, wrap that function info #ifdef _WIN32.
Quoted 8 lines
Quoted 1 line
Signed-off-by Lev Stipakov <[email protected]...>
This patch introduces another ifdef, but it is enclosing a whole function to prevent it from being compiled on non-windows platforms, therefore I consider it "acceptable" for the time being.
This patch fixes building openvpn on OpenBSD <= 4.9 (maybe this info should be added to the commit message).

Quoted 8 lines

Also there is a misplaced and mismatched comment added to buffer.c that says
+ /* vswprintf is missing in OpenBSD 4.2 */
         len = vswprintf(str, size, format, arglist);
Makes little sense in that context and why 4.2..
That one could be removed.

I'd say we should have such a reference in right below the #ifdef _WIN32 a few lines higher up. And it could be slightly more informative (and should reference OpenBSD 4.9, which is the distro exploding without this patch.

Primary reason for this message is to explain why we do not provide this function to "everyone", if someone has a use case for it later on in a non-Windows code path. If this is needed elsewhere, we at least know instantly what might break and we can evaluate if it's worth keeping OpenBSD 4.9 support.

I suggest the comment below the #ifdef _WIN32 to be something like:

/*
* openvpn_swprintf() is currently only used by Windows code paths
* and when enabled for all platforms it will currently break
* OpenBSD 4.9 which lacks vswprintf(3) support in its libc. */

Signature · 4 lines

kind regards,

David Sommerseth
OpenVPN Inc

Selva Nair

2018-10-24 13:36 UTC #12 lore raw

Re: [Openvpn-devel] [PATCH] Wrap openvpn_swprintf into Windows define

Hi,

Quoted 11 lines · David Sommerseth

Quoted 9 lines · Selva Nair
Quoted 3 lines · Antonio Quartulli
Hi,
Also there is a misplaced and mismatched comment added to buffer.c that says

Quoted 8 lines

Quoted 5 lines
+ /* vswprintf is missing in OpenBSD 4.2 */
         len = vswprintf(str, size, format, arglist);
Makes little sense in that context and why 4.2..
That one could be removed.
I'd say we should have such a reference in right below the #ifdef _WIN32 a few lines higher up. And it could be slightly more informative (and should reference OpenBSD 4.9, which is the distro exploding without this patch.

Yes, that's why I wrote "misplaced" and "mismatched" :)

Quoted 7 lines

I suggest the comment below the #ifdef _WIN32 to be something like:
/*
* openvpn_swprintf() is currently only used by Windows code paths
* and when enabled for all platforms it will currently break
* OpenBSD 4.9 which lacks vswprintf(3) support in its libc. */

Looks good except, <=4.9, not just 4.9, or whatever version is appropriate. (As Antonio already pointed out.)

Selva

Gert Doering

2018-10-24 15:55 UTC #13 lore raw

[Openvpn-devel] [PATCH applied] Re: Wrap openvpn_swprintf into Windows define

In reply to Lev Stipakov

Your patch has been applied to the master branch.

Make OpenBSD great^w green again! (Tested before pushing :) )

(I have amended the comments according to the discussion)

commit 4ce1a9b6b3ae0be32a3d999c5bc93731c64d7bbb

Author: Lev Stipakov
Date:   Tue Oct 23 17:51:28 2018 +0300

     Wrap openvpn_swprintf into Windows define

     Signed-off-by: Lev Stipakov <[email protected]...>
     Acked-by: Antonio Quartulli <[email protected]...>
     Acked-by: Selva Nair <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17799.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering