snwprintf() => _snwprintf()

From a technical point of view, it looks like a fine patch. But it is hard for me
(as a non-Windows dev) to understand *why* this is needed.
It would be good to explain the rationale for a change so others can understand it as quickly as possible.

I won't dive deep into how to write good commit messages here, as this blog post covers it very well: <https://chris.beams.io/posts/git-commit/>

Hi,

Quoted 1 line

From a technical point of view, it looks like a fine patch. But it is

hard

Quoted 4 lines

for me
(as a non-Windows dev) to understand *why* this is needed. It would be good to explain the rationale for a change so others can understand it as quickly as possible.

snwprintf() is not defined in Windows SDK. _snwprintf() is.
[https://docs.microsoft.com/en-us/cpp/c-runtime-library/
reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l]

Mingw probably accepts both. That's why nobody noticed. Looking around OpenVPN
code, I have found some code using _snwprintf(), some snwprintf(). Probably,
since MSVC was neglected for so long, nobody paid attention to such a subtle
difference.

For the code to compile on MSVC all snwprintf() should be replaced with _snwprintf(). Or, since the prefix underscore imposed by Windows SDK is not mostly aesthetical, the following #define could be introduced:
#ifdef _MSVC_VER
#define snwprintf _snwprintf
#endif

Nevertheless, I suggest consistent use of snwprintf/_snwprintf.

From: Simon Rozman <[email protected]...>

Note: NETIOAPI_API is defined as:
#define NETIOAPI_API NETIO_STATUS NETIOAPI_API_
#define NETIOAPI_API_ WINAPI

I am not sure whether interactive.c will compile using mingw with this patch. If not:
1. We can introduce some

   #ifdef _MSC_VER
   #else
   #endif

2. Since NTDDI_VERSION=NTDDI_VISTA, most of the dynamic API loading by GetProcAddress() could be simplified to statically linked API calls, avoiding function prototype declarations altogether. I suggest we do this anyway to clean-up the code in the future.

From: Simon Rozman <[email protected]...>

---
 src/openvpnserv/interactive.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

src/openvpnserv/interactive.c +0 -0

index 9d473e4..4b08086 100644
@@ -370,12 +370,12 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir,
 const WCHAR *options)
     BOOL ret = FALSE;
     int i;
     const WCHAR *msg1 = L"You have specified a config file location (%s

relative to %s)"
- " that requires admin approval. This error may be
avoided"

src/openvpnserv/interactive.c +1 -1

-                        " by adding your account to the \"%s\" group";
+                        L" that requires admin approval. This error may

be avoided"
+ L" by adding your account to the \"%s\" group";

const WCHAR *msg2 = L"You have specified an option (%s) that may be
used"
- " only with admin approval. This error may be
avoided"

src/openvpnserv/interactive.c +1 -1

-                        " by adding your account to the \"%s\" group";
+                        L" only with admin approval. This error may be

avoided"
+ L" by adding your account to the \"%s\" group";

     argv = CommandLineToArgvW(options, &argc);

From a technical point of view, it looks like a fine patch. But it is hard for me
(as a non-Windows dev) to understand *why* this is needed.
It would be good to explain the rationale for a change so others can understand it as quickly as possible.

replacing GetProcAddr() with statically linked Vista+ API calls would break compatibility with Windows XP.

Hi,

I agree migration towards ISO-C's snwprintf() is better.

From: Simon Rozman <[email protected]...>

---
 src/openvpnserv/interactive.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

src/openvpnserv/interactive.c +5 -5

index 2019d38..7ff45b1 100644
@@ -1891,6 +1891,11 @@ FreeWaitHandles(LPHANDLE h)
     free(h);
 }
 
+static BOOL
+CmpHandle(LPVOID item, LPVOID hnd)
+{
+    return item == hnd;
+}
 
 VOID WINAPI
 ServiceStartInteractive(DWORD dwArgc, LPTSTR *lpszArgv)

@@ -1901,11 +1906,6 @@ ServiceStartInteractive(DWORD dwArgc, LPTSTR
 *lpszArgv)
     list_item_t *threads = NULL;
     PHANDLE handles = NULL;
     DWORD handle_count;
-    BOOL
-    CmpHandle(LPVOID item, LPVOID hnd)
-    {
-        return item == hnd;
-    }
 
     service = RegisterServiceCtrlHandlerEx(interactive_service.name,

ServiceCtrlInteractive, &status);
if (!service)

Do you still maintain 2.3 branch for Windows XP? You do know, that replacing GetProcAddr() with statically linked Vista+ API calls would break compatibility with Windows XP.

From: Simon Rozman <[email protected]...>

---
 src/openvpnserv/interactive.c | 1 +
 1 file changed, 1 insertion(+)

src/openvpnserv/interactive.c +0 -0

index 4b08086..2019d38 100644

From: Simon Rozman <[email protected]...>

1. The project file was created from scratch.
2. XML nodes were reordered to match old project order for easier diff-ing.
3. All non-essential settings reset to MSVC defaults.
4. .h/.c file list updated.
5. Missing /I include paths added.
6. List of required .lib files updated.

The openvpnserv.exe compiles now. Warnings will be addressed in future patches.

Though we do not have a patch allocation policy, as I have responded to 1 to 5 I might have become the de-facto reviewer of this patch set.

So this is just to say patches 6 to 13 (except 9 and 12) are VS project files and related stuff that I have no idea about and will be hard for me to review. Hope someone else take a look at those.

From: Simon Rozman <[email protected]...>

---
 src/openvpn/block_dns.c       | 2 +-
 src/openvpnserv/automatic.c   | 2 +-
 src/openvpnserv/common.c      | 2 +-
 src/openvpnserv/interactive.c | 4 ++--
 src/openvpnserv/validate.c    | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

src/openvpn/block_dns.c +0 -0

 /*

src/openvpnserv/automatic.c +0 -0

index 4123d0f..6c39aaa 100644

src/openvpnserv/validate.c IsUserInGroup(PSID sid, const PTOKEN_GROUPS

Quoted 4 lines

index 7a2d0e3..5422d33 100644

src/openvpnserv/validate.c +0 -0

@@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS

token_groups, const WCHAR *group_nam

src/openvpnserv/validate.c IsUserInGroup(PSID sid, const PTOKEN_GROUPS · line 300

Quoted 3 lines

src/openvpnserv/validate.c +0 -0

             break;
         }
         /* If a match is already found, ret == TRUE and the loop is

skipped */

src/openvpnserv/validate.c IsUserInGroup(PSID sid, const PTOKEN_GROUPS token_groups, const WCHAR *group_nam · line 301

Quoted 2 lines

src/openvpnserv/validate.c +1 -1

-        for (int i = 0; i < nread && !ret; ++i)
+        for (DWORD i = 0; i < nread && !ret; ++i)

... and here.

Consider me not convinced...

what exactly are the warnings MSVC spits out here?

src/openvpnserv/automatic.c match(const WIN32_FIND_DATA *find, LPCTSTR ext) · line 160

Quoted 4 lines

src/openvpnserv/automatic.c +1 -1

-modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext)
+modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext)
 {
     int i;

Not sure why this is needed? The code verifies that size is ">0", so a signed variable is ok here.

src/openvpn/block_dns.c get_interface_metric(const NET_IFINDEX index,

Quoted 5 lines

src/openvpn/block_dns.c index renamed from src/openvpn/block_dns.c +0 -0

d43cbcf..f88ba2c 100644

src/openvpn/block_dns.c +0 -0

@@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index,

const ADDRESS_FAMILY family)

src/openvpn/block_dns.c get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family) · line 374

Quoted 6 lines

src/openvpn/block_dns.c +1 -1

         }
         return ipiface.Metric;
     }
-    return -err;
+    return -(int)err;
 }	

This, I can somewhat agree to, as "err" is an unsigned type so there might be a hidden integer overflow if it happens to be "large". Which it won't be, but still.

src/openvpnserv/automatic.c match(const WIN32_FIND_DATA *find, LPCTSTR

Quoted 6 lines

src/openvpn/block_dns.c +0 -0

 /*

src/openvpnserv/automatic.c +0 -0

index 4123d0f..6c39aaa 100644
@@ -155,7 +155,7 @@ match(const WIN32_FIND_DATA *find, LPCTSTR

ext)

src/openvpnserv/automatic.c match(const WIN32_FIND_DATA *find, LPCTSTR ext) · line 160

Quoted 7 lines

src/openvpnserv/automatic.c +1 -1

  * Modify the extension on a filename.
  */
 static bool
-modext(LPTSTR dest, int size, LPCTSTR src, LPCTSTR newext)
+modext(LPTSTR dest, size_t size, LPCTSTR src, LPCTSTR newext)
 {
     int i;

Not sure why this is needed? The code verifies that size is ">0", so a signed variable is ok here.

src/openvpnserv/common.c

Quoted 1 line

src/openvpnserv/common.c +0 -0

index

src/openvpnserv/common.c openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR

Quoted 4 lines

b8b817b..7901fd6 100644

src/openvpnserv/common.c +0 -0

@@ -36,7 +36,7 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR

format, va_list arglist)

src/openvpnserv/common.c openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist) · line 40

Quoted 6 lines

src/openvpnserv/common.c +1 -1

         len = _vsntprintf(str, size, format, arglist);
         str[size - 1] = 0;
     }
-    return (len >= 0 && len < size);
+    return (len >= 0 && (size_t)len < size);
 }

This is, if I understand right, because "len < size" will implicitly cast size to (int), causing integer overflow if size is too big for a signed int?

Quoted 7 lines

src/openvpnserv/common.c +0 -1

 int
 openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...) diff
--git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c

index 8d94197..96e0de0 100644

src/openvpnserv/interactive.c +0 -0

@@ -188,7 +188,7 @@ typedef enum {

src/openvpnserv/interactive.c typedef enum { · line 191

Quoted 5 lines

src/openvpnserv/interactive.c +1 -1

 static DWORD
 AsyncPipeOp(async_op_t op, HANDLE pipe, LPVOID buffer, DWORD size,
 DWORD count, LPHANDLE events)  {	
-    int i;
+    DWORD i;

... and this totally escapes me. Why would an "int" suddenly be no longer a legal loop variable...?

src/openvpnserv/interactive.c RegisterDNS(LPVOID unused) · line 1064

Quoted 9 lines

src/openvpnserv/interactive.c +1 -1

     BOOL success;
     HANDLE io_event;
     DWORD res, bytes = 0;

@@ -1061,7 +1061,7 @@ RegisterDNS(LPVOID unused)
         { ipcfg, L"ipconfig /flushdns",    timeout },
         { ipcfg, L"ipconfig /registerdns", timeout },
     };
-    int ncmds = sizeof(cmds) / sizeof(cmds[0]);
+    DWORD ncmds = sizeof(cmds) / sizeof(cmds[0]);

Same thing here...

src/openvpnserv/validate.c IsUserInGroup(PSID sid, const PTOKEN_GROUPS

Quoted 4 lines

index 7a2d0e3..5422d33 100644

src/openvpnserv/validate.c +0 -0

@@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS

token_groups, const WCHAR *group_nam

src/openvpnserv/validate.c IsUserInGroup(PSID sid, const PTOKEN_GROUPS token_groups, const WCHAR *group_nam · line 301

Quoted 5 lines

src/openvpnserv/validate.c +1 -1

             break;
         }
         /* If a match is already found, ret == TRUE and the loop is skipped */
-        for (int i = 0; i < nread && !ret; ++i)
+        for (DWORD i = 0; i < nread && !ret; ++i)

... and here.

Consider me not convinced...

what exactly are the warnings MSVC spits out here?

Hi,

Let me explain all proposed modifications case-by-case below...

Quoted 14 lines

src/openvpn/block_dns.c get_interface_metric(const NET_IFINDEX index,

Quoted 5 lines

src/openvpn/block_dns.c index renamed from src/openvpn/block_dns.c +0 -0

d43cbcf..f88ba2c 100644

src/openvpn/block_dns.c +0 -0

@@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index,

const ADDRESS_FAMILY family)

src/openvpn/block_dns.c get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family) · line 374

Quoted 6 lines

src/openvpn/block_dns.c +1 -1

         }
         return ipiface.Metric;
     }
-    return -err;
+    return -(int)err;
 }

This, I can somewhat agree to, as "err" is an unsigned type so there

Quoted 1 line

a hidden integer overflow if it happens to be "large". Which it won't

Quoted 1 line

still.

Adding the (int) resolves the warning C4146: unary minus operator applied to unsigned type, result still unsigned.
https://msdn.microsoft.com/en-us/library/4kh09110.aspx

It doesn't change much, but it shut-ups one compiler warning.

does represent a potential threat. But you don't know which one that is. Therefore it is best to address them all. :)

Quoted 17 lines

Quoted 13 lines

src/openvpn/block_dns.c get_interface_metric(const NET_IFINDEX index,

Quoted 5 lines

src/openvpn/block_dns.c +0 -0

index d43cbcf..f88ba2c 100644
@@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index,

const ADDRESS_FAMILY family)

src/openvpn/block_dns.c get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family) · line 374

Quoted 6 lines

src/openvpn/block_dns.c +1 -1

          }
          return ipiface.Metric;
      }
-    return -err;
+    return -(int)err;
  }

Quoted 3 lines

This, I can somewhat agree to, as "err" is an unsigned type so there might be a hidden integer overflow if it happens to be "large". Which it won't be, but still.

Quoted 5 lines

Adding the (int) resolves the warning C4146: unary minus operator applied to unsigned type, result still unsigned.
https://msdn.microsoft.com/en-us/library/4kh09110.aspx

It doesn't change much, but it shut-ups one compiler warning.

The original code could be argued to be logically wrong (though perfectly legal C) if err becomes larger than a signed int. This change does not fix that. So what's the point?

As an example, consider this

err = 2147483649 (2 more than INT_MAX)
Original will return  2147483647 (not a -ve int) The new code will return the same 2147483647 That is, the return value is +ve int which the caller will wrongly interpret as valid result.

This is hypothetical as Windows system err codes do not get that large. But then the original is as good as the replacement except for a C++-trained compiler being silenced.

To repeat,  cast does not eliminate a potential for error, it just just hides it.

Quoted 1 line

To repeat,  cast does not eliminate a potential for error, it just just hides it.

True and I totally agree with you. I just didn't dare to rewrite the function more thoroughly. Can I, please?

Hi,

Quoted 24 lines

Quoted 17 lines

Quoted 13 lines

src/openvpn/block_dns.c get_interface_metric(const NET_IFINDEX index,

Quoted 5 lines

src/openvpn/block_dns.c +0 -0

index d43cbcf..f88ba2c 100644
@@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index,

const ADDRESS_FAMILY family)

src/openvpn/block_dns.c get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family) · line 374

Quoted 6 lines

src/openvpn/block_dns.c +1 -1

         }
         return ipiface.Metric;
     }
-    return -err;
+    return -(int)err;
 }

Quoted 3 lines

This, I can somewhat agree to, as "err" is an unsigned type so there might be a hidden integer overflow if it happens to be "large". Which it won't be, but still.

Quoted 5 lines

Adding the (int) resolves the warning C4146: unary minus operator applied to unsigned type, result still unsigned.
https://msdn.microsoft.com/en-us/library/4kh09110.aspx

It doesn't change much, but it shut-ups one compiler warning.

The original code could be argued to be logically wrong (though

Quoted 1 line

legal C) if err becomes larger than a signed int. This change does not

Quoted 6 lines

So what's the point?

As an example, consider this

err = 2147483649 <(214)%20748-3649> (2 more than INT_MAX) Original will return 2147483647 <(214)%20748-3647> (not a -ve int) The

Quoted 1 line

same 2147483647 <(214)%20748-3647> That is, the return value is +ve int

Quoted 5 lines

wrongly interpret as valid result.

This is hypothetical as Windows system err codes do not get that large. But then the original is as good as the replacement except for a C++-trained compiler being silenced.

I totally agree with your point, the original code might be logically

The get_interface_metric()also return error codes > INT_MAX as a positive

Quoted 2 lines

To repeat, cast does not eliminate a potential for error, it just just

True and I totally agree with you. I just didn't dare to rewrite the

Option 1: Add an if to limit metric to INT_MAX, and an if to return

Option 2: redefine function prototype to return error code unmodified,

I'd prefer option 1 after reviewing all get_interface_metrics() calls.

The best time to re-factor a function would be when a a new use case needs to change its semantics. Apart from the ill-chosen -err as a return value, currently it returns 0 if automatic metric is in use, making it impossible to use it as a generic function to find the current metric of an interface.

In fact I've a pending patch where such a change would help.

So, if you re-write this, consider some improvements: I think it belongs to win32.c (declaration in win32.h). Second, return the current metric even if Automatic metric is on (could indicate automatic metric needed in current use case by a boolean parameter, probably). In case of error, logging it and returning -1 should be good enough.

Based on my tests, Windows does not permit setting a metric larger than INT_MAX, so a return value of signed int is fine and convenient for indicating error.

From: Simon Rozman <[email protected]...>

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

src/openvpnserv/automatic.c +0 -0

index 6c39aaa..3e8f6ed 100644

src/openvpnserv/automatic.c +2 -2

-    if (!_tcslen(ext))
+    len_ext = _tcslen(ext);
+    if (!len_ext)
     {
         return true;
     }
 
-    i = _tcslen(find->cFileName) - _tcslen(ext) - 1;

src/openvpnserv/interactive.c +0 -0

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

src/openvpnserv/interactive.c +1 -1

-    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

src/openvpnserv/interactive.c +0 -0

     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 */

src/openvpnserv/interactive.c +1 -1

-    int ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 + 1;
+    size_t ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 +

1;

         free(input);
     }

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:

    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.

(quoted diff) +1 -1

-    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?

(quoted diff) +1 -1

-    WritePipeAsync(pipe, result, wcslen(result) * 2, count, events);
+    WritePipeAsync(pipe, result, (DWORD)(wcslen(result) * 2), count,

events);

Same here.

(quoted diff) +1 -1

-        WriteFile(stdin_write, input, strlen(input), &written, NULL);
+        WriteFile(stdin_write, input, (DWORD)strlen(input), &written,

NULL);

As above..

Quoted 5 lines

    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.

Quoted 9 lines

(quoted diff) +1 -1

-    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.

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   | 20 ++++----------------
 src/openvpnserv/interactive.c | 12 ++++++------
 2 files changed, 10 insertions(+), 22 deletions(-)

src/openvpnserv/automatic.c +0 -7

index 75c3be2..4d5501b 100644
@@ -115,25 +115,13 @@ 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))
-    {
-        return true;
-    }
-

Hi,

Thanks for the v2

Quoted 29 lines · Simon Rozman

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   | 20 ++++----------------
 src/openvpnserv/interactive.c | 12 ++++++------
 2 files changed, 10 insertions(+), 22 deletions(-)

src/openvpnserv/automatic.c +0 -7

index 75c3be2..4d5501b 100644
@@ -115,25 +115,13 @@ 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))
-    {
-        return true;
-    }
-

src/openvpnserv/automatic.c close_if_open(HANDLE h) · line 125

Quoted 10 lines

src/openvpnserv/automatic.c +2 -7

-    i = _tcslen(find->cFileName) - _tcslen(ext) - 1;
-    if (i < 1)
-    {
-        return false;
-    }
-
-    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;

FWIW, ensuring the extension string is not empty seems to be required to avoid a regression (see my reply to v1). A check for ext[0] != TEXT('\0') should do the trick.

The rest looks good.

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   | 20 ++++----------------
 src/openvpnserv/interactive.c | 12 ++++++------
 2 files changed, 10 insertions(+), 22 deletions(-)

src/openvpnserv/automatic.c +2 -14

index 75c3be2..4d5501b 100644
@@ -115,25 +115,13 @@ 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))
-    {
-        return true;
-    }
-
-    i = _tcslen(find->cFileName) - _tcslen(ext) - 1;
-    if (i < 1)
-    {
-        return false;
-    }
-
-    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;
 }
 

/*

src/openvpnserv/automatic.c +2 -2

@@ -142,14 +130,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('\\'))
             {

src/openvpnserv/interactive.c +1 -1

index ed4603e..5d58ceb 100644
@@ -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

src/openvpnserv/interactive.c +0 -0

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

src/openvpnserv/interactive.c +1 -1

-    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

src/openvpnserv/interactive.c +2 -2

@@ -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 */

src/openvpnserv/interactive.c +1 -1

-    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)
     {

src/openvpnserv/interactive.c +1 -1

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

     }

--
2.9.0.windows.1

Hi Selva,

I have git-send-email v3 of this patch on November 13th but it got lost somewhere along the way. :( I can see it in the SMTP log, but didn't notice it hasn't appeared on the mailing list.

Here you go again. I hope this time it makes it thru.

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
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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(-)

src/openvpnserv/automatic.c +0 -1

index 75c3be2..6f82b4e 100644
@@ -115,25 +115,20 @@ close_if_open(HANDLE h)
 static bool
 match(const WIN32_FIND_DATA *find, LPCTSTR ext)
 {
-    int i;

src/openvpnserv/automatic.c +1 -5

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

src/openvpnserv/automatic.c +4 -2

+    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('\\'))
             {

src/openvpnserv/interactive.c +1 -1

index ed4603e..5d58ceb 100644
@@ -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

src/openvpnserv/interactive.c +0 -0

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

src/openvpnserv/interactive.c +1 -1

-    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

src/openvpnserv/interactive.c +2 -2

@@ -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 */

src/openvpnserv/interactive.c +1 -1

-    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)
     {

src/openvpnserv/interactive.c +1 -1

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

     }

Quoted 6 lines

The best time to re-factor a function would be when a a new use case needs to change its semantics. Apart from the ill-chosen -err as a return value, currently it returns 0 if automatic metric is in use, making it impossible to use it as a generic function to find the current metric of an interface.

In fact I've a pending patch where such a change would help.

I shall give it a look after the Hackathon.

Quoted 4 lines

Quoted 4 lines

The best time to re-factor a function would be when a a new use case needs to change its semantics. Apart from the ill-chosen -err as a return value, currently it returns 0 if automatic metric is in use, making it impossible to use it as a generic function to find the current

metric of an interface.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

In fact I've a pending patch where such a change would help.

[..]

Quoted 1 line

I shall give it a look after the Hackathon.

"ping"?

This seems to be the only hunk left from the MSVC correction series (as far as .c/.h files are concerned, not .proj)

Hi,

Quoted 13 lines · Simon Rozman

Quoted 4 lines

Quoted 4 lines

The best time to re-factor a function would be when a a new use case needs to change its semantics. Apart from the ill-chosen -err as a return value, currently it returns 0 if automatic metric is in use, making it impossible to use it as a generic function to find the current

metric of an interface.

Quoted 2 lines

Quoted 2 lines

In fact I've a pending patch where such a change would help.

[..]

Quoted 1 line

I shall give it a look after the Hackathon.

"ping"?

Quoted 3 lines

This seems to be the only hunk left from the MSVC correction series (as far as .c/.h files are concerned, not .proj)

I really appreciate your "ping" Gert. I totally forgot about this one and have now flagged this thread so I shall finish it in the following weeks.

The get_interface_metric() function should get a more thorough rewrite than just a compiler warning shut-up. So the patch will probably get divided in two - the simple signed/unsigned fixes and get_interface_metric() redesign.

Quoted 1 line

The get_interface_metric() function should get a more thorough rewrite

than just a compiler warning shut-up. So the patch will probably get divided in two - the simple signed/unsigned fixes and get_interface_metric() redesign.

For the latter, I had "re-invented" the get_interface_metric function for the pending "use lowest metric interface when multiple interfaces match a route" patch.

Obviously, its better to refactor this one and use it there, so I just copied the implementation and submitted a patch to do so. Unlike I had thought earlier, this has to stay in block_dns.h for ease of sharing with the service.

Could you please take a look and see whether it addresses MSVC's concerns among other things?

Hi,

Quoted 13 lines

Quoted 1 line

The get_interface_metric() function should get a more thorough rewrite

than just a compiler warning shut-up. So the patch will probably get divided in two - the simple signed/unsigned fixes and get_interface_metric() redesign.

For the latter, I had "re-invented" the get_interface_metric function for the pending "use lowest metric interface when multiple interfaces match a route" patch.

Obviously, its better to refactor this one and use it there, so I just copied the implementation and submitted a patch to do so. Unlike I had thought earlier, this has to stay in block_dns.h for ease of sharing with the service.

Quoted 2 lines

Could you please take a look and see whether it addresses MSVC's concerns among other things?

I have tested and am confirming MSVC is happy with the block_dns.c now.

Should your patch be merged, I shall rebase the "[PATCH 09/13] Signed/unsigned warnings of MSVC resolved" to the new master and deliver the next version.

Quoted 1 line

Should your patch be merged, I shall rebase the "[PATCH 09/13]

Signed/unsigned warnings of MSVC resolved" to the new master and deliver the next version.

Yes, if you can review and ack/nak it :)

Quoted 5 lines

Quoted 1 line

Should your patch be merged, I shall rebase the "[PATCH 09/13]

Signed/unsigned warnings of MSVC resolved" to the new master and deliver the next version.

Yes, if you can review and ack/nak it :)

You mean Gert to ack/nak it? I don't believe I have earned enough reputation to do it yet.

However, I did stare-review your code:
- It does not introduce any new Windows API calls it has not used before.
- It compiles fine.

However, I did stare-review your code:
- It does not introduce any new Windows API calls it has not used before.
- It compiles fine.

Quoted 3 lines · Simon Rozman

However, I did stare-review your code:
- It does not introduce any new Windows API calls it has not used before.
- It compiles fine.

It also builds fine here with cl v19.11.
But using clang-cl v5, I'm getting:
In file included from src/openvpn/argv.c:36:
./src/openvpn/syshead.h(368,13) : error: typedef redefinition with
different types ('int' vs 'enum MIB_TCP_STATE')

  typedef int MIB_TCP_STATE;
              ^
  f:\ProgramFiler-x86\WindowsKits\Include\10.0.15063.0\shared\tcpmib.h(56,3)
:  note: previous definition is here
  } MIB_TCP_STATE;
    ^

1 error generated.

I fail to why I don't get this error with MSVC. But the lines in syshead.h should simply not assume MinGW is the only compiler on Windows. Line 365:

#ifdef _WIN32
/* Missing declarations for old-school MinGW32 or MinGW-w64 v1.x.
*/
#if defined(__MINGW32__) && !(defined(__MINGW64_VERSION_MAJOR) &&
__MINGW64_VERSION_MAJOR < 2)
typedef int MIB_TCP_STATE;
#endif

Quoted 4 lines

Quoted 1 line

The get_interface_metric() function should get a more thorough rewrite

than just a compiler warning shut-up. So the patch will probably get divided in two - the simple signed/unsigned fixes and get_interface_metric() redesign.

I have tested and am confirming MSVC is happy with the block_dns.c now.

Should your patch be merged, I shall rebase the "[PATCH 09/13] Signed/unsigned warnings of MSVC resolved" to the new master and deliver the next version.

Quoted 3 lines

I have tested and am confirming MSVC is happy with the block_dns.c now.

Should your patch be merged, I shall rebase the "[PATCH 09/13]

Signed/unsigned warnings of MSVC resolved" to the new master and deliver the next version.

I finally came around to merging the get_interface_metric() patch from

today, so I await your fixup patch of the remaining MSVC warnings :-)

thanks,

gert

Hi,

Quoted 7 lines

Quoted 3 lines

I have tested and am confirming MSVC is happy with the block_dns.c now.

Should your patch be merged, I shall rebase the "[PATCH 09/13]

Signed/unsigned warnings of MSVC resolved" to the new master and deliver the next version.

I finally came around to merging the get_interface_metric() patch from

Selva

Quoted 5 lines

today, so I await your fixup patch of the remaining MSVC warnings :-)

thanks,

gert

I have been insanely busy for the last few months and the OpenVPN contribution was one of the sacrifices I had to make. Please apologize for the inconvenience.

After I get back to my-old-self, count my children and see if my wife still recognizes me, I shall address all the OpenVPN things. I have quite some messages flagged for a follow-up here. Including this thread.