| Message ID | 20210321144627.1621-4-simon@rozman.si |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,1/5] MSVC: Disable LZ4 | expand |
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
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
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 --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)
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(-)