diff mbox series

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

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

Commit Message

haixiao.yan.cn--- via Openvpn-devel March 21, 2021, 3:46 a.m. UTC 
  wcsncat() was declared unsafe in favour of wcsncat_s(). However, the
string concatenation follows the string length check, making wcsncat()
safe too. Code analysis is just not smart enough (yet) to detect this.

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

Signed-off-by: Simon Rozman <simon@rozman.si>
---
 src/tapctl/tap.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Arne Schwabe March 21, 2021, 5:29 a.m. UTC | #1 
Am 21.03.21 um 15:46 schrieb Simon Rozman via Openvpn-devel:
> wcsncat() was declared unsafe in favour of wcsncat_s(). However, the
> string concatenation follows the string length check, making wcsncat()
> safe too. Code analysis is just not smart enough (yet) to detect this.
> 
> The code was refactored to use wcscat_s() MSVC is considering as "safe".
> 
> Signed-off-by: Simon Rozman <simon@rozman.si>
> ---
>  src/tapctl/tap.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/src/tapctl/tap.c b/src/tapctl/tap.c
> index dd4a10a3..3f76c43a 100644
> --- a/src/tapctl/tap.c
> +++ b/src/tapctl/tap.c
> @@ -2,7 +2,7 @@
>   *  tapctl -- Utility to manipulate TUN/TAP adapters on Windows
>   *            https://community.openvpn.net/openvpn/wiki/Tapctl
>   *
> - *  Copyright (C) 2018-2020 Simon Rozman <simon@rozman.si>
> + *  Copyright (C) 2018-2021 Simon Rozman <simon@rozman.si>
>   *
>   *  This program is free software; you can redistribute it and/or modify
>   *  it under the terms of the GNU General Public License version 2
> @@ -73,14 +73,13 @@ find_function(const WCHAR *libname, const char *funcname, HMODULE *m)
>         return NULL;
>      }
>  
> -    size_t len = _countof(libpath) - wcslen(libpath) - 1;
> -    if (len < wcslen(libname) + 1)
> +    if (wcslen(libpath) + 1 /*\*/ + wcslen(libname) >= _countof(libpath))

This random inline comment feels extremely weird.

Arne
haixiao.yan.cn--- via Openvpn-devel March 21, 2021, 5:37 a.m. UTC | #2 
Hi,

> > -73,14 +73,13 @@ find_function(const WCHAR *libname, const char
> *funcname, HMODULE *m)
> >         return NULL;
> >      }
> >
> > -    size_t len = _countof(libpath) - wcslen(libpath) - 1;
> > -    if (len < wcslen(libname) + 1)
> > +    if (wcslen(libpath) + 1 /*\*/ + wcslen(libname) >=
> > + _countof(libpath))
> 
> This random inline comment feels extremely weird.

It's trying to describe the "+ 1" amounts for a backslash \ being strcat-ed in the process below.

Regards, Simon
Arne Schwabe March 21, 2021, 5:49 a.m. UTC | #3 
Am 21.03.21 um 17:37 schrieb Simon Rozman:
> Hi,
> 
>>> -73,14 +73,13 @@ find_function(const WCHAR *libname, const char
>> *funcname, HMODULE *m)
>>>         return NULL;
>>>      }
>>>
>>> -    size_t len = _countof(libpath) - wcslen(libpath) - 1;
>>> -    if (len < wcslen(libname) + 1)
>>> +    if (wcslen(libpath) + 1 /*\*/ + wcslen(libname) >=
>>> + _countof(libpath))
>>
>> This random inline comment feels extremely weird.
> 
> It's trying to describe the "+ 1" amounts for a backslash \ being strcat-ed in the process below.
> 
> Regards, Simon
> 

I totally didn't get that. I was actually wondering why that is valid
code and why the \* doesn't escape the end of comment. I would prefer it
be either a longer inline comment or even better use a variable with a
normal comment like

/* +1 for the path seperator '\' */
size_t pathlenght = wcslen(libpath) + 1 +  wcslen(libname);

Arne
diff mbox series

Patch

diff --git a/src/tapctl/tap.c b/src/tapctl/tap.c
index dd4a10a3..3f76c43a 100644
--- a/src/tapctl/tap.c
+++ b/src/tapctl/tap.c
@@ -2,7 +2,7 @@ 
  *  tapctl -- Utility to manipulate TUN/TAP adapters on Windows
  *            https://community.openvpn.net/openvpn/wiki/Tapctl
  *
- *  Copyright (C) 2018-2020 Simon Rozman <simon@rozman.si>
+ *  Copyright (C) 2018-2021 Simon Rozman <simon@rozman.si>
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2
@@ -73,14 +73,13 @@  find_function(const WCHAR *libname, const char *funcname, HMODULE *m)
        return NULL;
     }
 
-    size_t len = _countof(libpath) - wcslen(libpath) - 1;
-    if (len < wcslen(libname) + 1)
+    if (wcslen(libpath) + 1 /*\*/ + wcslen(libname) >= _countof(libpath))
     {
        SetLastError(ERROR_INSUFFICIENT_BUFFER);
        return NULL;
     }
-    wcsncat(libpath, L"\\", len);
-    wcsncat(libpath, libname, len-1);
+    wcscat_s(libpath, _countof(libpath), L"\\");
+    wcscat_s(libpath, _countof(libpath), libname);
 
     *m = LoadLibraryW(libpath);
     if (*m == NULL)