| Message ID | 20171203203007.6628-1-simon@rozman.si |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v5] openvpnserv: Review MSVC down-casting warnings | expand |
Hi, This looks good now. The v1 of this patch and main review comments are in a different thread: https://sourceforge.net/p/openvpn/mailman/openvpn-devel/ thread/20171010231130.6832-12-simon%40rozman.si/#msg36071613 On Sun, Dec 3, 2017 at 3:30 PM, Simon Rozman <simon@rozman.si> wrote: > Data size arithmetic was reviewed according to 64-bit MSVC complaints. > > The warnings were addressed by migrating to size_t, rewriting the code, > or silencing the warnings by an explicit cast where appropriate. > --- > src/openvpnserv/automatic.c | 17 ++++++----------- > src/openvpnserv/interactive.c | 12 ++++++------ > 2 files changed, 12 insertions(+), 17 deletions(-) > > diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c > index 75c3be2..6f82b4e 100644 > --- a/src/openvpnserv/automatic.c > +++ b/src/openvpnserv/automatic.c > @@ -115,25 +115,20 @@ close_if_open(HANDLE h) > static bool > match(const WIN32_FIND_DATA *find, LPCTSTR ext) > { > - int i; > - > if (find->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) > { > return false; > } > > - if (!_tcslen(ext)) > + if (*ext == TEXT('\0')) > { > return true; } > Empty extension matching all files is somewhat strange but that's the original behaviour (unlike I initially thought), and is preserved by the patch. > > - i = _tcslen(find->cFileName) - _tcslen(ext) - 1; > - if (i < 1) > - { > - return false; > - } > + /* find the pointer to that last '.' in filename and match ext > against the rest */ > > - return find->cFileName[i] == '.' && !_tcsicmp(find->cFileName + i + > 1, ext); > + const TCHAR *p = _tcsrchr(find->cFileName, TEXT('.')); > + return p && p != find->cFileName && _tcsicmp(p + 1, ext) == 0; > } > > /* > @@ -142,14 +137,14 @@ match(const WIN32_FIND_DATA *find, LPCTSTR ext) > static bool > modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext) > { > - int i; > + size_t i; > > if (size > 0 && (_tcslen(src) + 1) <= size) > { > _tcscpy(dest, src); > dest [size - 1] = TEXT('\0'); > i = _tcslen(dest); > - while (--i >= 0) > + while (i-- > 0) > { > if (dest[i] == TEXT('\\')) > { > diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c > index ed4603e..5d58ceb 100644 > --- a/src/openvpnserv/interactive.c > +++ b/src/openvpnserv/interactive.c > @@ -280,7 +280,7 @@ ReturnProcessId(HANDLE pipe, DWORD pid, DWORD count, > LPHANDLE events) > swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg); > buf[_countof(buf) - 1] = '\0'; > > - WritePipeAsync(pipe, buf, wcslen(buf) * 2, count, events); > + WritePipeAsync(pipe, buf, (DWORD)(wcslen(buf) * 2), count, events); > } > > static VOID > @@ -308,7 +308,7 @@ ReturnError(HANDLE pipe, DWORD error, LPCWSTR func, > DWORD count, LPHANDLE events > L"0x%1!08x!\n%2!s!\n%3!s!", 0, 0, > (LPWSTR) &result, 0, (va_list *) args); > > - WritePipeAsync(pipe, result, wcslen(result) * 2, count, events); > + WritePipeAsync(pipe, result, (DWORD)(wcslen(result) * 2), count, > events); > #ifdef UNICODE > MsgToEventLog(MSG_FLAGS_ERROR, result); > #else > @@ -452,10 +452,10 @@ out: > static BOOL > GetStartupData(HANDLE pipe, STARTUP_DATA *sud) > { > - size_t len; > + size_t size, len; > BOOL ret = FALSE; > WCHAR *data = NULL; > - DWORD size, bytes, read; > + DWORD bytes, read; > > bytes = PeekNamedPipeAsync(pipe, 1, &exit_event); > if (bytes == 0) > @@ -1051,7 +1051,7 @@ netsh_dns_cmd(const wchar_t *action, const wchar_t > *proto, const wchar_t *if_nam > const wchar_t *fmt = L"netsh interface %s %s dns \"%s\" %s"; > > /* max cmdline length in wchars -- include room for worst case and > some */ > - int ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 + 1; > + size_t ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 + > 1; > wchar_t *cmdline = malloc(ncmdline*sizeof(wchar_t)); > if (!cmdline) > { > @@ -1571,7 +1571,7 @@ RunOpenvpn(LPVOID p) > { > DWORD written; > WideCharToMultiByte(CP_UTF8, 0, sud.std_input, -1, input, > input_size, NULL, NULL); > - WriteFile(stdin_write, input, strlen(input), &written, NULL); > + WriteFile(stdin_write, input, (DWORD)strlen(input), &written, > NULL); > free(input); > } > Cleaner and shorter code and should silence some pesky MSVC warnings. Acked-by: Selva Nair <selva.nair@gmail.com> <div dir="ltr">Hi,<div><br></div><div>This looks good now.</div><div><br></div><div>The v1 of this patch and main review comments are in a different thread:<br></div><div><a href="https://sourceforge.net/p/openvpn/mailman/openvpn-devel/thread/20171010231130.6832-12-simon%40rozman.si/#msg36071613" target="_blank">https://sourceforge.net/p/<wbr>openvpn/mailman/openvpn-devel/<wbr>thread/20171010231130.6832-12-<wbr>simon%40rozman.si/#msg36071613</a><br></div><div><br></div><div><div class="gmail_extra"><div class="gmail_quote">On Sun, Dec 3, 2017 at 3:30 PM, Simon Rozman <span dir="ltr"><<a href="mailto:simon@rozman.si" target="_blank">simon@rozman.si</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_6898804296425451684gmail-">Data size arithmetic was reviewed according to 64-bit MSVC complaints.<br> <br> The warnings were addressed by migrating to size_t, rewriting the code,<br> or silencing the warnings by an explicit cast where appropriate.<br> ---<br> </span> src/openvpnserv/automatic.c | 17 ++++++-----------<br> src/openvpnserv/interactive.c | 12 ++++++------<br> 2 files changed, 12 insertions(+), 17 deletions(-)<br> <br> diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c<br> index 75c3be2..6f82b4e 100644<br> <span class="m_6898804296425451684gmail-">--- a/src/openvpnserv/automatic.c<br> +++ b/src/openvpnserv/automatic.c<br> @@ -115,25 +115,20 @@ close_if_open(HANDLE h)<br> static bool<br> match(const WIN32_FIND_DATA *find, LPCTSTR ext)<br> {<br> - int i;<br> -<br> if (find->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)<br> {<br> return false;<br> }<br> <br> - if (!_tcslen(ext))<br> </span><span class="m_6898804296425451684gmail-">+ if (*ext == TEXT('\0'))<br> {<br> </span> return true; </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <span class="m_6898804296425451684gmail-"> }<br></span></blockquote><div><br></div><div>Empty extension matching all files is somewhat strange but that's the</div><div>original behaviour (unlike I initially thought), and is preserved by the patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_6898804296425451684gmail-"> <br> - i = _tcslen(find->cFileName) - _tcslen(ext) - 1;<br> - if (i < 1)<br> </span><span class="m_6898804296425451684gmail-">- {<br> - return false;<br> - }<br> </span><span class="m_6898804296425451684gmail-">+ /* find the pointer to that last '.' in filename and match ext against the rest */<br> <br> - return find->cFileName[i] == '.' && !_tcsicmp(find->cFileName + i + 1, ext);<br> </span><div><div class="m_6898804296425451684gmail-h5">+ const TCHAR *p = _tcsrchr(find->cFileName, TEXT('.'));<br> + return p && p != find->cFileName && _tcsicmp(p + 1, ext) == 0;<br> }<br> <br> /*<br> @@ -142,14 +137,14 @@ match(const WIN32_FIND_DATA *find, LPCTSTR ext)<br> static bool<br> modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext)<br> {<br> - int i;<br> + size_t i;<br> <br> if (size > 0 && (_tcslen(src) + 1) <= size)<br> {<br> _tcscpy(dest, src);<br> dest [size - 1] = TEXT('\0');<br> i = _tcslen(dest);<br> - while (--i >= 0)<br> + while (i-- > 0)<br> {<br> if (dest[i] == TEXT('\\'))<br> {<br> diff --git a/src/openvpnserv/interactive.<wbr>c b/src/openvpnserv/interactive.<wbr>c<br> index ed4603e..5d58ceb 100644<br> --- a/src/openvpnserv/interactive.<wbr>c<br> +++ b/src/openvpnserv/interactive.<wbr>c<br> @@ -280,7 +280,7 @@ ReturnProcessId(HANDLE pipe, DWORD pid, DWORD count, LPHANDLE events)<br> swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg);<br> buf[_countof(buf) - 1] = '\0';<br> <br> - WritePipeAsync(pipe, buf, wcslen(buf) * 2, count, events);<br> + WritePipeAsync(pipe, buf, (DWORD)(wcslen(buf) * 2), count, events);<br> }<br> <br> static VOID<br> @@ -308,7 +308,7 @@ ReturnError(HANDLE pipe, DWORD error, LPCWSTR func, DWORD count, LPHANDLE events<br> L"0x%1!08x!\n%2!s!\n%3!s!", 0, 0,<br> (LPWSTR) &result, 0, (va_list *) args);<br> <br> - WritePipeAsync(pipe, result, wcslen(result) * 2, count, events);<br> + WritePipeAsync(pipe, result, (DWORD)(wcslen(result) * 2), count, events);<br> #ifdef UNICODE<br> MsgToEventLog(MSG_FLAGS_<wbr>ERROR, result);<br> #else<br> @@ -452,10 +452,10 @@ out:<br> static BOOL<br> GetStartupData(HANDLE pipe, STARTUP_DATA *sud)<br> {<br> - size_t len;<br> + size_t size, len;<br> BOOL ret = FALSE;<br> WCHAR *data = NULL;<br> - DWORD size, bytes, read;<br> + DWORD bytes, read;<br> <br> bytes = PeekNamedPipeAsync(pipe, 1, &exit_event);<br> if (bytes == 0)<br> @@ -1051,7 +1051,7 @@ netsh_dns_cmd(const wchar_t *action, const wchar_t *proto, const wchar_t *if_nam<br> const wchar_t *fmt = L"netsh interface %s %s dns \"%s\" %s";<br> <br> /* max cmdline length in wchars -- include room for worst case and some */<br> - int ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 + 1;<br> + size_t ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 + 1;<br> wchar_t *cmdline = malloc(ncmdline*sizeof(wchar_t<wbr>));<br> if (!cmdline)<br> {<br> @@ -1571,7 +1571,7 @@ RunOpenvpn(LPVOID p)<br> {<br> DWORD written;<br> WideCharToMultiByte(CP_UTF8, 0, sud.std_input, -1, input, input_size, NULL, NULL);<br> - WriteFile(stdin_write, input, strlen(input), &written, NULL);<br> + WriteFile(stdin_write, input, (DWORD)strlen(input), &written, NULL);<br> free(input);<br> }<br></div></div></blockquote><div><br></div><div>Cleaner and shorter code and should silence some pesky MSVC</div><div>warnings.</div><div><br></div><div>Acked-by: Selva Nair <<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>></div></div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Thanks for your patience, and thanks, Selva, for reviewing :-)
I haven't test-compiled or even test-run it, but stared at the code and
it looks good for me, too.
Your patch has been applied to the master branch (refactoring -> master).
commit 02fa8565498d701b5750ad19ee29be4fe657a7f5
Author: Simon Rozman
Date: Sun Dec 3 21:30:07 2017 +0100
openvpnserv: Review MSVC down-casting warnings
Acked-by: Selva Nair <selva.nair@gmail.com>
Message-Id: <20171203203007.6628-1-simon@rozman.si>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16001.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c index 75c3be2..6f82b4e 100644 --- a/src/openvpnserv/automatic.c +++ b/src/openvpnserv/automatic.c @@ -115,25 +115,20 @@ close_if_open(HANDLE h) static bool match(const WIN32_FIND_DATA *find, LPCTSTR ext) { - int i; - if (find->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) { return false; } - if (!_tcslen(ext)) + if (*ext == TEXT('\0')) { return true; } - i = _tcslen(find->cFileName) - _tcslen(ext) - 1; - if (i < 1) - { - return false; - } + /* find the pointer to that last '.' in filename and match ext against the rest */ - return find->cFileName[i] == '.' && !_tcsicmp(find->cFileName + i + 1, ext); + const TCHAR *p = _tcsrchr(find->cFileName, TEXT('.')); + return p && p != find->cFileName && _tcsicmp(p + 1, ext) == 0; } /* @@ -142,14 +137,14 @@ match(const WIN32_FIND_DATA *find, LPCTSTR ext) static bool modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext) { - int i; + size_t i; if (size > 0 && (_tcslen(src) + 1) <= size) { _tcscpy(dest, src); dest [size - 1] = TEXT('\0'); i = _tcslen(dest); - while (--i >= 0) + while (i-- > 0) { if (dest[i] == TEXT('\\')) { diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index ed4603e..5d58ceb 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -280,7 +280,7 @@ ReturnProcessId(HANDLE pipe, DWORD pid, DWORD count, LPHANDLE events) swprintf(buf, _countof(buf), L"0x%08x\n0x%08x\n%s", 0, pid, msg); buf[_countof(buf) - 1] = '\0'; - WritePipeAsync(pipe, buf, wcslen(buf) * 2, count, events); + WritePipeAsync(pipe, buf, (DWORD)(wcslen(buf) * 2), count, events); } static VOID @@ -308,7 +308,7 @@ ReturnError(HANDLE pipe, DWORD error, LPCWSTR func, DWORD count, LPHANDLE events L"0x%1!08x!\n%2!s!\n%3!s!", 0, 0, (LPWSTR) &result, 0, (va_list *) args); - WritePipeAsync(pipe, result, wcslen(result) * 2, count, events); + WritePipeAsync(pipe, result, (DWORD)(wcslen(result) * 2), count, events); #ifdef UNICODE MsgToEventLog(MSG_FLAGS_ERROR, result); #else @@ -452,10 +452,10 @@ out: static BOOL GetStartupData(HANDLE pipe, STARTUP_DATA *sud) { - size_t len; + size_t size, len; BOOL ret = FALSE; WCHAR *data = NULL; - DWORD size, bytes, read; + DWORD bytes, read; bytes = PeekNamedPipeAsync(pipe, 1, &exit_event); if (bytes == 0) @@ -1051,7 +1051,7 @@ netsh_dns_cmd(const wchar_t *action, const wchar_t *proto, const wchar_t *if_nam const wchar_t *fmt = L"netsh interface %s %s dns \"%s\" %s"; /* max cmdline length in wchars -- include room for worst case and some */ - int ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 + 1; + size_t ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 + 1; wchar_t *cmdline = malloc(ncmdline*sizeof(wchar_t)); if (!cmdline) { @@ -1571,7 +1571,7 @@ RunOpenvpn(LPVOID p) { DWORD written; WideCharToMultiByte(CP_UTF8, 0, sud.std_input, -1, input, input_size, NULL, NULL); - WriteFile(stdin_write, input, strlen(input), &written, NULL); + WriteFile(stdin_write, input, (DWORD)strlen(input), &written, NULL); free(input); }