diff mbox series

[Openvpn-devel,1/3] Move get system directory to a separate function

Message ID 1538510474-27602-1-git-send-email-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,1/3] Move get system directory to a separate function | expand

Commit Message

Selva Nair Oct. 2, 2018, 10:01 a.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

Only refactoring to reduce code-duplication, no functional changes.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpnserv/common.c      | 16 +++++++++++++++-
 src/openvpnserv/interactive.c | 23 ++++++-----------------
 src/openvpnserv/service.h     |  3 +++
 3 files changed, 24 insertions(+), 18 deletions(-)

Comments

Lev Stipakov Oct. 2, 2018, 10:53 p.m. UTC | #1 
Hi,


> +    if (!GetSystemDirectoryW(win_sys_path, _countof(win_sys_path)))
> +    {
> +        wcsncpy(win_sys_path, default_sys_path, _countof(win_sys_path));
> +        win_sys_path[_countof(win_sys_path) - 1] = L'\0';
> +    }
>

Is there need in adding null terminator to win_sys_path?

Since it is static, it will be initialized with zeroes at startup. Besides,
wcsncpy also adds null terminator.
Lev Stipakov Oct. 2, 2018, 11:21 p.m. UTC | #2 
Apparently it is a defensive programming to make sure that string is
null-terminated also in cases where default_sys_path length equals
to win_sys_path.

So, ACK.



ke 3. lokak. 2018 klo 11.53 Lev Stipakov (lstipakov@gmail.com) kirjoitti:

> Hi,
>
>
>> +    if (!GetSystemDirectoryW(win_sys_path, _countof(win_sys_path)))
>> +    {
>> +        wcsncpy(win_sys_path, default_sys_path, _countof(win_sys_path));
>> +        win_sys_path[_countof(win_sys_path) - 1] = L'\0';
>> +    }
>>
>
> Is there need in adding null terminator to win_sys_path?
>
> Since it is static, it will be initialized with zeroes at startup.
> Besides, wcsncpy also adds null terminator.
>
> --
> -Lev
>
Gert Doering Oct. 5, 2018, 2:46 a.m. UTC | #3 
Another ACK from me, looks good.

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

(This is refactoring so normally shouldn't go to 2.4, but since the 
"set interface to DHCP" patch really is really fixing an omission in 
the initial 2.4 interactive service implementation, I see this as a 
necessary prerequisite to a bugfix)

Test compiled on ubuntu1604/mingw, did not actually run.

commit 4eb465537ce06e07ee63c2fc93c7b192dd4e29de (master)
commit f0516a803fdcf588be67159a240d6ec840ad0090 (release/2.4)
Author: Selva Nair
Date:   Tue Oct 2 16:01:12 2018 -0400

     Move get system directory to a separate function

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Alon Bar-Lev <alon.barlev@gmail.com>
     Message-Id: <1538510474-27602-1-git-send-email-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17518.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
index dc47666..73c418f 100644
--- a/src/openvpnserv/common.c
+++ b/src/openvpnserv/common.c
@@ -25,7 +25,7 @@ 
 #include "validate.h"
 
 LPCTSTR service_instance = TEXT("");
-
+static wchar_t win_sys_path[MAX_PATH];
 
 /*
  * These are necessary due to certain buggy implementations of (v)snprintf,
@@ -285,3 +285,17 @@  utf8to16(const char *utf8)
     MultiByteToWideChar(CP_UTF8, 0, utf8, -1, utf16, n);
     return utf16;
 }
+
+const wchar_t *
+get_win_sys_path(void)
+{
+    const wchar_t *default_sys_path = L"C:\\Windows\\system32";
+
+    if (!GetSystemDirectoryW(win_sys_path, _countof(win_sys_path)))
+    {
+        wcsncpy(win_sys_path, default_sys_path, _countof(win_sys_path));
+        win_sys_path[_countof(win_sys_path) - 1] = L'\0';
+    }
+
+    return win_sys_path;
+}
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 861f5e7..0489684 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -934,11 +934,10 @@  RegisterDNS(LPVOID unused)
 {
     DWORD err;
     DWORD i;
-    WCHAR sys_path[MAX_PATH];
     DWORD timeout = RDNS_TIMEOUT * 1000; /* in milliseconds */
 
-    /* default path of ipconfig command */
-    WCHAR ipcfg[MAX_PATH] = L"C:\\Windows\\system32\\ipconfig.exe";
+    /* path of ipconfig command */
+    WCHAR ipcfg[MAX_PATH];
 
     struct
     {
@@ -953,11 +952,8 @@  RegisterDNS(LPVOID unused)
 
     HANDLE wait_handles[2] = {rdns_semaphore, exit_event};
 
-    if (GetSystemDirectory(sys_path, MAX_PATH))
-    {
-        swprintf(ipcfg, MAX_PATH, L"%s\\%s", sys_path, L"ipconfig.exe");
-        ipcfg[MAX_PATH-1] = L'\0';
-    }
+    swprintf(ipcfg, _countof(ipcfg), L"%s\\%s", get_win_sys_path(), L"ipconfig.exe");
+    ipcfg[_countof(ipcfg) - 1] = L'\0';
 
     if (WaitForMultipleObjects(2, wait_handles, FALSE, timeout) == WAIT_OBJECT_0)
     {
@@ -1035,15 +1031,8 @@  netsh_dns_cmd(const wchar_t *action, const wchar_t *proto, const wchar_t *if_nam
     }
 
     /* Path of netsh */
-    int n = GetSystemDirectory(argv0, MAX_PATH);
-    if (n > 0 && n < MAX_PATH) /* got system directory */
-    {
-        wcsncat(argv0, L"\\netsh.exe", MAX_PATH - n - 1);
-    }
-    else
-    {
-        wcsncpy(argv0, L"C:\\Windows\\system32\\netsh.exe", MAX_PATH);
-    }
+    swprintf(argv0, _countof(argv0), L"%s\\%s", get_win_sys_path(), L"netsh.exe");
+    argv0[_countof(argv0) - 1] = L'\0';
 
     /* cmd template:
      * netsh interface $proto $action dns $if_name $addr [validate=no]
diff --git a/src/openvpnserv/service.h b/src/openvpnserv/service.h
index af8f37f..23b105f 100644
--- a/src/openvpnserv/service.h
+++ b/src/openvpnserv/service.h
@@ -96,4 +96,7 @@  DWORD MsgToEventLog(DWORD flags, LPCTSTR lpszMsg, ...);
 /* Convert a utf8 string to utf16. Caller should free the result */
 wchar_t *utf8to16(const char *utf8);
 
+/* return windows system directory as a pointer to a static string */
+const wchar_t *get_win_sys_path(void);
+
 #endif /* ifndef _SERVICE_H */