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

Message ID 1540117186-10445-1-git-send-email-lstipakov@gmail.com
State Superseded
Headers show
Series
  • [Openvpn-devel,v2] Introduce openvpn_swprintf() with nul termination guarantee
Related show

Commit Message

Lev Stipakov Oct. 21, 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>
---
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(-)

Comments

Selva Nair Oct. 21, 2018, 4:31 p.m. | #1
Hi,

On Sun, Oct 21, 2018 at 6:24 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>
> ---
> 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:

>
>  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

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..b065c39 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,
+            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);
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