[Openvpn-devel,v3] Introduce openvpn_swprintf() with nul termination guarantee

Message ID 1540203571-17646-1-git-send-email-lstipakov@gmail.com
State Accepted, archived
Headers show
Series
  • [Openvpn-devel,v3] Introduce openvpn_swprintf() with nul termination guarantee
Related show

Commit Message

Lev Stipakov Oct. 22, 2018, 10:19 a.m.
From: Lev Stipakov <lev@openvpn.net>

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 <lev@openvpn.net>
---
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(-)

Comments

Selva Nair Oct. 22, 2018, 4:41 p.m. | #1
Hi,

On Mon, Oct 22, 2018 at 6:22 AM Lev Stipakov <lstipakov@gmail.com> wrote:
>
> From: Lev Stipakov <lev@openvpn.net>
>
> 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 <lev@openvpn.net>
> ---
> 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(-)
>
> diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
> index 80fbb75..8ca189f 100644
> --- a/src/openvpn/buffer.c
> +++ b/src/openvpn/buffer.c
> @@ -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
> diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h
> index aab249f..c5b78a0 100644
> --- a/src/openvpn/buffer.h
> +++ b/src/openvpn/buffer.h
> @@ -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
>   */
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 948fd17..c091401 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -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;
> diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
> index 33a8097..fd51776 100644
> --- a/src/openvpnserv/common.c
> +++ b/src/openvpnserv/common.c
> @@ -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)
>  {
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index 4a571f5..2dc152c 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -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);
> diff --git a/src/openvpnserv/service.h b/src/openvpnserv/service.h
> index d7b7b3f..cb7020f 100644
> --- a/src/openvpnserv/service.h
> +++ b/src/openvpnserv/service.h
> @@ -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);
> diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c
> index c576ac1..5506e90 100644
> --- a/src/openvpnserv/validate.c
> +++ b/src/openvpnserv/validate.c
> @@ -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 <selva.nair@gmail.com>
Gert Doering Oct. 22, 2018, 6:37 p.m. | #2
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 <lev@openvpn.net>
     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <1540203571-17646-1-git-send-email-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17786.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Selva Nair Oct. 23, 2018, 2:10 p.m. | #3
On Mon, Oct 22, 2018 at 2:38 PM Gert Doering <gert@greenie.muc.de> wrote:
>
> 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

Patch

diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
index 80fbb75..8ca189f 100644
--- a/src/openvpn/buffer.c
+++ b/src/openvpn/buffer.c
@@ -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
diff --git a/src/openvpn/buffer.h b/src/openvpn/buffer.h
index aab249f..c5b78a0 100644
--- a/src/openvpn/buffer.h
+++ b/src/openvpn/buffer.h
@@ -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
  */
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 948fd17..c091401 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -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;
diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
index 33a8097..fd51776 100644
--- a/src/openvpnserv/common.c
+++ b/src/openvpnserv/common.c
@@ -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)
 {
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 4a571f5..2dc152c 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -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);
diff --git a/src/openvpnserv/service.h b/src/openvpnserv/service.h
index d7b7b3f..cb7020f 100644
--- a/src/openvpnserv/service.h
+++ b/src/openvpnserv/service.h
@@ -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);
diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c
index c576ac1..5506e90 100644
--- a/src/openvpnserv/validate.c
+++ b/src/openvpnserv/validate.c
@@ -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