diff mbox series

[Openvpn-devel,v5] openvpnserv: Review MSVC down-casting warnings

Message ID 20171203203007.6628-1-simon@rozman.si
State Accepted
Headers
Series [Openvpn-devel,v5] openvpnserv: Review MSVC down-casting warnings |

Commit Message

Simon Rozman Dec. 3, 2017, 9:30 a.m. UTC 
  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(-)

Comments

Selva Nair Dec. 3, 2017, 10:25 a.m. UTC | #1 
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">&lt;<a href="mailto:simon@rozman.si" target="_blank">simon@rozman.si</a>&gt;</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-&gt;dwFileAttributes &amp; FILE_ATTRIBUTE_DIRECTORY)<br>
     {<br>
         return false;<br>
     }<br>
<br>
-    if (!_tcslen(ext))<br>
</span><span class="m_6898804296425451684gmail-">+    if (*ext == TEXT(&#39;\0&#39;))<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&#39;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-&gt;cFileName) - _tcslen(ext) - 1;<br>
-    if (i &lt; 1)<br>
</span><span class="m_6898804296425451684gmail-">-    {<br>
-        return false;<br>
-    }<br>
</span><span class="m_6898804296425451684gmail-">+    /* find the pointer to that last &#39;.&#39; in filename and match ext against the rest */<br>
<br>
-    return find-&gt;cFileName[i] == &#39;.&#39; &amp;&amp; !_tcsicmp(find-&gt;cFileName + i + 1, ext);<br>
</span><div><div class="m_6898804296425451684gmail-h5">+    const TCHAR *p = _tcsrchr(find-&gt;cFileName, TEXT(&#39;.&#39;));<br>
+    return p &amp;&amp; p != find-&gt;cFileName &amp;&amp; _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 &gt; 0 &amp;&amp; (_tcslen(src) + 1) &lt;= size)<br>
     {<br>
         _tcscpy(dest, src);<br>
         dest [size - 1] = TEXT(&#39;\0&#39;);<br>
         i = _tcslen(dest);<br>
-        while (--i &gt;= 0)<br>
+        while (i-- &gt; 0)<br>
         {<br>
             if (dest[i] == TEXT(&#39;\\&#39;))<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&quot;0x%08x\n0x%08x\n%s&quot;, 0, pid, msg);<br>
     buf[_countof(buf) - 1] = &#39;\0&#39;;<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&quot;0x%1!08x!\n%2!s!\n%3!s!&quot;, 0, 0,<br>
                                 (LPWSTR) &amp;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, &amp;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&quot;netsh interface %s %s dns \&quot;%s\&quot; %s&quot;;<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), &amp;written, NULL);<br>
+        WriteFile(stdin_write, input, (DWORD)strlen(input), &amp;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 &lt;<a href="mailto:selva.nair@gmail.com" target="_blank">selva.nair@gmail.com</a>&gt;</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
Gert Doering Dec. 4, 2017, 5:54 a.m. UTC | #2 
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 mbox series

Patch

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);
     }