[Openvpn-devel] [PATCH 01/13] snwprintf() => _snwprintf()

[Openvpn-devel] [PATCH 01/13] snwprintf() => _snwprintf()

[simon]

From: Simon Rozman <simon@...2293...>

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

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index fe9c7d6..9d473e4 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -402,8 +402,8 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
 
         if (!CheckOption(workdir, 2, argv_tmp, &settings))
         {
-            snwprintf(buf, _countof(buf), msg1, argv[0], workdir,
-                      settings.ovpn_admin_group);
+            _snwprintf(buf, _countof(buf), msg1, argv[0], workdir,
+                       settings.ovpn_admin_group);
             buf[_countof(buf) - 1] = L'\0';
             ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
         }
@@ -421,15 +421,15 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options)
         {
             if (wcscmp(L"--config", argv[i]) == 0 && argc-i > 1)
             {
-                snwprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
-                          settings.ovpn_admin_group);
+                _snwprintf(buf, _countof(buf), msg1, argv[i+1], workdir,
+                           settings.ovpn_admin_group);
                 buf[_countof(buf) - 1] = L'\0';
                 ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
             }
             else
             {
-                snwprintf(buf, _countof(buf), msg2, argv[i],
-                          settings.ovpn_admin_group);
+                _snwprintf(buf, _countof(buf), msg2, argv[i],
+                           settings.ovpn_admin_group);
                 buf[_countof(buf) - 1] = L'\0';
                 ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event);
             }
diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c
index f6a97e9..7a2d0e3 100644
--- a/src/openvpnserv/validate.c
+++ b/src/openvpnserv/validate.c
@@ -65,7 +65,7 @@ CheckConfigPath(const WCHAR *workdir, const WCHAR *fname, const settings_t *s)
     /* convert fname to full path */
     if (PathIsRelativeW(fname) )
     {
-        snwprintf(tmp, _countof(tmp), L"%s\\%s", workdir, fname);
+        _snwprintf(tmp, _countof(tmp), L"%s\\%s", workdir, fname);
         tmp[_countof(tmp)-1] = L'\0';
         config_file = tmp;
     }
-- 
2.9.0.windows.1

[Openvpn-devel] [PATCH 02/13] Mixing wide and regular strings in concatenations is not allowed in MSVC.

[simon]

From: Simon Rozman <simon@...2293...>

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

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 9d473e4..4b08086 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -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"
-                        " 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"
-                        " 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);
 
-- 
2.9.0.windows.1

[Openvpn-devel] [PATCH 03/13] RtlIpv6AddressToStringW() and RtlIpv4AddressToStringW() require mstcpip.h

[simon]

From: Simon Rozman <simon@...2293...>

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

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 4b08086..2019d38 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -33,6 +33,7 @@
 #include <stdio.h>
 #include <sddl.h>
 #include <shellapi.h>
+#include <mstcpip.h>
 
 #ifdef HAVE_VERSIONHELPERS_H
 #include <versionhelpers.h>
-- 
2.9.0.windows.1

[Openvpn-devel] [PATCH 04/13] Local functions are not supported in MSVC. Bummer.

[simon]

From: Simon Rozman <simon@...2293...>

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

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 2019d38..7ff45b1 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -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)
-- 
2.9.0.windows.1

[Openvpn-devel] [PATCH 05/13] Function prototypes are declared as "typedef <return type> (<calling convention> *type_name)(<arguments>)" in MSVC.

[simon]

From: Simon Rozman <simon@...2293...>

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.
---
 src/openvpnserv/interactive.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 7ff45b1..8d94197 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -550,7 +550,7 @@ InterfaceLuid(const char *iface_name, PNET_LUID luid)
     LPWSTR wide_name;
     int n;
 
-    typedef NETIO_STATUS WINAPI (*ConvertInterfaceAliasToLuidFn) (LPCWSTR, PNET_LUID);
+    typedef NETIO_STATUS (WINAPI *ConvertInterfaceAliasToLuidFn) (LPCWSTR, PNET_LUID);
     static ConvertInterfaceAliasToLuidFn ConvertInterfaceAliasToLuid = NULL;
     if (!ConvertInterfaceAliasToLuid)
     {
@@ -585,7 +585,7 @@ CmpAddress(LPVOID item, LPVOID address)
 static DWORD
 DeleteAddress(PMIB_UNICASTIPADDRESS_ROW addr_row)
 {
-    typedef NETIOAPI_API (*DeleteUnicastIpAddressEntryFn) (const PMIB_UNICASTIPADDRESS_ROW);
+    typedef NETIO_STATUS (NETIOAPI_API_ *DeleteUnicastIpAddressEntryFn) (const PMIB_UNICASTIPADDRESS_ROW);
     static DeleteUnicastIpAddressEntryFn DeleteUnicastIpAddressEntry = NULL;
 
     if (!DeleteUnicastIpAddressEntry)
@@ -613,8 +613,8 @@ HandleAddressMessage(address_message_t *msg, undo_lists_t *lists)
     PMIB_UNICASTIPADDRESS_ROW addr_row;
     BOOL add = msg->header.type == msg_add_address;
 
-    typedef NETIOAPI_API (*CreateUnicastIpAddressEntryFn) (const PMIB_UNICASTIPADDRESS_ROW);
-    typedef NETIOAPI_API (*InitializeUnicastIpAddressEntryFn) (PMIB_UNICASTIPADDRESS_ROW);
+    typedef NETIO_STATUS (NETIOAPI_API_ *CreateUnicastIpAddressEntryFn) (const PMIB_UNICASTIPADDRESS_ROW);
+    typedef NETIO_STATUS (NETIOAPI_API_ *InitializeUnicastIpAddressEntryFn) (PMIB_UNICASTIPADDRESS_ROW);
     static CreateUnicastIpAddressEntryFn CreateUnicastIpAddressEntry = NULL;
     static InitializeUnicastIpAddressEntryFn InitializeUnicastIpAddressEntry = NULL;
 
@@ -707,7 +707,7 @@ CmpRoute(LPVOID item, LPVOID route)
 static DWORD
 DeleteRoute(PMIB_IPFORWARD_ROW2 fwd_row)
 {
-    typedef NETIOAPI_API (*DeleteIpForwardEntry2Fn) (PMIB_IPFORWARD_ROW2);
+    typedef NETIO_STATUS (NETIOAPI_API_ *DeleteIpForwardEntry2Fn) (PMIB_IPFORWARD_ROW2);
     static DeleteIpForwardEntry2Fn DeleteIpForwardEntry2 = NULL;
 
     if (!DeleteIpForwardEntry2)
@@ -735,7 +735,7 @@ HandleRouteMessage(route_message_t *msg, undo_lists_t *lists)
     PMIB_IPFORWARD_ROW2 fwd_row;
     BOOL add = msg->header.type == msg_add_route;
 
-    typedef NETIOAPI_API (*CreateIpForwardEntry2Fn) (PMIB_IPFORWARD_ROW2);
+    typedef NETIO_STATUS (NETIOAPI_API_ *CreateIpForwardEntry2Fn) (PMIB_IPFORWARD_ROW2);
     static CreateIpForwardEntry2Fn CreateIpForwardEntry2 = NULL;
 
     if (!CreateIpForwardEntry2)
@@ -821,7 +821,7 @@ out:
 static DWORD
 HandleFlushNeighborsMessage(flush_neighbors_message_t *msg)
 {
-    typedef NETIOAPI_API (*FlushIpNetTable2Fn) (ADDRESS_FAMILY, NET_IFINDEX);
+    typedef NETIO_STATUS (NETIOAPI_API_ *FlushIpNetTable2Fn) (ADDRESS_FAMILY, NET_IFINDEX);
     static FlushIpNetTable2Fn flush_fn = NULL;
 
     if (msg->family == AF_INET)
-- 
2.9.0.windows.1

[Openvpn-devel] [PATCH 06/13] openvpnserv.vcxproj project file recreated using Visual Studio 2017

[simon]

From: Simon Rozman <simon@...2293...>

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.
---
 openvpn.sln                                 |  6 ++--
 src/openvpnserv/openvpnserv.vcxproj         | 52 +++++++++++++++--------------
 src/openvpnserv/openvpnserv.vcxproj.filters | 34 ++++++++++++++-----
 3 files changed, 57 insertions(+), 35 deletions(-)

diff --git a/openvpn.sln b/openvpn.sln
index 90c01b8..9f50f11 100644
--- a/openvpn.sln
+++ b/openvpn.sln
@@ -1,6 +1,8 @@
 
-Microsoft Visual Studio Solution File, Format Version 11.00
-# Visual C++ Express 2010
+Microsoft Visual Studio Solution File, Format Version 12.00
+# Visual Studio 15
+VisualStudioVersion = 15.0.26430.16
+MinimumVisualStudioVersion = 10.0.40219.1
 Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "openvpnserv", "src\openvpnserv\openvpnserv.vcxproj", "{9C91EE0B-817D-420A-A1E6-15A5A9D98BAD}"
 EndProject
 Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "openvpn", "src\openvpn\openvpn.vcxproj", "{29DF226E-4D4E-440F-ADAF-5829CFD4CA94}"
diff --git a/src/openvpnserv/openvpnserv.vcxproj b/src/openvpnserv/openvpnserv.vcxproj
index c6760da..a865bf0 100644
--- a/src/openvpnserv/openvpnserv.vcxproj
+++ b/src/openvpnserv/openvpnserv.vcxproj
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="utf-8"?>
-<Project DefaultTargets="Build" ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+<Project DefaultTargets="Build" ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <ItemGroup Label="ProjectConfigurations">
     <ProjectConfiguration Include="Debug|Win32">
       <Configuration>Debug</Configuration>
@@ -11,6 +11,7 @@
     </ProjectConfiguration>
   </ItemGroup>
   <PropertyGroup Label="Globals">
+    <VCProjectVersion>15.0</VCProjectVersion>
     <ProjectGuid>{9C91EE0B-817D-420A-A1E6-15A5A9D98BAD}</ProjectGuid>
     <RootNamespace>openvpnserv</RootNamespace>
     <Keyword>Win32Proj</Keyword>
@@ -18,18 +19,20 @@
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.Default.props" />
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="Configuration">
     <ConfigurationType>Application</ConfigurationType>
-    <CharacterSet>MultiByte</CharacterSet>
+    <CharacterSet>Unicode</CharacterSet>
     <WholeProgramOptimization>true</WholeProgramOptimization>
-    <PlatformToolset>v120</PlatformToolset>
+    <PlatformToolset>v141</PlatformToolset>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'" Label="Configuration">
     <ConfigurationType>Application</ConfigurationType>
-    <CharacterSet>MultiByte</CharacterSet>
-    <PlatformToolset>v120</PlatformToolset>
+    <CharacterSet>Unicode</CharacterSet>
+    <PlatformToolset>v141</PlatformToolset>
   </PropertyGroup>
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.props" />
   <ImportGroup Label="ExtensionSettings">
   </ImportGroup>
+  <ImportGroup Label="Shared">
+  </ImportGroup>
   <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="PropertySheets">
     <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
   </ImportGroup>
@@ -37,67 +40,66 @@
     <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
   </ImportGroup>
   <PropertyGroup Label="UserMacros" />
-  <PropertyGroup>
-    <_ProjectFileVersion>10.0.30319.1</_ProjectFileVersion>
-    <OutDir Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
-    <IntDir Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">$(Configuration)\</IntDir>
-    <LinkIncremental Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">true</LinkIncremental>
-    <OutDir Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
-    <IntDir Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(Configuration)\</IntDir>
-    <LinkIncremental Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">false</LinkIncremental>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
+    <OutDir>$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
+    <IntDir>$(Configuration)\</IntDir>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
+    <OutDir>$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
+    <IntDir>$(Configuration)\</IntDir>
   </PropertyGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
     <ClCompile>
       <Optimization>Disabled</Optimization>
-      <AdditionalIncludeDirectories>$(SOURCEBASE);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <AdditionalIncludeDirectories>$(SOURCEBASE);$(SOURCEBASE)\include;$(SOURCEBASE)\src\openvpn;$(SOURCEBASE)\src\compat;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
       <PreprocessorDefinitions>WIN32;_DEBUG;_CONSOLE;$(CPPFLAGS);%(PreprocessorDefinitions)</PreprocessorDefinitions>
-      <MinimalRebuild>true</MinimalRebuild>
-      <BasicRuntimeChecks>EnableFastChecks</BasicRuntimeChecks>
       <RuntimeLibrary>MultiThreadedDebugDLL</RuntimeLibrary>
       <PrecompiledHeader>
       </PrecompiledHeader>
       <WarningLevel>Level3</WarningLevel>
-      <DebugInformationFormat>EditAndContinue</DebugInformationFormat>
     </ClCompile>
     <ResourceCompile>
       <AdditionalIncludeDirectories>$(SOURCEBASE);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
     </ResourceCompile>
     <Link>
-      <GenerateDebugInformation>true</GenerateDebugInformation>
       <SubSystem>Console</SubSystem>
       <TargetMachine>MachineX86</TargetMachine>
+      <AdditionalDependencies>fwpuclnt.lib;ntdll.lib;shlwapi.lib;netapi32.lib;userenv.lib;iphlpapi.lib;%(AdditionalDependencies)</AdditionalDependencies>
     </Link>
   </ItemDefinitionGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
     <ClCompile>
       <Optimization>MaxSpeed</Optimization>
-      <IntrinsicFunctions>true</IntrinsicFunctions>
-      <AdditionalIncludeDirectories>$(SOURCEBASE);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <AdditionalIncludeDirectories>$(SOURCEBASE);$(SOURCEBASE)\include;$(SOURCEBASE)\src\openvpn;$(SOURCEBASE)\src\compat;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
       <PreprocessorDefinitions>WIN32;NDEBUG;_CONSOLE;$(CPPFLAGS);%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <RuntimeLibrary>MultiThreadedDLL</RuntimeLibrary>
-      <FunctionLevelLinking>true</FunctionLevelLinking>
       <PrecompiledHeader>
       </PrecompiledHeader>
       <WarningLevel>Level3</WarningLevel>
-      <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>
     </ClCompile>
     <ResourceCompile>
       <AdditionalIncludeDirectories>$(SOURCEBASE);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
     </ResourceCompile>
     <Link>
-      <GenerateDebugInformation>true</GenerateDebugInformation>
       <SubSystem>Console</SubSystem>
       <OptimizeReferences>true</OptimizeReferences>
       <EnableCOMDATFolding>true</EnableCOMDATFolding>
       <TargetMachine>MachineX86</TargetMachine>
+      <AdditionalDependencies>fwpuclnt.lib;ntdll.lib;shlwapi.lib;netapi32.lib;userenv.lib;iphlpapi.lib;%(AdditionalDependencies)</AdditionalDependencies>
     </Link>
   </ItemDefinitionGroup>
   <ItemGroup>
-    <ClCompile Include="openvpnserv.c" />
+    <ClCompile Include="..\openvpn\block_dns.c" />
+    <ClCompile Include="automatic.c" />
+    <ClCompile Include="common.c" />
+    <ClCompile Include="interactive.c" />
     <ClCompile Include="service.c" />
+    <ClCompile Include="validate.c" />
   </ItemGroup>
   <ItemGroup>
+    <ClInclude Include="..\openvpn\block_dns.h" />
     <ClInclude Include="service.h" />
+    <ClInclude Include="validate.h" />
   </ItemGroup>
   <ItemGroup>
     <ResourceCompile Include="openvpnserv_resources.rc" />
@@ -111,4 +113,4 @@
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
   <ImportGroup Label="ExtensionTargets">
   </ImportGroup>
-</Project>
+</Project>
\ No newline at end of file
diff --git a/src/openvpnserv/openvpnserv.vcxproj.filters b/src/openvpnserv/openvpnserv.vcxproj.filters
index 0c89b4f..c3267d1 100644
--- a/src/openvpnserv/openvpnserv.vcxproj.filters
+++ b/src/openvpnserv/openvpnserv.vcxproj.filters
@@ -7,29 +7,47 @@
     </Filter>
     <Filter Include="Header Files">
       <UniqueIdentifier>{93995380-89BD-4b04-88EB-625FBE52EBFB}</UniqueIdentifier>
-      <Extensions>h;hpp;hxx;hm;inl;inc;xsd</Extensions>
+      <Extensions>h;hh;hpp;hxx;hm;inl;inc;xsd</Extensions>
     </Filter>
     <Filter Include="Resource Files">
       <UniqueIdentifier>{67DA6AB6-F800-4c08-8B7A-83BB121AAD01}</UniqueIdentifier>
-      <Extensions>rc;ico;cur;bmp;dlg;rc2;rct;bin;rgs;gif;jpg;jpeg;jpe;resx;tiff;tif;png;wav</Extensions>
+      <Extensions>rc;ico;cur;bmp;dlg;rc2;rct;bin;rgs;gif;jpg;jpeg;jpe;resx;tiff;tif;png;wav;mfcribbon-ms</Extensions>
     </Filter>
   </ItemGroup>
   <ItemGroup>
-    <ClCompile Include="openvpnserv.c">
+    <ClCompile Include="automatic.c">
+      <Filter>Source Files</Filter>
+    </ClCompile>
+    <ClCompile Include="common.c">
+      <Filter>Source Files</Filter>
+    </ClCompile>
+    <ClCompile Include="interactive.c">
       <Filter>Source Files</Filter>
     </ClCompile>
     <ClCompile Include="service.c">
       <Filter>Source Files</Filter>
     </ClCompile>
-  </ItemGroup>
-  <ItemGroup>
-    <ClInclude Include="service.h">
-      <Filter>Header Files</Filter>
-    </ClInclude>
+    <ClCompile Include="validate.c">
+      <Filter>Source Files</Filter>
+    </ClCompile>
+    <ClCompile Include="..\openvpn\block_dns.c">
+      <Filter>Source Files</Filter>
+    </ClCompile>
   </ItemGroup>
   <ItemGroup>
     <ResourceCompile Include="openvpnserv_resources.rc">
       <Filter>Resource Files</Filter>
     </ResourceCompile>
   </ItemGroup>
+  <ItemGroup>
+    <ClInclude Include="service.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
+    <ClInclude Include="validate.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
+    <ClInclude Include="..\openvpn\block_dns.h">
+      <Filter>Header Files</Filter>
+    </ClInclude>
+  </ItemGroup>
 </Project>
\ No newline at end of file
-- 
2.9.0.windows.1

[Openvpn-devel] [PATCH 07/13] compat.vcxproj project file upgraded to Visual Studio 2017

[simon]

From: Simon Rozman <simon@...2293...>

1. The project file was ported to VS2017.
2. All non-essential settings reset to MSVC defaults.
---
 src/compat/compat.vcxproj         | 36 +++++++++++++++++-------------------
 src/compat/compat.vcxproj.filters |  4 ++--
 2 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/src/compat/compat.vcxproj b/src/compat/compat.vcxproj
index d2695e6..997aaf7 100644
--- a/src/compat/compat.vcxproj
+++ b/src/compat/compat.vcxproj
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="utf-8"?>
-<Project DefaultTargets="Build" ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+<Project DefaultTargets="Build" ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <ItemGroup Label="ProjectConfigurations">
     <ProjectConfiguration Include="Debug|Win32">
       <Configuration>Debug</Configuration>
@@ -11,6 +11,7 @@
     </ProjectConfiguration>
   </ItemGroup>
   <PropertyGroup Label="Globals">
+    <VCProjectVersion>15.0</VCProjectVersion>
     <ProjectGuid>{4B2E2719-E661-45D7-9203-F6F456B22F19}</ProjectGuid>
     <RootNamespace>compat</RootNamespace>
     <Keyword>Win32Proj</Keyword>
@@ -18,18 +19,20 @@
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.Default.props" />
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="Configuration">
     <ConfigurationType>StaticLibrary</ConfigurationType>
-    <CharacterSet>MultiByte</CharacterSet>
+    <CharacterSet>Unicode</CharacterSet>
     <WholeProgramOptimization>true</WholeProgramOptimization>
-    <PlatformToolset>v120</PlatformToolset>
+    <PlatformToolset>v141</PlatformToolset>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'" Label="Configuration">
     <ConfigurationType>StaticLibrary</ConfigurationType>
-    <CharacterSet>MultiByte</CharacterSet>
-    <PlatformToolset>v120</PlatformToolset>
+    <CharacterSet>Unicode</CharacterSet>
+    <PlatformToolset>v141</PlatformToolset>
   </PropertyGroup>
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.props" />
   <ImportGroup Label="ExtensionSettings">
   </ImportGroup>
+  <ImportGroup Label="Shared">
+  </ImportGroup>
   <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="PropertySheets">
     <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
   </ImportGroup>
@@ -37,39 +40,34 @@
     <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
   </ImportGroup>
   <PropertyGroup Label="UserMacros" />
-  <PropertyGroup>
-    <_ProjectFileVersion>10.0.30319.1</_ProjectFileVersion>
-    <OutDir Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
-    <IntDir Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">$(Configuration)\</IntDir>
-    <OutDir Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
-    <IntDir Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(Configuration)\</IntDir>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
+    <OutDir>$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
+    <IntDir>$(Configuration)\</IntDir>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
+    <OutDir>$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
+    <IntDir>$(Configuration)\</IntDir>
   </PropertyGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
     <ClCompile>
       <Optimization>Disabled</Optimization>
-      <AdditionalIncludeDirectories>$(SOURCEBASE);$(SOURCEBASE)/include;$(OPENSSL_HOME)/include;$(LZO_HOME)/include;$(PKCS11H_HOME)/include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <AdditionalIncludeDirectories>$(SOURCEBASE);$(SOURCEBASE)\include;$(OPENSSL_HOME)\include;$(LZO_HOME)\include;$(PKCS11H_HOME)\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
       <PreprocessorDefinitions>WIN32;_DEBUG;_LIB;$(CPPFLAGS);%(PreprocessorDefinitions)</PreprocessorDefinitions>
-      <MinimalRebuild>true</MinimalRebuild>
-      <BasicRuntimeChecks>EnableFastChecks</BasicRuntimeChecks>
       <RuntimeLibrary>MultiThreadedDebugDLL</RuntimeLibrary>
       <PrecompiledHeader>
       </PrecompiledHeader>
       <WarningLevel>Level3</WarningLevel>
-      <DebugInformationFormat>EditAndContinue</DebugInformationFormat>
     </ClCompile>
   </ItemDefinitionGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
     <ClCompile>
       <Optimization>MaxSpeed</Optimization>
-      <IntrinsicFunctions>true</IntrinsicFunctions>
-      <AdditionalIncludeDirectories>$(SOURCEBASE);$(SOURCEBASE)/include;$(OPENSSL_HOME)/include;$(LZO_HOME)/include;$(PKCS11H_HOME)/include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <AdditionalIncludeDirectories>$(SOURCEBASE);$(SOURCEBASE)\include;$(OPENSSL_HOME)\include;$(LZO_HOME)\include;$(PKCS11H_HOME)\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
       <PreprocessorDefinitions>WIN32;NDEBUG;_LIB;$(CPPFLAGS);%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <RuntimeLibrary>MultiThreadedDLL</RuntimeLibrary>
-      <FunctionLevelLinking>true</FunctionLevelLinking>
       <PrecompiledHeader>
       </PrecompiledHeader>
       <WarningLevel>Level3</WarningLevel>
-      <DebugInformationFormat>ProgramDatabase</DebugInformationFormat>
     </ClCompile>
   </ItemDefinitionGroup>
   <ItemGroup>
diff --git a/src/compat/compat.vcxproj.filters b/src/compat/compat.vcxproj.filters
index 0f78e86..acff7e7 100644
--- a/src/compat/compat.vcxproj.filters
+++ b/src/compat/compat.vcxproj.filters
@@ -7,11 +7,11 @@
     </Filter>
     <Filter Include="Header Files">
       <UniqueIdentifier>{93995380-89BD-4b04-88EB-625FBE52EBFB}</UniqueIdentifier>
-      <Extensions>h;hpp;hxx;hm;inl;inc;xsd</Extensions>
+      <Extensions>h;hh;hpp;hxx;hm;inl;inc;xsd</Extensions>
     </Filter>
     <Filter Include="Resource Files">
       <UniqueIdentifier>{67DA6AB6-F800-4c08-8B7A-83BB121AAD01}</UniqueIdentifier>
-      <Extensions>rc;ico;cur;bmp;dlg;rc2;rct;bin;rgs;gif;jpg;jpeg;jpe;resx;tiff;tif;png;wav</Extensions>
+      <Extensions>rc;ico;cur;bmp;dlg;rc2;rct;bin;rgs;gif;jpg;jpeg;jpe;resx;tiff;tif;png;wav;mfcribbon-ms</Extensions>
     </Filter>
   </ItemGroup>
   <ItemGroup>
-- 
2.9.0.windows.1

[Openvpn-devel] [PATCH 08/13] MSVC meta files added to .gitignore list

[simon]

From: Simon Rozman <simon@...2293...>

---
 .gitignore | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.gitignore b/.gitignore
index 30e289b..4543674 100644
--- a/.gitignore
+++ b/.gitignore
@@ -17,6 +17,7 @@
 Release
 Debug
 Win32-Output
+.vs
 .deps
 .libs
 Makefile
@@ -55,6 +56,7 @@ distro/systemd/*.service
 sample/sample-keys/sample-ca/
 vendor/.build
 vendor/dist
+build/msvc/msvc-generate/version.m4
 
 tests/t_client.sh
 tests/t_client-*-20??????-??????/
-- 
2.9.0.windows.1

[Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[simon]

From: Simon Rozman <simon@...2293...>

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

diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c
index d43cbcf..f88ba2c 100644
--- a/src/openvpn/block_dns.c
+++ b/src/openvpn/block_dns.c
@@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family)
         }
         return ipiface.Metric;
     }
-    return -err;
+    return -(int)err;
 }
 
 /*
diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
index 4123d0f..6c39aaa 100644
--- a/src/openvpnserv/automatic.c
+++ b/src/openvpnserv/automatic.c
@@ -155,7 +155,7 @@ match(const WIN32_FIND_DATA *find, LPCTSTR ext)
  * 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;
 
diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
index b8b817b..7901fd6 100644
--- a/src/openvpnserv/common.c
+++ b/src/openvpnserv/common.c
@@ -36,7 +36,7 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist)
         len = _vsntprintf(str, size, format, arglist);
         str[size - 1] = 0;
     }
-    return (len >= 0 && len < size);
+    return (len >= 0 && (size_t)len < size);
 }
 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
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -188,7 +188,7 @@ typedef enum {
 static DWORD
 AsyncPipeOp(async_op_t op, HANDLE pipe, LPVOID buffer, DWORD size, DWORD count, LPHANDLE events)
 {
-    int i;
+    DWORD i;
     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]);
 
     HANDLE wait_handles[2] = {rdns_semaphore, exit_event};
 
diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c
index 7a2d0e3..5422d33 100644
--- a/src/openvpnserv/validate.c
+++ b/src/openvpnserv/validate.c
@@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS token_groups, const WCHAR *group_nam
             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)
         {
             ret = EqualSid(members[i].lgrmi0_sid, sid);
         }
-- 
2.9.0.windows.1

[Openvpn-devel] [PATCH 10/13] msvc-generate.vcxproj project file upgraded to Visual Studio 2017

[simon]

From: Simon Rozman <simon@...2293...>

1. The project file was ported to VS2017.
2. All non-essential settings reset to MSVC defaults.
3. -ls flags added to NMake to reduce output clutter.
---
 build/msvc/msvc-generate/msvc-generate.vcxproj     | 42 +++++++++-------------
 .../msvc-generate/msvc-generate.vcxproj.filters    |  7 ++++
 2 files changed, 24 insertions(+), 25 deletions(-)
 create mode 100644 build/msvc/msvc-generate/msvc-generate.vcxproj.filters

diff --git a/build/msvc/msvc-generate/msvc-generate.vcxproj b/build/msvc/msvc-generate/msvc-generate.vcxproj
index 8b7ec22..f5fc921 100644
--- a/build/msvc/msvc-generate/msvc-generate.vcxproj
+++ b/build/msvc/msvc-generate/msvc-generate.vcxproj
@@ -1,5 +1,5 @@
 <?xml version="1.0" encoding="utf-8"?>
-<Project DefaultTargets="Build" ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+<Project DefaultTargets="Build" ToolsVersion="15.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <ItemGroup Label="ProjectConfigurations">
     <ProjectConfiguration Include="Debug|Win32">
       <Configuration>Debug</Configuration>
@@ -11,6 +11,7 @@
     </ProjectConfiguration>
   </ItemGroup>
   <PropertyGroup Label="Globals">
+    <VCProjectVersion>15.0</VCProjectVersion>
     <ProjectGuid>{8598C2C8-34C4-47A1-99B0-7C295A890615}</ProjectGuid>
     <RootNamespace>msvc-generate</RootNamespace>
     <Keyword>MakeFileProj</Keyword>
@@ -18,13 +19,19 @@
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.Default.props" />
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="Configuration">
     <ConfigurationType>Makefile</ConfigurationType>
+    <OutDir>$(Configuration)\</OutDir>
+    <IntDir>$(Configuration)\</IntDir>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'" Label="Configuration">
     <ConfigurationType>Makefile</ConfigurationType>
+    <OutDir>$(Configuration)\</OutDir>
+    <IntDir>$(Configuration)\</IntDir>
   </PropertyGroup>
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.props" />
   <ImportGroup Label="ExtensionSettings">
   </ImportGroup>
+  <ImportGroup Label="Shared">
+  </ImportGroup>
   <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="PropertySheets">
     <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
   </ImportGroup>
@@ -32,30 +39,15 @@
     <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
   </ImportGroup>
   <PropertyGroup Label="UserMacros" />
-  <PropertyGroup>
-    <_ProjectFileVersion>10.0.30319.1</_ProjectFileVersion>
-    <OutDir Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">$(Configuration)\</OutDir>
-    <IntDir Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">$(Configuration)\</IntDir>
-    <NMakeBuildCommandLine Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">nmake -f Makefile.mak all</NMakeBuildCommandLine>
-    <NMakeReBuildCommandLine Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">nmake -f Makefile.mak clean all</NMakeReBuildCommandLine>
-    <NMakeCleanCommandLine Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">nmake -f Makefile.mak clean</NMakeCleanCommandLine>
-    <NMakeOutput Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">config-msvc-version.h</NMakeOutput>
-    <NMakePreprocessorDefinitions Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">WIN32;_DEBUG;$(NMakePreprocessorDefinitions)</NMakePreprocessorDefinitions>
-    <NMakeIncludeSearchPath Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">$(NMakeIncludeSearchPath)</NMakeIncludeSearchPath>
-    <NMakeForcedIncludes Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">$(NMakeForcedIncludes)</NMakeForcedIncludes>
-    <NMakeAssemblySearchPath Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">$(NMakeAssemblySearchPath)</NMakeAssemblySearchPath>
-    <NMakeForcedUsingAssemblies Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">$(NMakeForcedUsingAssemblies)</NMakeForcedUsingAssemblies>
-    <OutDir Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(Configuration)\</OutDir>
-    <IntDir Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(Configuration)\</IntDir>
-    <NMakeBuildCommandLine Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">nmake -f Makefile.mak all</NMakeBuildCommandLine>
-    <NMakeReBuildCommandLine Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">nmake -f Makefile.mak clean all</NMakeReBuildCommandLine>
-    <NMakeCleanCommandLine Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">nmake -f Makefile.mak clean</NMakeCleanCommandLine>
-    <NMakeOutput Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">config-msvc-version.h</NMakeOutput>
-    <NMakePreprocessorDefinitions Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">WIN32;NDEBUG;$(NMakePreprocessorDefinitions)</NMakePreprocessorDefinitions>
-    <NMakeIncludeSearchPath Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(NMakeIncludeSearchPath)</NMakeIncludeSearchPath>
-    <NMakeForcedIncludes Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(NMakeForcedIncludes)</NMakeForcedIncludes>
-    <NMakeAssemblySearchPath Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(NMakeAssemblySearchPath)</NMakeAssemblySearchPath>
-    <NMakeForcedUsingAssemblies Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">$(NMakeForcedUsingAssemblies)</NMakeForcedUsingAssemblies>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
+    <NMakeBuildCommandLine>nmake -ls -f Makefile.mak all</NMakeBuildCommandLine>
+    <NMakeCleanCommandLine>nmake -ls -f Makefile.mak clean</NMakeCleanCommandLine>
+    <NMakeReBuildCommandLine>nmake -lsa -f Makefile.mak all</NMakeReBuildCommandLine>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
+    <NMakeBuildCommandLine>nmake -ls -f Makefile.mak all</NMakeBuildCommandLine>
+    <NMakeCleanCommandLine>nmake -ls -f Makefile.mak clean</NMakeCleanCommandLine>
+    <NMakeReBuildCommandLine>nmake -lsa -f Makefile.mak all</NMakeReBuildCommandLine>
   </PropertyGroup>
   <ItemDefinitionGroup>
   </ItemDefinitionGroup>
diff --git a/build/msvc/msvc-generate/msvc-generate.vcxproj.filters b/build/msvc/msvc-generate/msvc-generate.vcxproj.filters
new file mode 100644
index 0000000..1d04662
--- /dev/null
+++ b/build/msvc/msvc-generate/msvc-generate.vcxproj.filters
@@ -0,0 +1,7 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <ItemGroup>
+    <None Include="Makefile.mak" />
+    <None Include="msc-generate.js" />
+  </ItemGroup>
+</Project>
\ No newline at end of file
-- 
2.9.0.windows.1

[Openvpn-devel] [PATCH 11/13] openvpn.vcxproj temporarily disabled from building in openvpn.sln until we re-establish MSVC building for it

[simon]

From: Simon Rozman <simon@...2293...>

---
 openvpn.sln | 2 --
 1 file changed, 2 deletions(-)

diff --git a/openvpn.sln b/openvpn.sln
index 9f50f11..ac865bb 100644
--- a/openvpn.sln
+++ b/openvpn.sln
@@ -22,9 +22,7 @@ Global
 		{9C91EE0B-817D-420A-A1E6-15A5A9D98BAD}.Release|Win32.ActiveCfg = Release|Win32
 		{9C91EE0B-817D-420A-A1E6-15A5A9D98BAD}.Release|Win32.Build.0 = Release|Win32
 		{29DF226E-4D4E-440F-ADAF-5829CFD4CA94}.Debug|Win32.ActiveCfg = Debug|Win32
-		{29DF226E-4D4E-440F-ADAF-5829CFD4CA94}.Debug|Win32.Build.0 = Debug|Win32
 		{29DF226E-4D4E-440F-ADAF-5829CFD4CA94}.Release|Win32.ActiveCfg = Release|Win32
-		{29DF226E-4D4E-440F-ADAF-5829CFD4CA94}.Release|Win32.Build.0 = Release|Win32
 		{8598C2C8-34C4-47A1-99B0-7C295A890615}.Debug|Win32.ActiveCfg = Debug|Win32
 		{8598C2C8-34C4-47A1-99B0-7C295A890615}.Debug|Win32.Build.0 = Debug|Win32
 		{8598C2C8-34C4-47A1-99B0-7C295A890615}.Release|Win32.ActiveCfg = Release|Win32
-- 
2.9.0.windows.1

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

[simon]

From: Simon Rozman <simon@...2293...>

---
 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;
 
     if (find->dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
     {
         return false;
     }
 
-    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);
 }
 
@@ -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;
 
     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);
 }
 
 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)
@@ -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;
     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);
         free(input);
     }
 
-- 
2.9.0.windows.1

[Openvpn-devel] [PATCH 13/13] x64 MSVC configurations introduced

[simon]

From: Simon Rozman <simon@...2293...>

---
 .gitignore                                     |  1 +
 build/msvc/msvc-generate/msvc-generate.vcxproj | 42 ++++++++++++--
 msvc-env.bat                                   |  1 -
 openvpn.sln                                    | 16 ++++++
 src/compat/compat.vcxproj                      | 61 +++++++++++++++++++-
 src/openvpnserv/openvpnserv.vcxproj            | 77 ++++++++++++++++++++++++--
 6 files changed, 186 insertions(+), 12 deletions(-)

diff --git a/.gitignore b/.gitignore
index 4543674..ced65ec 100644
--- a/.gitignore
+++ b/.gitignore
@@ -17,6 +17,7 @@
 Release
 Debug
 Win32-Output
+x64-Output
 .vs
 .deps
 .libs
diff --git a/build/msvc/msvc-generate/msvc-generate.vcxproj b/build/msvc/msvc-generate/msvc-generate.vcxproj
index f5fc921..8978197 100644
--- a/build/msvc/msvc-generate/msvc-generate.vcxproj
+++ b/build/msvc/msvc-generate/msvc-generate.vcxproj
@@ -5,10 +5,18 @@
       <Configuration>Debug</Configuration>
       <Platform>Win32</Platform>
     </ProjectConfiguration>
+    <ProjectConfiguration Include="Debug|x64">
+      <Configuration>Debug</Configuration>
+      <Platform>x64</Platform>
+    </ProjectConfiguration>
     <ProjectConfiguration Include="Release|Win32">
       <Configuration>Release</Configuration>
       <Platform>Win32</Platform>
     </ProjectConfiguration>
+    <ProjectConfiguration Include="Release|x64">
+      <Configuration>Release</Configuration>
+      <Platform>x64</Platform>
+    </ProjectConfiguration>
   </ItemGroup>
   <PropertyGroup Label="Globals">
     <VCProjectVersion>15.0</VCProjectVersion>
@@ -19,13 +27,23 @@
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.Default.props" />
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="Configuration">
     <ConfigurationType>Makefile</ConfigurationType>
-    <OutDir>$(Configuration)\</OutDir>
-    <IntDir>$(Configuration)\</IntDir>
+    <OutDir>$(Configuration)\$(Platform)\</OutDir>
+    <IntDir>$(Configuration)\$(Platform)\</IntDir>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'" Label="Configuration">
+    <ConfigurationType>Makefile</ConfigurationType>
+    <OutDir>$(Configuration)\$(Platform)\</OutDir>
+    <IntDir>$(Configuration)\$(Platform)\</IntDir>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'" Label="Configuration">
     <ConfigurationType>Makefile</ConfigurationType>
-    <OutDir>$(Configuration)\</OutDir>
-    <IntDir>$(Configuration)\</IntDir>
+    <OutDir>$(Configuration)\$(Platform)\</OutDir>
+    <IntDir>$(Configuration)\$(Platform)\</IntDir>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'" Label="Configuration">
+    <ConfigurationType>Makefile</ConfigurationType>
+    <OutDir>$(Configuration)\$(Platform)\</OutDir>
+    <IntDir>$(Configuration)\$(Platform)\</IntDir>
   </PropertyGroup>
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.props" />
   <ImportGroup Label="ExtensionSettings">
@@ -35,20 +53,36 @@
   <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="PropertySheets">
     <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
   </ImportGroup>
+  <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'" Label="PropertySheets">
+    <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
+  </ImportGroup>
   <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'" Label="PropertySheets">
     <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
   </ImportGroup>
+  <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'" Label="PropertySheets">
+    <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
+  </ImportGroup>
   <PropertyGroup Label="UserMacros" />
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
     <NMakeBuildCommandLine>nmake -ls -f Makefile.mak all</NMakeBuildCommandLine>
     <NMakeCleanCommandLine>nmake -ls -f Makefile.mak clean</NMakeCleanCommandLine>
     <NMakeReBuildCommandLine>nmake -lsa -f Makefile.mak all</NMakeReBuildCommandLine>
   </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
+    <NMakeBuildCommandLine>nmake -ls -f Makefile.mak all</NMakeBuildCommandLine>
+    <NMakeCleanCommandLine>nmake -ls -f Makefile.mak clean</NMakeCleanCommandLine>
+    <NMakeReBuildCommandLine>nmake -lsa -f Makefile.mak all</NMakeReBuildCommandLine>
+  </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
     <NMakeBuildCommandLine>nmake -ls -f Makefile.mak all</NMakeBuildCommandLine>
     <NMakeCleanCommandLine>nmake -ls -f Makefile.mak clean</NMakeCleanCommandLine>
     <NMakeReBuildCommandLine>nmake -lsa -f Makefile.mak all</NMakeReBuildCommandLine>
   </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
+    <NMakeBuildCommandLine>nmake -ls -f Makefile.mak all</NMakeBuildCommandLine>
+    <NMakeCleanCommandLine>nmake -ls -f Makefile.mak clean</NMakeCleanCommandLine>
+    <NMakeReBuildCommandLine>nmake -lsa -f Makefile.mak all</NMakeReBuildCommandLine>
+  </PropertyGroup>
   <ItemDefinitionGroup>
   </ItemDefinitionGroup>
   <ItemGroup>
diff --git a/msvc-env.bat b/msvc-env.bat
index aabed75..56b469f 100644
--- a/msvc-env.bat
+++ b/msvc-env.bat
@@ -13,7 +13,6 @@ set SOURCEBASE=%cd%
 set SOLUTION=openvpn.sln
 set CPPFLAGS=%CPPFLAGS%;_CRT_SECURE_NO_WARNINGS;WIN32_LEAN_AND_MEAN;_CRT_NONSTDC_NO_WARNINGS;_CRT_SECURE_NO_WARNINGS
 set CPPFLAGS=%CPPFLAGS%;NTDDI_VERSION=NTDDI_VISTA;_WIN32_WINNT=_WIN32_WINNT_VISTA
-set CPPFLAGS=%CPPFLAGS%;_USE_32BIT_TIME_T
 set CPPFLAGS=%CPPFLAGS%;%EXTRA_CPPFLAGS%
 
 if exist config-msvc-local.h set CPPFLAGS="%CPPFLAGS%;HAVE_CONFIG_MSVC_LOCAL_H"
diff --git a/openvpn.sln b/openvpn.sln
index ac865bb..cf5a089 100644
--- a/openvpn.sln
+++ b/openvpn.sln
@@ -14,23 +14,39 @@ EndProject
 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Win32 = Debug|Win32
+		Debug|x64 = Debug|x64
 		Release|Win32 = Release|Win32
+		Release|x64 = Release|x64
 	EndGlobalSection
 	GlobalSection(ProjectConfigurationPlatforms) = postSolution
 		{9C91EE0B-817D-420A-A1E6-15A5A9D98BAD}.Debug|Win32.ActiveCfg = Debug|Win32
 		{9C91EE0B-817D-420A-A1E6-15A5A9D98BAD}.Debug|Win32.Build.0 = Debug|Win32
+		{9C91EE0B-817D-420A-A1E6-15A5A9D98BAD}.Debug|x64.ActiveCfg = Debug|x64
+		{9C91EE0B-817D-420A-A1E6-15A5A9D98BAD}.Debug|x64.Build.0 = Debug|x64
 		{9C91EE0B-817D-420A-A1E6-15A5A9D98BAD}.Release|Win32.ActiveCfg = Release|Win32
 		{9C91EE0B-817D-420A-A1E6-15A5A9D98BAD}.Release|Win32.Build.0 = Release|Win32
+		{9C91EE0B-817D-420A-A1E6-15A5A9D98BAD}.Release|x64.ActiveCfg = Release|x64
+		{9C91EE0B-817D-420A-A1E6-15A5A9D98BAD}.Release|x64.Build.0 = Release|x64
 		{29DF226E-4D4E-440F-ADAF-5829CFD4CA94}.Debug|Win32.ActiveCfg = Debug|Win32
+		{29DF226E-4D4E-440F-ADAF-5829CFD4CA94}.Debug|x64.ActiveCfg = Debug|Win32
 		{29DF226E-4D4E-440F-ADAF-5829CFD4CA94}.Release|Win32.ActiveCfg = Release|Win32
+		{29DF226E-4D4E-440F-ADAF-5829CFD4CA94}.Release|x64.ActiveCfg = Release|Win32
 		{8598C2C8-34C4-47A1-99B0-7C295A890615}.Debug|Win32.ActiveCfg = Debug|Win32
 		{8598C2C8-34C4-47A1-99B0-7C295A890615}.Debug|Win32.Build.0 = Debug|Win32
+		{8598C2C8-34C4-47A1-99B0-7C295A890615}.Debug|x64.ActiveCfg = Debug|x64
+		{8598C2C8-34C4-47A1-99B0-7C295A890615}.Debug|x64.Build.0 = Debug|x64
 		{8598C2C8-34C4-47A1-99B0-7C295A890615}.Release|Win32.ActiveCfg = Release|Win32
 		{8598C2C8-34C4-47A1-99B0-7C295A890615}.Release|Win32.Build.0 = Release|Win32
+		{8598C2C8-34C4-47A1-99B0-7C295A890615}.Release|x64.ActiveCfg = Release|x64
+		{8598C2C8-34C4-47A1-99B0-7C295A890615}.Release|x64.Build.0 = Release|x64
 		{4B2E2719-E661-45D7-9203-F6F456B22F19}.Debug|Win32.ActiveCfg = Debug|Win32
 		{4B2E2719-E661-45D7-9203-F6F456B22F19}.Debug|Win32.Build.0 = Debug|Win32
+		{4B2E2719-E661-45D7-9203-F6F456B22F19}.Debug|x64.ActiveCfg = Debug|x64
+		{4B2E2719-E661-45D7-9203-F6F456B22F19}.Debug|x64.Build.0 = Debug|x64
 		{4B2E2719-E661-45D7-9203-F6F456B22F19}.Release|Win32.ActiveCfg = Release|Win32
 		{4B2E2719-E661-45D7-9203-F6F456B22F19}.Release|Win32.Build.0 = Release|Win32
+		{4B2E2719-E661-45D7-9203-F6F456B22F19}.Release|x64.ActiveCfg = Release|x64
+		{4B2E2719-E661-45D7-9203-F6F456B22F19}.Release|x64.Build.0 = Release|x64
 	EndGlobalSection
 	GlobalSection(SolutionProperties) = preSolution
 		HideSolutionNode = FALSE
diff --git a/src/compat/compat.vcxproj b/src/compat/compat.vcxproj
index 997aaf7..0ebd3d5 100644
--- a/src/compat/compat.vcxproj
+++ b/src/compat/compat.vcxproj
@@ -5,10 +5,18 @@
       <Configuration>Debug</Configuration>
       <Platform>Win32</Platform>
     </ProjectConfiguration>
+    <ProjectConfiguration Include="Debug|x64">
+      <Configuration>Debug</Configuration>
+      <Platform>x64</Platform>
+    </ProjectConfiguration>
     <ProjectConfiguration Include="Release|Win32">
       <Configuration>Release</Configuration>
       <Platform>Win32</Platform>
     </ProjectConfiguration>
+    <ProjectConfiguration Include="Release|x64">
+      <Configuration>Release</Configuration>
+      <Platform>x64</Platform>
+    </ProjectConfiguration>
   </ItemGroup>
   <PropertyGroup Label="Globals">
     <VCProjectVersion>15.0</VCProjectVersion>
@@ -23,11 +31,22 @@
     <WholeProgramOptimization>true</WholeProgramOptimization>
     <PlatformToolset>v141</PlatformToolset>
   </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'" Label="Configuration">
+    <ConfigurationType>StaticLibrary</ConfigurationType>
+    <CharacterSet>Unicode</CharacterSet>
+    <WholeProgramOptimization>true</WholeProgramOptimization>
+    <PlatformToolset>v141</PlatformToolset>
+  </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'" Label="Configuration">
     <ConfigurationType>StaticLibrary</ConfigurationType>
     <CharacterSet>Unicode</CharacterSet>
     <PlatformToolset>v141</PlatformToolset>
   </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'" Label="Configuration">
+    <ConfigurationType>StaticLibrary</ConfigurationType>
+    <CharacterSet>Unicode</CharacterSet>
+    <PlatformToolset>v141</PlatformToolset>
+  </PropertyGroup>
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.props" />
   <ImportGroup Label="ExtensionSettings">
   </ImportGroup>
@@ -36,17 +55,31 @@
   <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="PropertySheets">
     <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
   </ImportGroup>
+  <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'" Label="PropertySheets">
+    <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
+  </ImportGroup>
   <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'" Label="PropertySheets">
     <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
   </ImportGroup>
+  <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'" Label="PropertySheets">
+    <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
+  </ImportGroup>
   <PropertyGroup Label="UserMacros" />
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
     <OutDir>$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
-    <IntDir>$(Configuration)\</IntDir>
+    <IntDir>$(Configuration)\$(Platform)\</IntDir>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
+    <OutDir>$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
+    <IntDir>$(Configuration)\$(Platform)\</IntDir>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
     <OutDir>$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
-    <IntDir>$(Configuration)\</IntDir>
+    <IntDir>$(Configuration)\$(Platform)\</IntDir>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
+    <OutDir>$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
+    <IntDir>$(Configuration)\$(Platform)\</IntDir>
   </PropertyGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
     <ClCompile>
@@ -59,6 +92,17 @@
       <WarningLevel>Level3</WarningLevel>
     </ClCompile>
   </ItemDefinitionGroup>
+  <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
+    <ClCompile>
+      <Optimization>Disabled</Optimization>
+      <AdditionalIncludeDirectories>$(SOURCEBASE);$(SOURCEBASE)\include;$(OPENSSL_HOME)\include;$(LZO_HOME)\include;$(PKCS11H_HOME)\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <PreprocessorDefinitions>WIN32;_DEBUG;_LIB;$(CPPFLAGS);%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <RuntimeLibrary>MultiThreadedDebugDLL</RuntimeLibrary>
+      <PrecompiledHeader>
+      </PrecompiledHeader>
+      <WarningLevel>Level3</WarningLevel>
+    </ClCompile>
+  </ItemDefinitionGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
     <ClCompile>
       <Optimization>MaxSpeed</Optimization>
@@ -70,6 +114,17 @@
       <WarningLevel>Level3</WarningLevel>
     </ClCompile>
   </ItemDefinitionGroup>
+  <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
+    <ClCompile>
+      <Optimization>MaxSpeed</Optimization>
+      <AdditionalIncludeDirectories>$(SOURCEBASE);$(SOURCEBASE)\include;$(OPENSSL_HOME)\include;$(LZO_HOME)\include;$(PKCS11H_HOME)\include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <PreprocessorDefinitions>WIN32;NDEBUG;_LIB;$(CPPFLAGS);%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <RuntimeLibrary>MultiThreadedDLL</RuntimeLibrary>
+      <PrecompiledHeader>
+      </PrecompiledHeader>
+      <WarningLevel>Level3</WarningLevel>
+    </ClCompile>
+  </ItemDefinitionGroup>
   <ItemGroup>
     <ClCompile Include="compat-basename.c" />
     <ClCompile Include="compat-dirname.c" />
@@ -85,4 +140,4 @@
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
   <ImportGroup Label="ExtensionTargets">
   </ImportGroup>
-</Project>
+</Project>
\ No newline at end of file
diff --git a/src/openvpnserv/openvpnserv.vcxproj b/src/openvpnserv/openvpnserv.vcxproj
index a865bf0..3cbc287 100644
--- a/src/openvpnserv/openvpnserv.vcxproj
+++ b/src/openvpnserv/openvpnserv.vcxproj
@@ -5,10 +5,18 @@
       <Configuration>Debug</Configuration>
       <Platform>Win32</Platform>
     </ProjectConfiguration>
+    <ProjectConfiguration Include="Debug|x64">
+      <Configuration>Debug</Configuration>
+      <Platform>x64</Platform>
+    </ProjectConfiguration>
     <ProjectConfiguration Include="Release|Win32">
       <Configuration>Release</Configuration>
       <Platform>Win32</Platform>
     </ProjectConfiguration>
+    <ProjectConfiguration Include="Release|x64">
+      <Configuration>Release</Configuration>
+      <Platform>x64</Platform>
+    </ProjectConfiguration>
   </ItemGroup>
   <PropertyGroup Label="Globals">
     <VCProjectVersion>15.0</VCProjectVersion>
@@ -23,11 +31,22 @@
     <WholeProgramOptimization>true</WholeProgramOptimization>
     <PlatformToolset>v141</PlatformToolset>
   </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'" Label="Configuration">
+    <ConfigurationType>Application</ConfigurationType>
+    <CharacterSet>Unicode</CharacterSet>
+    <WholeProgramOptimization>true</WholeProgramOptimization>
+    <PlatformToolset>v141</PlatformToolset>
+  </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'" Label="Configuration">
     <ConfigurationType>Application</ConfigurationType>
     <CharacterSet>Unicode</CharacterSet>
     <PlatformToolset>v141</PlatformToolset>
   </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'" Label="Configuration">
+    <ConfigurationType>Application</ConfigurationType>
+    <CharacterSet>Unicode</CharacterSet>
+    <PlatformToolset>v141</PlatformToolset>
+  </PropertyGroup>
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.props" />
   <ImportGroup Label="ExtensionSettings">
   </ImportGroup>
@@ -36,17 +55,31 @@
   <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'" Label="PropertySheets">
     <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
   </ImportGroup>
+  <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'" Label="PropertySheets">
+    <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
+  </ImportGroup>
   <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'" Label="PropertySheets">
     <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
   </ImportGroup>
+  <ImportGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'" Label="PropertySheets">
+    <Import Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props" Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')" Label="LocalAppDataPlatform" />
+  </ImportGroup>
   <PropertyGroup Label="UserMacros" />
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
     <OutDir>$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
-    <IntDir>$(Configuration)\</IntDir>
+    <IntDir>$(Configuration)\$(Platform)\</IntDir>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
+    <OutDir>$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
+    <IntDir>$(Configuration)\$(Platform)\</IntDir>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
     <OutDir>$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
-    <IntDir>$(Configuration)\</IntDir>
+    <IntDir>$(Configuration)\$(Platform)\</IntDir>
+  </PropertyGroup>
+  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
+    <OutDir>$(SolutionDir)$(Platform)-Output\$(Configuration)\</OutDir>
+    <IntDir>$(Configuration)\$(Platform)\</IntDir>
   </PropertyGroup>
   <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
     <ClCompile>
@@ -63,7 +96,24 @@
     </ResourceCompile>
     <Link>
       <SubSystem>Console</SubSystem>
-      <TargetMachine>MachineX86</TargetMachine>
+      <AdditionalDependencies>fwpuclnt.lib;ntdll.lib;shlwapi.lib;netapi32.lib;userenv.lib;iphlpapi.lib;%(AdditionalDependencies)</AdditionalDependencies>
+    </Link>
+  </ItemDefinitionGroup>
+  <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
+    <ClCompile>
+      <Optimization>Disabled</Optimization>
+      <AdditionalIncludeDirectories>$(SOURCEBASE);$(SOURCEBASE)\include;$(SOURCEBASE)\src\openvpn;$(SOURCEBASE)\src\compat;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <PreprocessorDefinitions>WIN32;_DEBUG;_CONSOLE;$(CPPFLAGS);%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <RuntimeLibrary>MultiThreadedDebugDLL</RuntimeLibrary>
+      <PrecompiledHeader>
+      </PrecompiledHeader>
+      <WarningLevel>Level3</WarningLevel>
+    </ClCompile>
+    <ResourceCompile>
+      <AdditionalIncludeDirectories>$(SOURCEBASE);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+    </ResourceCompile>
+    <Link>
+      <SubSystem>Console</SubSystem>
       <AdditionalDependencies>fwpuclnt.lib;ntdll.lib;shlwapi.lib;netapi32.lib;userenv.lib;iphlpapi.lib;%(AdditionalDependencies)</AdditionalDependencies>
     </Link>
   </ItemDefinitionGroup>
@@ -84,7 +134,26 @@
       <SubSystem>Console</SubSystem>
       <OptimizeReferences>true</OptimizeReferences>
       <EnableCOMDATFolding>true</EnableCOMDATFolding>
-      <TargetMachine>MachineX86</TargetMachine>
+      <AdditionalDependencies>fwpuclnt.lib;ntdll.lib;shlwapi.lib;netapi32.lib;userenv.lib;iphlpapi.lib;%(AdditionalDependencies)</AdditionalDependencies>
+    </Link>
+  </ItemDefinitionGroup>
+  <ItemDefinitionGroup Condition="'$(Configuration)|$(Platform)'=='Release|x64'">
+    <ClCompile>
+      <Optimization>MaxSpeed</Optimization>
+      <AdditionalIncludeDirectories>$(SOURCEBASE);$(SOURCEBASE)\include;$(SOURCEBASE)\src\openvpn;$(SOURCEBASE)\src\compat;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <PreprocessorDefinitions>WIN32;NDEBUG;_CONSOLE;$(CPPFLAGS);%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <RuntimeLibrary>MultiThreadedDLL</RuntimeLibrary>
+      <PrecompiledHeader>
+      </PrecompiledHeader>
+      <WarningLevel>Level3</WarningLevel>
+    </ClCompile>
+    <ResourceCompile>
+      <AdditionalIncludeDirectories>$(SOURCEBASE);%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+    </ResourceCompile>
+    <Link>
+      <SubSystem>Console</SubSystem>
+      <OptimizeReferences>true</OptimizeReferences>
+      <EnableCOMDATFolding>true</EnableCOMDATFolding>
       <AdditionalDependencies>fwpuclnt.lib;ntdll.lib;shlwapi.lib;netapi32.lib;userenv.lib;iphlpapi.lib;%(AdditionalDependencies)</AdditionalDependencies>
     </Link>
   </ItemDefinitionGroup>
-- 
2.9.0.windows.1

Re: [Openvpn-devel] [PATCH 01/13] snwprintf() => _snwprintf()

[David Sommerseth]

[-- Attachment #1.1: Type: text/plain, Size: 11 bytes --]


Hi Simon,

Re: [Openvpn-devel] [PATCH 01/13] snwprintf() => _snwprintf()

[Simon Rozman]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

Hi,

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

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.

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

Please apologize. I'll learn. Thank you for pointing me in the right 
direction.

Best regards,
Simon

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3293 bytes --]

Re: [Openvpn-devel] [PATCH 01/13] snwprintf() => _snwprintf()

[Selva]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]

On Wed, Oct 11, 2017 at 6:06 AM, Simon Rozman <simon@...2293...> wrote:

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


While its true that mingw accepts both and Windows has only the latter, I
think the best option here is swprintf. Its ISO-C, does the same thing as
_snwprintf with added advantage that nul-termination is guaranteed by the
standard (though we need not bank on that and better continue the practice
of explicit nul termination).  Note that swprintf is like snprintf in that
it takes the buffer size as an argument.

Selva

[-- Attachment #2: Type: text/html, Size: 2239 bytes --]

Re: [Openvpn-devel] [PATCH 05/13] Function prototypes are declared as "typedef <return type> (<calling convention> *type_name)(<arguments>)" in MSVC.

[Selva]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

Hi,

Thanks for these patches.

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

> From: Simon Rozman <simon@...2293...>
>
> 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.


Yes, those declarations and run-time lookup were slated for removal but
somehow never happened. Its a good idea to get rid of those -- no point in
tweaking them to suit MSVC.

IIRC, at least the versions of mingw that I use (gcc 4.9.3 or later) will
build with those stripped but I can test again if you replace this patch
with one removing those declarations.

Selva

[-- Attachment #2: Type: text/html, Size: 1584 bytes --]

Re: [Openvpn-devel] [PATCH 02/13] Mixing wide and regular strings in concatenations is not allowed in MSVC.

[Selva]

[-- Attachment #1: Type: text/plain, Size: 1541 bytes --]

Hi,

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

> From: Simon Rozman <simon@...2293...>
>
> ---
>  src/openvpnserv/interactive.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index 9d473e4..4b08086 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -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"
> -                        " 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"
> -                        " 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);
>

This one is easy, though the original is valid C99, MSVC does not support
concatenating non-identically prefixed strings.

ACK.

[-- Attachment #2: Type: text/html, Size: 2324 bytes --]

Re: [Openvpn-devel] [PATCH 05/13] Function prototypes are declared as "typedef <return type> (<calling convention> *type_name)(<arguments>)" in MSVC.

[Simon Rozman]

[-- Attachment #1.1: Type: text/plain, Size: 1736 bytes --]

Hi,



I shall happily rewrite the code to remove those declarations. However, I'll 
need to check MSDN on a case-by-case basis to replace only API calls that are 
standard from Vista on.



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.



Best regards,

Simon



From: Selva [mailto:selva.nair@...277...]
Sent: Wednesday, October 11, 2017 3:21 PM
To: Simon Rozman
Cc: openvpn-devel@lists.sourceforge.net
Subject: Re: [Openvpn-devel] [PATCH 05/13] Function prototypes are declared as 
"typedef <return type> (<calling convention> *type_name)(<arguments>)" in 
MSVC.



Hi,



Thanks for these patches.



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

From: Simon Rozman <simon@...2293...>

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.



Yes, those declarations and run-time lookup were slated for removal but 
somehow never happened. Its a good idea to get rid of those -- no point in 
tweaking them to suit MSVC.



IIRC, at least the versions of mingw that I use (gcc 4.9.3 or later) will 
build with those stripped but I can test again if you replace this patch with 
one removing those declarations.



Selva


[-- Attachment #1.2: Type: text/html, Size: 6576 bytes --]

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3317 bytes --]

Re: [Openvpn-devel] [PATCH 01/13] snwprintf() => _snwprintf()

[Simon Rozman]

[-- Attachment #1.1: Type: text/plain, Size: 2037 bytes --]

Hi,



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



We can do it two way: using #define, or implementing a simple wrapper 
function. The advantage of wrapper function is we can introduce guaranteed 
null-terminated output to match ISO-C experience.



Best regards,

Simon



From: Selva [mailto:selva.nair@...277...]
Sent: Wednesday, October 11, 2017 3:20 PM
To: Simon Rozman
Cc: openvpn-devel@lists.sourceforge.net
Subject: Re: [Openvpn-devel] [PATCH 01/13] snwprintf() => _snwprintf()





On Wed, Oct 11, 2017 at 6:06 AM, Simon Rozman <simon@...2293...> wrote:

Hi,

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

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.





While its true that mingw accepts both and Windows has only the latter, I 
think the best option here is swprintf. Its ISO-C, does the same thing as 
_snwprintf with added advantage that nul-termination is guaranteed by the 
standard (though we need not bank on that and better continue the practice of 
explicit nul termination).  Note that swprintf is like snprintf in that it 
takes the buffer size as an argument.



Selva






[-- Attachment #1.2: Type: text/html, Size: 6501 bytes --]

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3317 bytes --]

Re: [Openvpn-devel] [PATCH 05/13] Function prototypes are declared as "typedef <return type> (<calling convention> *type_name)(<arguments>)" in MSVC.

[Selva]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

Hi,


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

The interactive service is only available in 2.4 and above and was never
released for XP. Those dynamic lookups are a holdover from the original
patch that implemented this feature and somehow never got removed. All the
calls involved are supported in vista and above and removing all those
GetProcAddr() calls in interactive.c should work --  indeed there was a
patch submitted by someone  for this which never got much traction mainly
because it was for an older version of MSVC which did not support C99 and
thus included other changes.

Selva

[-- Attachment #2: Type: text/html, Size: 1272 bytes --]

Re: [Openvpn-devel] [PATCH 01/13] snwprintf() => _snwprintf()

[Selva]

[-- Attachment #1: Type: text/plain, Size: 328 bytes --]

Hi

On Wed, Oct 11, 2017 at 10:03 AM, Simon Rozman <simon@...2293...> wrote:

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

Just to be sure, ISO-C is 'swprintf' not 'snwprintf'. In spite of 'n'
misisng from that name, it takes the buffer size argument and nul
terminates even if output is truncated.

[-- Attachment #2: Type: text/html, Size: 1228 bytes --]

Re: [Openvpn-devel] [PATCH 04/13] Local functions are not supported in MSVC. Bummer.

[Selva]

[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]

Hi,

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

> From: Simon Rozman <simon@...2293...>
>
> ---
>  src/openvpnserv/interactive.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index 2019d38..7ff45b1 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -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)
>


Well, nested function is a GNU extension so we can't blame MSVC here..
We had more of these and removed by a previous patch but it keeps coming
back -- this one from me (commit 600dd9a16fc)

ACK.

[-- Attachment #2: Type: text/html, Size: 1978 bytes --]

[Openvpn-devel] [PATCH applied] Re: Local functions are not supported in MSVC. Bummer.

[Gert Doering]

Your patch has been applied to the master and release/2.4 branch.

commit 0893b14a7f8023964760e6229badcd2cfef57de2 (master)
commit 1b49bda5e10435dd42c2fe48d40893689fbdf986 (release/2.4)
Author: Simon Rozman
Date:   Wed Oct 11 01:11:21 2017 +0200

     Local functions are not supported in MSVC. Bummer.

     Acked-by: Selva Nair <selva.nair@...277...>
     Message-Id: <20171010231130.6832-4-simon@...2293...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15587.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH 05/13] Function prototypes are declared as "typedef <return type> (<calling convention> *type_name)(<arguments>)" in MSVC.

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

Hi,

On Wed, Oct 11, 2017 at 01:57:03PM +0000, Simon Rozman wrote:
> 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.

These patches won't go into 2.3 - 2.3 is in strict maintenance mode,
and the code base is sufficiently different (especially wrt to openvpn
interactive service, which is not in the 2.3 code base and not needed on
XP anyway) that it would need more effort for backporting.

In other words: just ignore XP.

gert

-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             gert@...1296...
fax: +49-89-35655025                        gert@...1297...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

[Openvpn-devel] [PATCH applied] Re: Mixing wide and regular strings in concatenations is not allowed in MSVC.

[Gert Doering]

Your patch has been applied to the master and release/2.4 branch.

commit d6e0917922793315b06aba395ed0666e17c5b44c (master)
commit 6e9da5a55673fe01ae7b780f0c438c81773109d5 (release/2.4)
Author: Simon Rozman
Date:   Wed Oct 11 01:11:19 2017 +0200

     Mixing wide and regular strings in concatenations is not allowed in MSVC.

     Acked-by: Selva Nair <selva.nair@...277...>
     Message-Id: <20171010231130.6832-2-simon@...2293...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15579.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH 03/13] RtlIpv6AddressToStringW() and RtlIpv4AddressToStringW() require mstcpip.h

[Selva]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

Hi,

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

> From: Simon Rozman <simon@...2293...>
>
> ---
>  src/openvpnserv/interactive.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
> index 4b08086..2019d38 100644
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -33,6 +33,7 @@
>  #include <stdio.h>
>  #include <sddl.h>
>  #include <shellapi.h>
> +#include <mstcpip.h>
>
>  #ifdef HAVE_VERSIONHELPERS_H
>  #include <versionhelpers.h>
>

In mingw build mstcpip.h gets pulled-in though ws2tcpip.h but MSDN says to
explicitly include it for RtlIpv4AddressToStringW etc.

Anyway mingw is still happy at least with this order of includes, so looks
good to me.

ACK.

Selva

[-- Attachment #2: Type: text/html, Size: 1346 bytes --]

[Openvpn-devel] [PATCH applied] Re: RtlIpv6AddressToStringW() and RtlIpv4AddressToStringW() require mstcpip.h

[Gert Doering]

Your patch has been applied to the master and release/2.4 branch.

commit 55305a2fc66a768cbbf152da9092400590504574 (master)
commit 0e91a2ddb79f34a8cb361cda2f4d4a1df39d3c18 (release/2.4)
Author: Simon Rozman
Date:   Wed Oct 11 01:11:20 2017 +0200

     RtlIpv6AddressToStringW() and RtlIpv4AddressToStringW() require mstcpip.h

     Acked-by: Selva Nair <selva.nair@...277...>
     Message-Id: <20171010231130.6832-3-simon@...2293...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15582.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH 01/13] snwprintf() => _snwprintf()

[Simon Rozman]

[-- Attachment #1.1: Type: text/plain, Size: 734 bytes --]

Hi,



Thank you for your feedback.



Based on it, I have sent a new patch "[PATCH] Uniform snwprintf() across MinGW 
and MSVC compilers", that supersedes this one.



Best regards,

Simon



From: Selva [mailto:selva.nair@...277...]
Sent: Wednesday, October 11, 2017 4:32 PM
To: Simon Rozman
Cc: openvpn-devel@lists.sourceforge.net
Subject: Re: [Openvpn-devel] [PATCH 01/13] snwprintf() => _snwprintf()



Hi



On Wed, Oct 11, 2017 at 10:03 AM, Simon Rozman <simon@...2293...> wrote:

Hi,



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



Just to be sure, ISO-C is 'swprintf' not 'snwprintf'. In spite of 'n' misisng 
from that name, it takes the buffer size argument and nul terminates even if 
output is truncated.


[-- Attachment #1.2: Type: text/html, Size: 5329 bytes --]

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 3317 bytes --]

Re: [Openvpn-devel] [PATCH 06/13] openvpnserv.vcxproj project file recreated using Visual Studio 2017

[Selva]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

Hi,

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

> From: Simon Rozman <simon@...2293...>
>
> 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.

Patches 9 and 12 are not Windows specific and so hopefully someone will get
to
those before I may find time..

Thanks,

Selva

[-- Attachment #2: Type: text/html, Size: 1475 bytes --]

Re: [Openvpn-devel] [PATCH 06/13] openvpnserv.vcxproj project file recreated using Visual Studio 2017

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1033 bytes --]

Hi,

On Thu, Oct 12, 2017 at 11:56:23AM -0400, Selva wrote:
> 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. 

I had hoped that you'll review this, as you *are* the one who understands
windows best :-)

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

I'll take those if nobody else volunteers.  I have no idea about that
stuff, but if it looks like "it's the same thing, we just have new source
files that were not added when introducing the .c file" I can handle it :-)

thanks,

gert


-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             gert@...1296...
fax: +49-89-35655025                        gert@...1297...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

[Openvpn-devel] [PATCH applied] Re: MSVC meta files added to .gitignore list

[Gert Doering]

ACK.  This is something I can understand, without knowing MSVC :-)

Your patch has been applied to the master and release/2.4 branch.

commit 289ba682c70f9ea801fabca297115409acc437c9 (master)
commit 6d4d62859db9bbe9b979d485fb7539a56a25212c (release/2.4)
Author: Simon Rozman
Date:   Wed Oct 11 01:11:25 2017 +0200

     MSVC meta files added to .gitignore list

     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <20171010231130.6832-8-simon@...2293...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15585.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 3961 bytes --]

Hi,

On Wed, Oct 11, 2017 at 01:11:26AM +0200, simon@...2293... wrote:
> From: Simon Rozman <simon@...2293...>
> 
> ---
>  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(-)

So... trying to make sense of this.

> diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c
> index d43cbcf..f88ba2c 100644
> --- a/src/openvpn/block_dns.c
> +++ b/src/openvpn/block_dns.c
> @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index, const ADDRESS_FAMILY family)
>          }
>          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.

>  
>  /*
> diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
> index 4123d0f..6c39aaa 100644
> --- a/src/openvpnserv/automatic.c
> +++ b/src/openvpnserv/automatic.c
> @@ -155,7 +155,7 @@ match(const WIN32_FIND_DATA *find, LPCTSTR ext)
>   * 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.

>  
> diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
> index b8b817b..7901fd6 100644
> --- a/src/openvpnserv/common.c
> +++ b/src/openvpnserv/common.c
> @@ -36,7 +36,7 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist)
>          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?


>  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
> --- a/src/openvpnserv/interactive.c
> +++ b/src/openvpnserv/interactive.c
> @@ -188,7 +188,7 @@ typedef enum {
>  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...?
 
>      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...

> index 7a2d0e3..5422d33 100644
> --- a/src/openvpnserv/validate.c
> +++ b/src/openvpnserv/validate.c
> @@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS token_groups, const WCHAR *group_nam
>              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?

gert

-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             gert@...1296...
fax: +49-89-35655025                        gert@...1297...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Selva]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

Hi,


> > index 7a2d0e3..5422d33 100644
> > --- a/src/openvpnserv/validate.c
> > +++ b/src/openvpnserv/validate.c
> > @@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS
> token_groups, const WCHAR *group_nam
> >              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?
>

I think these are needed to silence signed/unsigned comparison warnings
(== is ok, but  < and > would warn). gcc also would warn if -Wextra or
-Wsign-compare is used.

Simon may have more to say.

Selva

[-- Attachment #2: Type: text/html, Size: 1211 bytes --]

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[David Sommerseth]

[-- Attachment #1.1: Type: text/plain, Size: 444 bytes --]

On 05/11/17 20:20, Gert Doering wrote:
>> -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.

I would label this more as "use size_t for size/length related
variables".  So I read this more as a coding style change than anything
else.

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Simon Rozman]

Hi,

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

> > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c index
> > d43cbcf..f88ba2c 100644
> > --- a/src/openvpn/block_dns.c
> > +++ b/src/openvpn/block_dns.c
> > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index,
> const ADDRESS_FAMILY family)
> >          }
> >          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.

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.

> >  /*
> > diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
> > index 4123d0f..6c39aaa 100644
> > --- a/src/openvpnserv/automatic.c
> > +++ b/src/openvpnserv/automatic.c
> > @@ -155,7 +155,7 @@ match(const WIN32_FIND_DATA *find, LPCTSTR
> ext)
> >   * 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.

warning C4018: '<=': signed/unsigned mismatch (in Win32 builds twice)
https://msdn.microsoft.com/en-us/library/y92ktdf2.aspx

Parameter size is compared to the result of _tcslen() which is size_t. Hence, you had a signed int and unsigned size_t comparison.

However, modext() is called exactly at one location in the code:

                if (!modext(log_file, _countof(log_file), find_obj.cFileName, TEXT("log")))
                    ...

Observe, the size parameter is _countof(log_file) which is size_t, converted to int to be passed as parameter to modext(), and finally compared against _tcslen() which is size_t in modext(). In other words: it had an orange, converted it to an apple, and finally compared it to another orange.

> > diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
> index
> > b8b817b..7901fd6 100644
> > --- a/src/openvpnserv/common.c
> > +++ b/src/openvpnserv/common.c
> > @@ -36,7 +36,7 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR
> format, va_list arglist)
> >          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?

warning C4018: '<': signed/unsigned mismatch (in Win32 builds).

The MSVC is not happy if it needs to decide between signed or unsigned comparison itself. So the programmer needs to cast either side to match the signed/unsigned manually.

The `len >= 0` assures that len is always positive or zero before `len < size` is evaluated. Therefore, it is no harm to cast len to size_t to force unsigned comparison.

> >  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
> > --- a/src/openvpnserv/interactive.c
> > +++ b/src/openvpnserv/interactive.c
> > @@ -188,7 +188,7 @@ typedef enum {
> >  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...?

This one is my favourite. :) I hope I'll manage to explain it (I am not a native English speaker. Please, be patient.) This one is not just about C4018 again. Although, the `i < count` in `for (i = 0; i < count; i++)` does raise the C4018. That's how it got my attention.

This particular loop variable i was used in one loop only. In that particular loop, it iterates on [0...count) interval. The lower bound is zero (any scalar zero). The upper bound is a DWORD. Thus, the interval is essentially a [DWORD...DWORD) type. So the most natural selection for the loop variable is DWORD then. This avoids all 32/64-bit issues, signed/unsigned comparison warnings etc.

When you need to iterate from 'A' to 'Z', the most natural selection for loop variable is char; when you count oranges, the appropriate loop variable is orange...

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

Exactly. This one's the other way around: Here, the original code used `DWORD i` as the loop variable. But the upper bound was `int ncmds`. Since ncmds is a school example for size_t, the unsigned DWORD is still a better choice than int.

Although, if I was coding this, I'd change everything to size_t. :)

> > index 7a2d0e3..5422d33 100644
> > --- a/src/openvpnserv/validate.c
> > +++ b/src/openvpnserv/validate.c
> > @@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS
> token_groups, const WCHAR *group_nam
> >              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.

Again, the nread is number of members of a security group - a DWORD (Windows API really likes this type). It should be iterated using a DWORD or face the dreaded signed/unsigned comparison warning again.

> Consider me not convinced...

I hope you might reconsider again. :)

> what exactly are the warnings MSVC spits out here?

As described above.

MSVC for OpenVPN projects is set to Warning level 3. On the scale of 0 to 4 Microsoft calls this level the "production quality" level. Apart from those signed/unsigned warnings, there were some 32/64-bit warnings reported at 64-bit MSVC builds and are addressed by "[PATCH 12/13] Memory size arithmetic reviewed according to 64-bit MSVC complaints".

All that said, the OpenVPN Interactive Service code is a very fine example of code that - given the number of lines - produces very low volume of warnings. Why not address and fix them, when we're so close?

While most of the MSVC compiler warnings are false-alarms, one out of many does represent a potential threat. But you don't know which one that is. Therefore it is best to address them all. :) Just my personal philosophy, using which, I keep the helpdesk guys out of my office quite efficiently.

Best regards,
Simon

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Selva]

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]

Hi,

I understand the intent here and its nice to get rid of compiler warnings,
when
it makes sense. But be careful while silencing by just a cast.

On Sun, Nov 5, 2017 at 5:06 PM, Simon Rozman <simon@...2293...> wrote:
>
> Hi,
>
> Let me explain all proposed modifications case-by-case below...
>
> > > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c index
> > > d43cbcf..f88ba2c 100644
> > > --- a/src/openvpn/block_dns.c
> > > +++ b/src/openvpn/block_dns.c
> > > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index,
> > const ADDRESS_FAMILY family)
> > >          }
> > >          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.
>
> 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 <(214)%20748-3649> (2 more than INT_MAX)
Original will return  2147483647 <(214)%20748-3647> (not a -ve int)
The new code will return the same 2147483647 <(214)%20748-3647>
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.

While most of the MSVC compiler warnings are false-alarms, one out of many
> does represent a potential threat. But you don't know which one that is.
> Therefore it is best to address them all. :)
>

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

Selva

[-- Attachment #2: Type: text/html, Size: 2992 bytes --]

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Simon Rozman]

Hi,

> > > > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c
> > > > index d43cbcf..f88ba2c 100644
> > > > --- a/src/openvpn/block_dns.c
> > > > +++ b/src/openvpn/block_dns.c
> > > > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index,
> > > const ADDRESS_FAMILY family)
> > > >          }
> > > >          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.
> >
> > 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.

I totally agree with your point, the original code might be logically wrong. If the interface metric is > INT_MAX (which ULONG theoretically could be), it'll get returned as a negative number => false-positive error condition.

The get_interface_metric()also return error codes > INT_MAX as a positive numbers => invalid interface metric instead of error condition. As you described this case yourself.

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

Option 1: Add an `if` to limit metric to INT_MAX, and an `if` to return all errors greater than INT_MAX simply as -1.

Option 2: redefine function prototype to return error code unmodified, and metrics using an extra parameter passed by reference.

I'd prefer option 1 after reviewing all get_interface_metrics() calls. None of them cares about the exact error code. They just test for negative value and set the metrics to -1. As a matter of fact, the get_interface_metrics() itself could be rewritten to return -1 as the metric for any error in the first place, thus making the error checks after each function call redundant, thus simplifying code.

Best regards,
Simon

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

Hi,

On Mon, Nov 06, 2017 at 08:35:48AM +0000, Simon Rozman wrote:
> > 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?

Refactoring is definitely welcome for master, and can be argued into 2.4 if 
it fixes things that need fixing...

(And thanks for taking your time to explain the background for all these
changes - I haven't had time today to do more than just read all the mails,
but they were helpful - thanks to Selva, too)

gert
-- 
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             gert@...1296...
fax: +49-89-35655025                        gert@...1297...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Selva]

[-- Attachment #1: Type: text/plain, Size: 3883 bytes --]

Hi,

On Mon, Nov 6, 2017 at 3:35 AM, Simon Rozman <simon@...2293...> wrote:
>
> Hi,
>
> > > > > diff --git a/src/openvpn/block_dns.c b/src/openvpn/block_dns.c
> > > > > index d43cbcf..f88ba2c 100644
> > > > > --- a/src/openvpn/block_dns.c
> > > > > +++ b/src/openvpn/block_dns.c
> > > > > @@ -370,7 +370,7 @@ get_interface_metric(const NET_IFINDEX index,
> > > > const ADDRESS_FAMILY family)
> > > > >          }
> > > > >          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.
> > >
> > > 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 <(214)%20748-3649> (2 more than INT_MAX)
> > Original will return  2147483647 <(214)%20748-3647> (not a -ve int) The
new code will return the
> > same 2147483647 <(214)%20748-3647> 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.
>
> I totally agree with your point, the original code might be logically
wrong. If the interface metric is > INT_MAX (which ULONG theoretically
could be), it'll get returned as a negative number => false-positive error
condition.
>
> The get_interface_metric()also return error codes > INT_MAX as a positive
numbers => invalid interface metric instead of error condition. As you
described this case yourself.
>
> > 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?
>
>
> Option 1: Add an `if` to limit metric to INT_MAX, and an `if` to return
all errors greater than INT_MAX simply as -1.
>
> Option 2: redefine function prototype to return error code unmodified,
and metrics using an extra parameter passed by reference.
>
>
> I'd prefer option 1 after reviewing all get_interface_metrics() calls.
None of them cares about the exact error code. They just test for negative
value and set the metrics to -1. As a matter of fact, the
get_interface_metrics() itself could be rewritten to return -1 as the
metric for any error in the first place, thus making the error checks after
each function call redundant, thus simplifying code.

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.

Selva

[-- Attachment #2: Type: text/html, Size: 5096 bytes --]

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Simon Rozman]

Hi,

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

I shall give it a look after the Hackathon.

Best regards,
Simon

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

[Selva]

[-- Attachment #1: Type: text/plain, Size: 5077 bytes --]

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@...2293...> wrote:

> From: Simon Rozman <simon@...2293...>
>
> ---
>  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

[-- Attachment #2: Type: text/html, Size: 9160 bytes --]

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

[Simon Rozman]

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

[Openvpn-devel] [PATCH v2] openvpnserv: Review MSVC down-casting warnings

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

diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
index 75c3be2..4d5501b 100644
--- a/src/openvpnserv/automatic.c
+++ b/src/openvpnserv/automatic.c
@@ -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;
 }
 
 /*
@@ -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('\\'))
             {
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);
     }
 
-- 
2.9.0.windows.1

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

[Selva]

[-- Attachment #1: Type: text/plain, Size: 1521 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 2214 bytes --]

Re: [Openvpn-devel] [PATCH v2] openvpnserv: Review MSVC down-casting warnings

[Selva]

[-- Attachment #1: Type: text/plain, Size: 1516 bytes --]

Hi,

Thanks for the v2

On Mon, Nov 13, 2017 at 4:49 AM, Simon Rozman <simon@...2293...> 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   | 20 ++++----------------
>  src/openvpnserv/interactive.c | 12 ++++++------
>  2 files changed, 10 insertions(+), 22 deletions(-)
>
> diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
> index 75c3be2..4d5501b 100644
> --- a/src/openvpnserv/automatic.c
> +++ b/src/openvpnserv/automatic.c
> @@ -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;


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.

Selva

[-- Attachment #2: Type: text/html, Size: 2156 bytes --]

Re: [Openvpn-devel] [PATCH v2] openvpnserv: Review MSVC down-casting warnings

[Selva Nair]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

Hi Simon,

And this one:

On Mon, Nov 13, 2017 at 11:26 AM, Selva <selva.nair@...277...> wrote:

> Hi,
>
> Thanks for the v2
>
> On Mon, Nov 13, 2017 at 4:49 AM, Simon Rozman <simon@...2293...> 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   | 20 ++++----------------
>>  src/openvpnserv/interactive.c | 12 ++++++------
>>  2 files changed, 10 insertions(+), 22 deletions(-)
>>
>> diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
>> index 75c3be2..4d5501b 100644
>> --- a/src/openvpnserv/automatic.c
>> +++ b/src/openvpnserv/automatic.c
>> @@ -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;
>
>
> 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.
>

Needs a v3, I suppose.

Thanks,

Selva

[-- Attachment #2: Type: text/html, Size: 2686 bytes --]

[Openvpn-devel] [PATCH v3] openvpnserv: Review MSVC down-casting warnings

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

diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
index 75c3be2..4d5501b 100644
--- a/src/openvpnserv/automatic.c
+++ b/src/openvpnserv/automatic.c
@@ -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;
 }
 
 /*
@@ -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('\\'))
             {
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);
     }
 
-- 
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

Re: [Openvpn-devel] [PATCH v3] openvpnserv: Review MSVC down-casting warnings

[Selva Nair]

[-- Attachment #1: Type: text/plain, Size: 5001 bytes --]

Hi Simon,

Thanks. The v3 has just arrived in patchwork -- for some reason not in my
mailbox yet, probably its coming..

Looks like v3 is an exact copy of v2 -- no check for empty ext which was
the only change required.

Am I missing something?

Thanks,

Selva

On Sun, Dec 3, 2017 at 12:19 PM, Simon Rozman <simon@...2293...> 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   | 20 ++++----------------
>  src/openvpnserv/interactive.c | 12 ++++++------
>  2 files changed, 10 insertions(+), 22 deletions(-)
>
> diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
> index 75c3be2..4d5501b 100644
> --- a/src/openvpnserv/automatic.c
> +++ b/src/openvpnserv/automatic.c
> @@ -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;
>  }
>
>  /*
> @@ -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('\\'))
>              {
> 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);
>      }
>
> --
> 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
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>

[-- Attachment #2: Type: text/html, Size: 6625 bytes --]

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

[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   | 19 +++++++------------
 src/openvpnserv/interactive.c | 12 ++++++------
 2 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
index 75c3be2..28f21cc 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))
-    {
-        return true;
-    }
-
-    i = _tcslen(find->cFileName) - _tcslen(ext) - 1;
-    if (i < 1)
+    if (*ext == TEXT('\0'))
     {
         return false;
     }
 
-    return find->cFileName[i] == '.' && !_tcsicmp(find->cFileName + i + 1, ext);
+    /* find the pointer to that last '.' in filename and match ext against the rest */
+
+    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);
     }
 
-- 
2.9.0.windows.1

Hi,

My mistake. Here's the v4 of the patch.

Regards,
Simon

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

[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   | 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;
     }
 
-    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);
     }
 
-- 
2.9.0.windows.1

Hi Selva,

After more careful review of your `if (*ext == TEXT('\0'))` proposal in reply to v1 patch, I noticed you changed the return value for empty ext string from true to false.

This version of the patch returns the same result for empty ext string as the original version of the function. When HKLM\Software\OpenVPN\config_ext is set to an empty string the automatic service starts openvpn.exe on all files found as previous versions.

Best regards,
Simon

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

[Selva Nair]

[-- Attachment #1: Type: text/plain, Size: 4632 bytes --]

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@...2293...> 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@...277...>

[-- Attachment #2: Type: text/html, Size: 6804 bytes --]

[Openvpn-devel] [PATCH applied] Re: openvpnserv: Review MSVC down-casting warnings

[Gert Doering]

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@...277...>
     Message-Id: <20171203203007.6628-1-simon@...2293...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16001.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

Hi,

On Wed, Nov 08, 2017 at 06:46:53PM +0000, Simon Rozman wrote:
> > 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.

"ping"?

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

gert

-- 
now what should I write here...

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Simon Rozman]

Hi,

> On Wed, Nov 08, 2017 at 06:46:53PM +0000, Simon Rozman wrote:
> > > 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.
> 
> "ping"?
> 
> 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.

Best regards,
Simon

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Selva Nair]

Hi Simon,

On Tue, Dec 5, 2017 at 4:44 AM, Simon Rozman <simon@...2293...> wrote:
> Hi,
>
>> On Wed, Nov 08, 2017 at 06:46:53PM +0000, Simon Rozman wrote:
>> > > 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.
>>
>> "ping"?
>>
>> 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.

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?

Thanks,

Selva

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Simon Rozman]

Hi,

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

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.

Best regards,
Simon

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Selva Nair]

Hi,

On Wed, Dec 6, 2017 at 12:10 PM, Simon Rozman <simon@...2293...> wrote:
> Hi,
>
>> > 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?
>
> 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.

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

Selva

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Simon Rozman]

Hi,

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

Regards,
Simon

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]

Hi,

On Thu, Dec 07, 2017 at 06:41:03AM +0000, Simon Rozman wrote:
> > > 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.

Formally we have no really clear line on "who can do ACK/NAK" (and this
has caused a bit of friction in the past, my fault, apologies).

I tend to see ACKs as "I have stared at the code, ran some tests, can't
see anything bad in the code and the change is useful" - but usually, 
David or I do our own review and tests before actually merging.  

Depending on the "track record" of the one sending that ACK, it might be 
a very loose review, or very thorough :-)  (like, when Steffan ACKs a crypto
patch, all I do is "does it compile and pass t_client test?", but I do not
try to understand crypto library API nuances...).

Sometimes I find things in post-ACK-review that reopen the discussion,
though :-)


NAKs are maybe even more important - "watch out, this code might break
under circumstances x, y, z!" - while the reviewer only considered cases
a, b and c...

So - feel free to comment on patches you feel qualified to review!


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

Good enough for a start :-)

gert
-- 
now what should I write here...

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Gisle Vanem]

Simon Rozman wrote:

> 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


-- 
--gv

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Selva Nair]

Hi,

On Thu, Dec 7, 2017 at 12:32 PM, Gisle Vanem <gisle.vanem@...277...> wrote:
> Simon Rozman wrote:
>
>> 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

Are you referring to the patch under discussion or something else?

The code you quote above is not in current master nor in 2.4. In fact that
!(defined(__MINGW64_VERSION_MAJOR) &&__MINGW64_VERSION_MAJOR < 2
even contradicts the comment above it. Its likely not from the
official codebase.

Anyway, MIB_TCP_STATE is typedef-ed to int in syshead.h whenever
 _WIN32 is defined. We may not need it anymore, but it hasn't so
far interfered with any builds.

Indeed its an enum as per MSDN (vista+ addition?). If clang doesn't
like passing an int as enum and is using Microsoft's headers, it could
trip there even if MSVC allows it. Such a combination has never been
tested before, I suppose.

Selva

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Gert Doering]

[-- Attachment #1: Type: text/plain, Size: 849 bytes --]

Hi,

On Wed, Dec 06, 2017 at 05:10:45PM +0000, Simon Rozman wrote:
> > > 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.

I finally came around to merging the get_interface_metric() patch from
Selva today, so I await your fixup patch of the remaining MSVC warnings :-)

thanks,

gert

-- 
now what should I write here...

Gert Doering - Munich, Germany                             gert@...1296...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[Simon Rozman]

[-- Attachment #1: Type: text/plain, Size: 818 bytes --]

Hi,

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

Best regards,
Simon

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4943 bytes --]

Re: [Openvpn-devel] [PATCH 09/13] Signed/unsigned warnings of MSVC resolved

[@ 2018-02-02 8:25 UTC (permalink / raw)]

Il 02/02/2018 03:00, Simon Rozman ha scritto:
> Hi,
> 
>>> 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
>> 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.
> 

Hi Simon,

Please take your time and recover. We can wait a bit more to get the old
Simon back :).

-- 
Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

[Openvpn-devel] [PATCH v2 09/13] Signed/unsigned warnings of MSVC resolved

[Simon Rozman]

This patch fixes the signed/unsigned comparison warnings discovered when
compiling openvpnserv using MSVC.

Wherever possible, it changes iterator and/or size variables to a more
appropriate type, or uses type-casting when it is safe to do so.
---
 src/openvpnserv/automatic.c   | 2 +-
 src/openvpnserv/common.c      | 2 +-
 src/openvpnserv/interactive.c | 7 +++----
 src/openvpnserv/validate.c    | 2 +-
 4 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/openvpnserv/automatic.c b/src/openvpnserv/automatic.c
index 1f98283..89367fc 100644
--- a/src/openvpnserv/automatic.c
+++ b/src/openvpnserv/automatic.c
@@ -135,7 +135,7 @@ match(const WIN32_FIND_DATA *find, LPCTSTR ext)
  * 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)
 {
     size_t i;
 
diff --git a/src/openvpnserv/common.c b/src/openvpnserv/common.c
index dc47666..59f73bd 100644
--- a/src/openvpnserv/common.c
+++ b/src/openvpnserv/common.c
@@ -40,7 +40,7 @@ openvpn_vsntprintf(LPTSTR str, size_t size, LPCTSTR format, va_list arglist)
         len = _vsntprintf(str, size, format, arglist);
         str[size - 1] = 0;
     }
-    return (len >= 0 && len < size);
+    return (len >= 0 && (size_t)len < size);
 }
 int
 openvpn_sntprintf(LPTSTR str, size_t size, LPCTSTR format, ...)
diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index fbc32f9..66ffeec 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -188,7 +188,7 @@ typedef enum {
 static DWORD
 AsyncPipeOp(async_op_t op, HANDLE pipe, LPVOID buffer, DWORD size, DWORD count, LPHANDLE events)
 {
-    int i;
+    DWORD i;
     BOOL success;
     HANDLE io_event;
     DWORD res, bytes = 0;
@@ -934,7 +934,7 @@ static DWORD WINAPI
 RegisterDNS(LPVOID unused)
 {
     DWORD err;
-    DWORD i;
+    size_t i;
     WCHAR sys_path[MAX_PATH];
     DWORD timeout = RDNS_TIMEOUT * 1000; /* in milliseconds */
 
@@ -950,7 +950,6 @@ RegisterDNS(LPVOID unused)
         { ipcfg, L"ipconfig /flushdns",    timeout },
         { ipcfg, L"ipconfig /registerdns", timeout },
     };
-    int ncmds = sizeof(cmds) / sizeof(cmds[0]);
 
     HANDLE wait_handles[2] = {rdns_semaphore, exit_event};
 
@@ -963,7 +962,7 @@ RegisterDNS(LPVOID unused)
     if (WaitForMultipleObjects(2, wait_handles, FALSE, timeout) == WAIT_OBJECT_0)
     {
         /* Semaphore locked */
-        for (i = 0; i < ncmds; ++i)
+        for (i = 0; i < _countof(cmds); ++i)
         {
             ExecCommand(cmds[i].argv0, cmds[i].cmdline, cmds[i].timeout);
         }
diff --git a/src/openvpnserv/validate.c b/src/openvpnserv/validate.c
index 653bd12..26ffa8f 100644
--- a/src/openvpnserv/validate.c
+++ b/src/openvpnserv/validate.c
@@ -298,7 +298,7 @@ IsUserInGroup(PSID sid, const PTOKEN_GROUPS token_groups, const WCHAR *group_nam
             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)
         {
             ret = EqualSid(members[i].lgrmi0_sid, sid);
         }
-- 
2.9.0.windows.1

[Openvpn-devel] [PATCH applied] Re: Signed/unsigned warnings of MSVC resolved

[Gert Doering]

Acked-by: Gert Doering <gert@...1296...>

I went back and read the discussion we had on v1 of this patch, so we
already agreed on this changes (or we let Simon convince us :)) - we just 
asked for a v2 because parts of v1 ("return -err") got fixed in parallel 
already. 

Test compiled on ubuntu1604/mingw.

Your patch has been applied to the master branch.

commit f755c992915b25acd114ef98e61dd9eae7ff57fe
Author: Simon Rozman
Date:   Fri Apr 13 14:47:56 2018 +0200

     Signed/unsigned warnings of MSVC resolved

     Acked-by: Gert Doering <gert@...1296...>
     Message-Id: <20180413124756.5756-1-simon@...2293...>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16756.html
     Signed-off-by: Gert Doering <gert@...1296...>


--
kind regards,

Gert Doering