diff mbox series

[Openvpn-devel,5/5] iservice: Resolve MSVC C4996 warnings

Message ID 20210321144627.1621-5-simon@rozman.si
State Accepted
Headers show
Series [Openvpn-devel,1/5] MSVC: Disable LZ4 | expand

Commit Message

Kristof Provost via Openvpn-devel March 21, 2021, 3:46 a.m. UTC 
Lots of string functions were declared unsafe in favor of ..._s()
counterparts. However, the code already is careful about the buffer
size. Code analysis is just not smart enough (yet) to detect this.

The code was refactored to use ..._s() variants MSVC is considering as
"safe".

Signed-off-by: Simon Rozman <simon@rozman.si>
---
 src/openvpnserv/automatic.c   | 8 ++++----
 src/openvpnserv/common.c      | 4 ++--
 src/openvpnserv/interactive.c | 2 +-
 src/openvpnserv/service.c     | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

Comments

Arne Schwabe March 21, 2021, 4:51 a.m. UTC | #1 
Am 21.03.21 um 15:46 schrieb Simon Rozman via Openvpn-devel:
> Lots of string functions were declared unsafe in favor of ..._s()
> counterparts. However, the code already is careful about the buffer
> size. Code analysis is just not smart enough (yet) to detect this.
> 
> The code was refactored to use ..._s() variants MSVC is considering as
> "safe".

Acked-By: Arne Schwabe <arne@rfc2549.org>

This change changes Windows specifc functions to other windows specific
functions that do not emit errors. Test compiled with msvc/clang-cl

Arne
Gert Doering March 21, 2021, 6:20 a.m. UTC | #2 
Tested on Ubuntu 18 / MinGW, compiles.

Checked with the MSVC documentation, seems to make sense :-) - I do notice
that we use wcscat_s() in one of these hunks, and _tcscat_s() in another,
which seems to be the same thing if _UNICODE is defined (which, I think
we do).  Maybe an opportunity for another round of cleanup - unify the
windows string functions used to "just one variant".

From: line fixed, so Author: is right for this commit.

Your patch has been applied to the master branch.

commit df471f4de8af0cbcf23a4e36910554bea7bd9058
Author: Simon Rozman
Date:   Sun Mar 21 15:46:27 2021 +0100

     iservice: Resolve MSVC C4996 warnings

     Signed-off-by: Simon Rozman <simon@rozman.si>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20210321144627.1621-5-simon@rozman.si>
     URL: https://www.mail-archive.com/search?l=mid&q=20210321144627.1621-5-simon@rozman.si
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
index 3f2ca345..0ba222a0 100644
--- a/src/openvpnserv/automatic.c
+++ b/src/openvpnserv/automatic.c
@@ -137,7 +137,7 @@  modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext)
 
     if (size > 0 && (_tcslen(src) + 1) <= size)
     {
-        _tcscpy(dest, src);
+        _tcscpy_s(dest, size, src);
         dest [size - 1] = TEXT('\0');
         i = _tcslen(dest);
         while (i-- > 0)
@@ -154,8 +154,8 @@  modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext)
         }
         if (_tcslen(dest) + _tcslen(newext) + 2 <= size)
         {
-            _tcscat(dest, TEXT("."));
-            _tcscat(dest, newext);
+            _tcscat_s(dest, size, TEXT("."));
+            _tcscat_s(dest, size, newext);
             return true;
         }
         dest[0] = TEXT('\0');
@@ -271,7 +271,7 @@  ServiceStartAutomatic(DWORD dwArgc, LPTSTR *lpszArgv)
         BOOL more_files;
         TCHAR find_string[MAX_PATH];
 
-        openvpn_sntprintf(find_string, MAX_PATH, TEXT("%s\\*"), settings.config_dir);
+        openvpn_sntprintf(find_string, _countof(find_string), TEXT("%s\\*"), settings.config_dir);
 
         find_handle = FindFirstFile(find_string, &find_obj);
         if (find_handle == INVALID_HANDLE_VALUE)
diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
index 958643df..48769be4 100644
--- a/src/openvpnserv/common.c
+++ b/src/openvpnserv/common.c
@@ -37,7 +37,7 @@  openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist)
     int len = -1;
     if (size > 0)
     {
-        len = _vsntprintf(str, size, format, arglist);
+        len = _vsntprintf_s(str, size, _TRUNCATE, format, arglist);
         str[size - 1] = 0;
     }
     return (len >= 0 && (size_t)len < size);
@@ -311,7 +311,7 @@  get_win_sys_path(void)
 
     if (!GetSystemDirectoryW(win_sys_path, _countof(win_sys_path)))
     {
-        wcsncpy(win_sys_path, default_sys_path, _countof(win_sys_path));
+        wcscpy_s(win_sys_path, _countof(win_sys_path), default_sys_path);
         win_sys_path[_countof(win_sys_path) - 1] = L'\0';
     }
 
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index b073a0d5..ed83d2a3 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -1067,7 +1067,7 @@  netsh_dns_cmd(const wchar_t *action, const wchar_t *proto, const wchar_t *if_nam
 
     if (IsWindows7OrGreater())
     {
-        wcsncat(cmdline, L" validate=no", ncmdline - wcslen(cmdline) - 1);
+        wcscat_s(cmdline, ncmdline, L" validate=no");
     }
     err = ExecCommand(argv0, cmdline, timeout);
 
diff --git a/src/openvpnserv/service.c b/src/openvpnserv/service.c
index 8efe25f9..8101f83d 100644
--- a/src/openvpnserv/service.c
+++ b/src/openvpnserv/service.c
@@ -61,14 +61,14 @@  CmdInstallServices()
     TCHAR path[512];
     int i, ret = _service_max;
 
-    if (GetModuleFileName(NULL, path + 1, 510) == 0)
+    if (GetModuleFileName(NULL, path + 1, _countof(path) - 2) == 0)
     {
         _tprintf(TEXT("Unable to install service - %s\n"), GetLastErrorText());
         return 1;
     }
 
     path[0] = TEXT('\"');
-    _tcscat(path, TEXT("\""));
+    _tcscat_s(path, _countof(path), TEXT("\""));
 
     svc_ctl_mgr = OpenSCManager(NULL, NULL, SC_MANAGER_CONNECT | SC_MANAGER_CREATE_SERVICE);
     if (svc_ctl_mgr == NULL)