[Openvpn-devel,12/13] Memory size arithmetic reviewed according to 64-bit MSVC complaints

  From: Simon Rozman <simon@rozman.si>

---
 src/openvpnserv/automatic.c   | 14 ++++++++------
 src/openvpnserv/interactive.c | 12 ++++++------
 2 files changed, 14 insertions(+), 12 deletions(-)
  

Hi,

Some of these changes are of dubious value as the string lengths
involved are guaranteed to be small and there is no scope
for overflow. And casting only stops the compiler warning, not potential
overflow, if any..

As for the offending mixed int/size_t arithmetic, a better option is
to just to avoid the arithmetic -- code gets clearer and shorter too:

On Tue, Oct 10, 2017 at 7:11 PM, <simon@rozman.si> wrote:

> From: Simon Rozman <simon@rozman.si>
>
> ---
>  src/openvpnserv/automatic.c   | 14 ++++++++------
>  src/openvpnserv/interactive.c | 12 ++++++------
>  2 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
> index 6c39aaa..3e8f6ed 100644
> --- a/src/openvpnserv/automatic.c
> +++ b/src/openvpnserv/automatic.c
> @@ -130,24 +130,26 @@ close_if_open(HANDLE h)
>  static bool
>  match(const WIN32_FIND_DATA *find, LPCTSTR ext)
>  {

-    int i;
> +    size_t i, len_filename, len_ext;
>

Instead, we can just get rid of these as below

     if (find->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
>      {
>          return false;
>      }
>

The rest of this function (extension matching) may be made simpler
and hopefully easier to follow as

    if (*ext == TEXT('\0'))
    {
         return false;
    }

    /* find the pointer to that last '.' in filename and match ext against
the rest */

    const char *p = _tcsrchr(find->cFileName, TEXT('.'));
    return p && p != find->cFileName && !_tcsicmp(p+1, ext);
}

No _tcslen() and no offending size variables.


> -    if (!_tcslen(ext))
> +    len_ext = _tcslen(ext);
> +    if (!len_ext)
>      {
>          return true;
>      }
>
> -    i = _tcslen(find->cFileName) - _tcslen(ext) - 1;
> -    if (i < 1)
> +    len_filename = _tcslen(find->cFileName);
> +    if (len_filename < len_ext + 2)
>      {
>          return false;
>      }
>
> +    i = len_filename - len_ext - 1;
>      return find->cFileName[i] == '.' && !_tcsicmp(find->cFileName + i +
> 1, ext);

 }
>

And that convoluted original is then gone.


> @@ -157,14 +159,14 @@ match(const WIN32_FIND_DATA *find, LPCTSTR ext)
>  static bool
>  modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext)
>  {
> -    int i;
> +    size_t i;
>

OK

     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 96e0de0..9a4c01a 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -280,7 +280,7 @@ ReturnProcessId(HANDLE pipe, DWORD pid, DWORD count,
> LPHANDLE events)
>      _snwprintf(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);


Isn't this identical to the original automatic conversion from size_t to
DWORD?
An overly paranoid assert(_countof(buf) < MAXDWORD/2) may catch an extremely
unlikely future coding error, but in what way a cast safer?



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

Same here.


>  #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;
>

OK


>
>      bytes = PeekNamedPipeAsync(pipe, 1, &exit_event);
>      if (bytes == 0)
> @@ -1163,7 +1163,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;
>

OK


>      wchar_t *cmdline = malloc(ncmdline*sizeof(wchar_t));
>      if (!cmdline)
>      {
> @@ -1683,7 +1683,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);
>

As above..


>          free(input);
>      }
>
>
Thanks,

Selva
<div dir="ltr">Hi,<br><div class="gmail_extra"><br></div><div class="gmail_extra">Some of these changes are of dubious value as the string lengths</div><div class="gmail_extra">involved are guaranteed to be small and there is no scope</div><div class="gmail_extra">for overflow. And casting only stops the compiler warning, not potential</div><div class="gmail_extra">overflow, if any..</div><div class="gmail_extra"><br></div><div class="gmail_extra">As for the offending mixed int/size_t arithmetic, a better option is</div><div class="gmail_extra">to just to avoid the arithmetic -- code gets clearer and shorter too:</div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_quote">On Tue, Oct 10, 2017 at 7:11 PM,  <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">From: Simon Rozman &lt;<a href="mailto:simon@rozman.si" target="_blank">simon@rozman.si</a>&gt;<br>
<br>
---<br>
 src/openvpnserv/automatic.c   | 14 ++++++++------<br>
 src/openvpnserv/interactive.c | 12 ++++++------<br>
 2 files changed, 14 insertions(+), 12 deletions(-)<br>
<br>
diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c<br>
index 6c39aaa..3e8f6ed 100644<br>
--- a/src/openvpnserv/automatic.c<br>
+++ b/src/openvpnserv/automatic.c<br>
@@ -130,24 +130,26 @@ close_if_open(HANDLE h)<br>
 static bool<br>
 match(const WIN32_FIND_DATA *find, LPCTSTR ext)<br>
 { </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
-    int i;<br>
+    size_t i, len_filename, len_ext;<br></blockquote><div><br></div><div>Instead, we can just get rid of these as below</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
     if (find-&gt;dwFileAttributes &amp; FILE_ATTRIBUTE_DIRECTORY)<br>
     {<br>
         return false;<br>
     }<br></blockquote><div><br></div><div>The rest of this function (extension matching) may be made simpler</div><div>and hopefully easier to follow as</div><div><br></div><div>    if (*ext == TEXT(&#39;\0&#39;))</div><div>    {</div><div>         return false;</div><div>    }</div><div><br></div><div>    /* find the pointer to that last &#39;.&#39; in filename and match ext against the rest */</div><div><br></div><div>    const char *p = _tcsrchr(find-&gt;cFileName, TEXT(&#39;.&#39;));</div><div>    return p &amp;&amp; p != find-&gt;cFileName &amp;&amp; !_tcsicmp(p+1, ext);</div><div>}</div><div><br></div><div>No _tcslen() and no offending size variables.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
-    if (!_tcslen(ext))<br>
+    len_ext = _tcslen(ext);<br>
+    if (!len_ext)<br>
     {<br>
         return true;<br>
     }<br>
<br>
-    i = _tcslen(find-&gt;cFileName) - _tcslen(ext) - 1;<br>
-    if (i &lt; 1)<br>
+    len_filename = _tcslen(find-&gt;cFileName);<br>
+    if (len_filename &lt; len_ext + 2)<br>
     {<br>
         return false;<br>
     }<br>
<br>
+    i = len_filename - len_ext - 1;<br>
     return find-&gt;cFileName[i] == &#39;.&#39; &amp;&amp; !_tcsicmp(find-&gt;cFileName + i + 1, ext);</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
 }<br></blockquote><div><br></div><div>And that convoluted original is then gone.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
@@ -157,14 +159,14 @@ match(const WIN32_FIND_DATA *find, LPCTSTR ext)<br>
 static bool<br>
 modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext)<br>
 {<br>
-    int i;<br>
+    size_t i;<br></blockquote><div><br></div><div>OK</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
     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) </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
         {<br>
             if (dest[i] == TEXT(&#39;\\&#39;))<br>
             {</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
diff --git a/src/openvpnserv/interactive.<wbr>c b/src/openvpnserv/interactive.<wbr>c<br>
index 96e0de0..9a4c01a 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>
     _snwprintf(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);</blockquote><div><br></div><div>Isn&#39;t this identical to the original automatic conversion from size_t to DWORD?</div><div>An overly paranoid assert(_countof(buf) &lt; MAXDWORD/2) may catch an extremely</div><div>unlikely future coding error, but in what way a cast safer?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
 }<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></blockquote><div><br></div><div>Same here.</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">
 #ifdef UNICODE<br>
     MsgToEventLog(MSG_FLAGS_ERROR<wbr>, 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></blockquote><div><br></div><div>OK</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">
<br>
     bytes = PeekNamedPipeAsync(pipe, 1, &amp;exit_event);<br>
     if (bytes == 0)<br>
@@ -1163,7 +1163,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></blockquote><div><br></div><div>OK</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">
     wchar_t *cmdline = malloc(ncmdline*sizeof(wchar_t<wbr>));<br>
     if (!cmdline)<br>
     {<br>
@@ -1683,7 +1683,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></blockquote><div><br></div><div>As above..</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
         free(input);<br>
     }<br>
<span class="m_2792988390416823077m_5558159874359295240m_8470299133570871556m_8856265837151638397m_6434295266320317594m_1582231105454850336gmail-m_-6650795910552092661HOEnZb"><font color="#888888"><br></font></span></blockquote><div><br></div><div>Thanks,</div><div><br></div><div>Selva</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
  

Hi,

> Some of these changes are of dubious value as the string lengths involved
> are guaranteed to be small and there is no scope for overflow. And casting
> only stops the compiler warning, not potential overflow, if any..

Exactly. Where there's no scope for an overflow and compiler is too dumb to notice that, why not "teaching" it a bit?


> As for the offending mixed int/size_t arithmetic, a better option is to just
> to
> avoid the arithmetic -- code gets clearer and shorter too:

Though, I see no problem with size_t/ptrdiff_t arithmetic when implemented properly. O:-)


>     const char *p = _tcsrchr(find->cFileName, TEXT('.'));
>     return p && p != find->cFileName && !_tcsicmp(p+1, ext);
>
> No _tcslen() and no offending size variables.
> And that convoluted original is then gone.

The resulting code would look a lot nicer indeed. Stay tuned for v2 of this patch.


> -    WritePipeAsync(pipe, buf, wcslen(buf) * 2, count, events);
> +    WritePipeAsync(pipe, buf, (DWORD)(wcslen(buf) * 2), count, events);
>
> Isn't this identical to the original automatic conversion from size_t to
> DWORD?
> An overly paranoid assert(_countof(buf) < MAXDWORD/2) may catch an
> extremely
> unlikely future coding error, but in what way a cast safer?

I didn't say it is safer. The buf is WCHAR[33] on the stack. There is a safety zero termination a line before the WritePipeAsync(). Therefore, wcslen(buf) will always return between 0 and 32. Times two (if I was coding this, I'd use *sizeof(WCHAR) instead of 2, to make a clear statement, what is multiplication by 2 about), making it between 0 and 64. Unfortunately, result is size_t where Win32 API expects a DWORD.

This will always produce a false compiler warning. I muted it by an explicit cast, so we can focus on more important warnings.


> -    WritePipeAsync(pipe, result, wcslen(result) * 2, count, events);
> +    WritePipeAsync(pipe, result, (DWORD)(wcslen(result) * 2), count,
> events);	
>
> Same here.

The `result` is a Windows error message. Do you really expect Windows error messages will ever grow beyond 2G?
(2G TCHARs*sizeof(TCHAR) = 4GB > MAXDWORD)


> -        WriteFile(stdin_write, input, strlen(input), &written, NULL);
> +        WriteFile(stdin_write, input, (DWORD)strlen(input), &written,
> NULL);
>
> As above..

Again, you should observe the broader context of the code. The `input` buffer is allocated on the heap and its size was calculated as DWORD. (Although, we could research here, how the size calculation behaves for strings longer than 4G.) Anyway, its size will always be <=MAXDWORD. According to the given parameters, the WideCharToMultiByte() MUST zero-terminate the `input` buffer. Therefore, strlen(input) is guaranteed to be <MAXDWORD. Therefore, casting from size_t to DWORD is perfectly safe.

Best regards,
Simon

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
  

Hi,


> >     const char *p = _tcsrchr(find->cFileName, TEXT('.'));
> >     return p && p != find->cFileName && !_tcsicmp(p+1, ext);
> >
> > No _tcslen() and no offending size variables.
> > And that convoluted original is then gone.
>
> The resulting code would look a lot nicer indeed. Stay tuned for v2 of
> this patch.
>
>
Yes that was the point -- not that we cant do arithmetic, but we need not
and in the end the code could be cleaner...


>
> > -    WritePipeAsync(pipe, buf, wcslen(buf) * 2, count, events);
> > +    WritePipeAsync(pipe, buf, (DWORD)(wcslen(buf) * 2), count, events);
> >
> > Isn't this identical to the original automatic conversion from size_t to
> > DWORD?
> > An overly paranoid assert(_countof(buf) < MAXDWORD/2) may catch an
> > extremely
> > unlikely future coding error, but in what way a cast safer?
>
> I didn't say it is safer. The buf is WCHAR[33] on the stack. There is a
> safety zero termination a line before the WritePipeAsync(). Therefore,
> wcslen(buf) will always return between 0 and 32. Times two (if I was coding
> this, I'd use *sizeof(WCHAR) instead of 2, to make a clear statement, what
> is multiplication by 2 about), making it between 0 and 64. Unfortunately,
> result is size_t where Win32 API expects a DWORD.
>
> This will always produce a false compiler warning. I muted it by an
> explicit cast, so we can focus on more important warnings.


Agreed, reducing compiler warnings is a useful side effect and we know the
type coercion here is safe.

cheers,

Selva
<div dir="ltr">Hi,<div><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
&gt;     const char *p = _tcsrchr(find-&gt;cFileName, TEXT(&#39;.&#39;));<br>
&gt;     return p &amp;&amp; p != find-&gt;cFileName &amp;&amp; !_tcsicmp(p+1, ext);<br>
&gt;<br>
&gt; No _tcslen() and no offending size variables.<br>
</span><span class="">&gt; And that convoluted original is then gone.<br>
<br>
</span>The resulting code would look a lot nicer indeed. Stay tuned for v2 of this patch.<br>
<span class=""><br></span></blockquote><div><br></div><div>Yes that was the point -- not that we cant do arithmetic, but we need not</div><div>and in the end the code could be cleaner...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
&gt; -    WritePipeAsync(pipe, buf, wcslen(buf) * 2, count, events);<br>
&gt; +    WritePipeAsync(pipe, buf, (DWORD)(wcslen(buf) * 2), count, events);<br>
&gt;<br>
&gt; Isn&#39;t this identical to the original automatic conversion from size_t to<br>
&gt; DWORD?<br>
&gt; An overly paranoid assert(_countof(buf) &lt; MAXDWORD/2) may catch an<br>
&gt; extremely<br>
&gt; unlikely future coding error, but in what way a cast safer?<br>
<br>
</span>I didn&#39;t say it is safer. The buf is WCHAR[33] on the stack. There is a safety zero termination a line before the WritePipeAsync(). Therefore, wcslen(buf) will always return between 0 and 32. Times two (if I was coding this, I&#39;d use *sizeof(WCHAR) instead of 2, to make a clear statement, what is multiplication by 2 about), making it between 0 and 64. Unfortunately, result is size_t where Win32 API expects a DWORD.<br>
<br>
This will always produce a false compiler warning. I muted it by an explicit cast, so we can focus on more important warnings.</blockquote><div><br></div><div>Agreed, reducing compiler warnings is a useful side effect and we know the</div><div>type coercion here is safe.</div><div><br></div><div>cheers,</div><div><br></div><div>Selva</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