Introduce tapctl.exe utility and openvpnmsica.dll MSI CA

The tapctl.exe utility is a future replacement for the devcon.exe/ tapinstall.exe utility. While this utility does not offer TAP driver installation or upgrading, its purpose is to manipulate TAP virtual network interfaces on Windows. In the long term, its code could be integrated into openvpn.exe with --mktun and --rmtun.

The openvpnmsica.dll provides additional MSI custom actions for TUN/TAP interface creation on install. The interface creation is customizable using the TAPInterface MSI table and is fully compliant with MSI's deffered processing, commit and rollback. Detailed instruction and documentation is to be published when MSI packaging completed.

Those utilities were placed into openvpn repository to join the established compile-sign-package OpenVPN workflow.

---
 configure.ac                                  |    2 +
 openvpn.sln                                   |   20 +
 src/Makefile.am                               |    2 +-
 src/compat/Makefile.am                        |    5 +-
 src/openvpnmsica/Makefile.am                  |   55 +
 src/openvpnmsica/dllmain.c                    |  179 +++
 src/openvpnmsica/msica_op.c                   |  935 +++++++++++++++
 src/openvpnmsica/msica_op.h                   |  429 +++++++
 src/openvpnmsica/msiex.c                      |  205 ++++
 src/openvpnmsica/msiex.h                      |  111 ++
 src/openvpnmsica/openvpnmsica-Debug.props     |   14 +
 src/openvpnmsica/openvpnmsica-Release.props   |   14 +
 src/openvpnmsica/openvpnmsica.c               |  668 +++++++++++
 src/openvpnmsica/openvpnmsica.h               |   99 ++
 src/openvpnmsica/openvpnmsica.props           |   15 +
 src/openvpnmsica/openvpnmsica.vcxproj         |  142 +++
 src/openvpnmsica/openvpnmsica.vcxproj.filters |   62 +
 src/openvpnmsica/openvpnmsica_resources.rc    |   62 +
 src/tapctl/Makefile.am                        |   51 +
 src/tapctl/basic.h                            |   54 +
 src/tapctl/error.c                            |   35 +
 src/tapctl/error.h                            |   95 ++
 src/tapctl/main.c                             |  385 ++++++
 src/tapctl/tap.c                              | 1038 +++++++++++++++++
 src/tapctl/tap.h                              |  139 +++
 src/tapctl/tapctl.exe.manifest                |   10 +
 src/tapctl/tapctl.props                       |   18 +
 src/tapctl/tapctl.vcxproj                     |  145 +++
 src/tapctl/tapctl.vcxproj.filters             |   49 +
 src/tapctl/tapctl_resources.rc                |   64 +
 30 files changed, 5100 insertions(+), 2 deletions(-)
 create mode 100644 src/openvpnmsica/Makefile.am
 create mode 100644 src/openvpnmsica/dllmain.c
 create mode 100644 src/openvpnmsica/msica_op.c
 create mode 100644 src/openvpnmsica/msica_op.h
 create mode 100644 src/openvpnmsica/msiex.c
 create mode 100644 src/openvpnmsica/msiex.h
 create mode 100644 src/openvpnmsica/openvpnmsica-Debug.props
 create mode 100644 src/openvpnmsica/openvpnmsica-Release.props
 create mode 100644 src/openvpnmsica/openvpnmsica.c
 create mode 100644 src/openvpnmsica/openvpnmsica.h
 create mode 100644 src/openvpnmsica/openvpnmsica.props
 create mode 100644 src/openvpnmsica/openvpnmsica.vcxproj
 create mode 100644 src/openvpnmsica/openvpnmsica.vcxproj.filters
 create mode 100644 src/openvpnmsica/openvpnmsica_resources.rc
 create mode 100644 src/tapctl/Makefile.am
 create mode 100644 src/tapctl/basic.h
 create mode 100644 src/tapctl/error.c
 create mode 100644 src/tapctl/error.h
 create mode 100644 src/tapctl/main.c
 create mode 100644 src/tapctl/tap.c
 create mode 100644 src/tapctl/tap.h
 create mode 100644 src/tapctl/tapctl.exe.manifest
 create mode 100644 src/tapctl/tapctl.props
 create mode 100644 src/tapctl/tapctl.vcxproj
 create mode 100644 src/tapctl/tapctl.vcxproj.filters
 create mode 100644 src/tapctl/tapctl_resources.rc

index 399cdf4e..9cb754e4 100644
@@ -1399,10 +1399,12 @@ AC_CONFIG_FILES([
 	src/Makefile
 	src/compat/Makefile
 	src/openvpn/Makefile
+	src/openvpnmsica/Makefile
 	src/openvpnserv/Makefile
 	src/plugins/Makefile
 	src/plugins/auth-pam/Makefile
 	src/plugins/down-root/Makefile
+	src/tapctl/Makefile
 	tests/Makefile
         tests/unit_tests/Makefile
         tests/unit_tests/example_test/Makefile

index 51fdaf08..803f555d 100644
@@ -11,6 +11,10 @@ Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "msvc-generate", "build\msvc
 EndProject
 Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "compat", "src\compat\compat.vcxproj", "{4B2E2719-E661-45D7-9203-F6F456B22F19}"
 EndProject
+Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "tapctl", "src\tapctl\tapctl.vcxproj", "{A06436E7-D576-490D-8BA0-0751D920334A}"
+EndProject
+Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "openvpnmsica", "src\openvpnmsica\openvpnmsica.vcxproj", "{D41AA9D6-B818-476E-992E-0E16EB86BEE2}"
+EndProject
 Global
 	GlobalSection(SolutionConfigurationPlatforms) = preSolution
 		Debug|Win32 = Debug|Win32

@@ -51,6 +55,22 @@ Global
 		{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
+		{A06436E7-D576-490D-8BA0-0751D920334A}.Debug|Win32.ActiveCfg = Debug|Win32
+		{A06436E7-D576-490D-8BA0-0751D920334A}.Debug|Win32.Build.0 = Debug|Win32
+		{A06436E7-D576-490D-8BA0-0751D920334A}.Debug|x64.ActiveCfg = Debug|x64
+		{A06436E7-D576-490D-8BA0-0751D920334A}.Debug|x64.Build.0 = Debug|x64
+		{A06436E7-D576-490D-8BA0-0751D920334A}.Release|Win32.ActiveCfg = Release|Win32
+		{A06436E7-D576-490D-8BA0-0751D920334A}.Release|Win32.Build.0 = Release|Win32
+		{A06436E7-D576-490D-8BA0-0751D920334A}.Release|x64.ActiveCfg = Release|x64
+		{A06436E7-D576-490D-8BA0-0751D920334A}.Release|x64.Build.0 = Release|x64
+		{D41AA9D6-B818-476E-992E-0E16EB86BEE2}.Debug|Win32.ActiveCfg = Debug|Win32
+		{D41AA9D6-B818-476E-992E-0E16EB86BEE2}.Debug|Win32.Build.0 = Debug|Win32
+		{D41AA9D6-B818-476E-992E-0E16EB86BEE2}.Debug|x64.ActiveCfg = Debug|x64
+		{D41AA9D6-B818-476E-992E-0E16EB86BEE2}.Debug|x64.Build.0 = Debug|x64
+		{D41AA9D6-B818-476E-992E-0E16EB86BEE2}.Release|Win32.ActiveCfg = Release|Win32
+		{D41AA9D6-B818-476E-992E-0E16EB86BEE2}.Release|Win32.Build.0 = Release|Win32
+		{D41AA9D6-B818-476E-992E-0E16EB86BEE2}.Release|x64.ActiveCfg = Release|x64
+		{D41AA9D6-B818-476E-992E-0E16EB86BEE2}.Release|x64.Build.0 = Release|x64
 	EndGlobalSection
 	GlobalSection(SolutionProperties) = preSolution
 		HideSolutionNode = FALSE

index c7f63027..313d289f 100644
@@ -12,4 +12,4 @@
 MAINTAINERCLEANFILES = \
 	$(srcdir)/Makefile.in
 
-SUBDIRS = compat openvpn openvpnserv plugins
+SUBDIRS = compat openvpn openvpnmsica openvpnserv plugins tapctl

index b4c3a4aa..b51f661e 100644
@@ -14,7 +14,10 @@ MAINTAINERCLEANFILES = \
 
 EXTRA_DIST = \
 	compat.vcxproj \
-	compat.vcxproj.filters
+	compat.vcxproj.filters \
+	PropertySheet.props \
+	Debug.props \
+	Release.props
 
 noinst_LTLIBRARIES = libcompat.la
 

new file mode 100644
index 00000000..43532cfe
@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <ImportGroup Label="PropertySheets">
+    <Import Project="openvpnmsica.props" />
+  </ImportGroup>
+  <PropertyGroup Label="UserMacros" />
+  <PropertyGroup />
+  <ItemDefinitionGroup>
+    <ClCompile>
+      <RuntimeLibrary>MultiThreadedDebug</RuntimeLibrary>
+    </ClCompile>
+  </ItemDefinitionGroup>
+  <ItemGroup />
+</Project>
\ No newline at end of file

new file mode 100644
index 00000000..848fda8f
@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <ImportGroup Label="PropertySheets">
+    <Import Project="openvpnmsica.props" />
+  </ImportGroup>
+  <PropertyGroup Label="UserMacros" />
+  <PropertyGroup />
+  <ItemDefinitionGroup>
+    <ClCompile>
+      <RuntimeLibrary>MultiThreaded</RuntimeLibrary>
+    </ClCompile>
+  </ItemDefinitionGroup>
+  <ItemGroup />
+</Project>
\ No newline at end of file

new file mode 100644
index 00000000..0e31bc4f
@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <ImportGroup Label="PropertySheets" />
+  <PropertyGroup Label="UserMacros" />
+  <PropertyGroup />
+  <ItemDefinitionGroup>
+    <ClCompile>
+      <AdditionalIncludeDirectories>..\compat;$(TAP_WINDOWS_HOME)/include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+    </ClCompile>
+    <Link>
+      <SubSystem>Windows</SubSystem>
+    </Link>
+  </ItemDefinitionGroup>
+  <ItemGroup />
+</Project>
\ No newline at end of file

new file mode 100644
index 00000000..42ab6ccb

new file mode 100644
index 00000000..1eb5ea83

new file mode 100644
index 00000000..152954ed

2.19.0.windows.1

Part of the diff is not shown here. View the raw message.

On MinGW builds, the Libtool produces libopenvpnmsica.dll. The MSVC properties were updated to match this.

---
 src/openvpnmsica/openvpnmsica.props        | 4 +++-
 src/openvpnmsica/openvpnmsica_resources.rc | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

index 0e31bc4f..fa408296 100644
@@ -2,7 +2,9 @@
 <Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
   <ImportGroup Label="PropertySheets" />
   <PropertyGroup Label="UserMacros" />
-  <PropertyGroup />
+  <PropertyGroup>
+    <TargetName>lib$(ProjectName)</TargetName>
+  </PropertyGroup>
   <ItemDefinitionGroup>
     <ClCompile>
       <AdditionalIncludeDirectories>..\compat;$(TAP_WINDOWS_HOME)/include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>

index ce60b4a1..9e8069f4 100644
@@ -50,7 +50,7 @@ BEGIN
             VALUE "FileVersion", PACKAGE_VERSION ".0"
             VALUE "InternalName", "OpenVPN"
             VALUE "LegalCopyright", "Copyright © The OpenVPN Project" 
-            VALUE "OriginalFilename", "openvpnmsica.dll"
+            VALUE "OriginalFilename", "libopenvpnmsica.dll"
             VALUE "ProductName", "OpenVPN"
             VALUE "ProductVersion", PACKAGE_VERSION ".0"
         END

2.19.0.windows.1

Using extern "C" __declspec(dllexport) __stdcall, Win32 MSVC compiler exports the functions are as [email protected].... Exporting functions using /EXPORT linker flag allows us to specify exact function name.

Note: The 64-bit MSVC compiler does not exhibit __stdcall name- mangling.

---
 src/openvpnmsica/openvpnmsica.c | 12 ++++++++++++
 src/openvpnmsica/openvpnmsica.h | 14 +++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

index 82333991..3b90ce05 100644
@@ -145,6 +145,10 @@ openvpnmsica_setup_sequence_filename(
 UINT __stdcall
 FindTAPInterfaces(_In_ MSIHANDLE hInstall)
 {
+#ifdef _MSC_VER
+#pragma comment(linker, DLLEXP_EXPORT)
+#endif
+
 #ifdef _DEBUG
     MessageBox(NULL, TEXT("Attach debugger!"), TEXT(__FUNCTION__) TEXT(" v")  TEXT(PACKAGE_VERSION), MB_OK);
 #endif

@@ -247,6 +251,10 @@ cleanup_CoInitialize:
 UINT __stdcall
 EvaluateTAPInterfaces(_In_ MSIHANDLE hInstall)
 {
+#ifdef _MSC_VER
+#pragma comment(linker, DLLEXP_EXPORT)
+#endif
+
 #ifdef _DEBUG
     MessageBox(NULL, TEXT("Attach debugger!"), TEXT(__FUNCTION__) TEXT(" v")  TEXT(PACKAGE_VERSION), MB_OK);
 #endif

@@ -505,6 +513,10 @@ cleanup_exec_seq:
 UINT __stdcall
 ProcessDeferredAction(_In_ MSIHANDLE hInstall)
 {
+#ifdef _MSC_VER
+#pragma comment(linker, DLLEXP_EXPORT)
+#endif
+
 #ifdef _DEBUG
     MessageBox(NULL, TEXT("Attach debugger!"), TEXT(__FUNCTION__) TEXT(" v")  TEXT(PACKAGE_VERSION), MB_OK);
 #endif

index 3a64fbaa..bb8e28ec 100644
@@ -55,6 +55,14 @@ extern DWORD openvpnmsica_tlsidx_session;
 extern "C" {
 #endif
 
+#ifdef __GNUC__
+#define DLLEXP_DECL __declspec(dllexport)
+#else
+#define DLLEXP_DECL
+#define DLLEXP_EXPORT "/EXPORT:"__FUNCTION__"="__FUNCDNAME__
+#endif
+
+
 /**
  * Find existing TAP interfaces and set TAPINTERFACES property with semicolon delimited list
  * of installed TAP interface GUIDs.

@@ -64,7 +72,7 @@ extern "C" {
  * @return ERROR_SUCCESS on success; An error code otherwise
  *         See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa368072.aspx
  */
-__declspec(dllexport) UINT __stdcall
+DLLEXP_DECL UINT __stdcall
 FindTAPInterfaces(_In_ MSIHANDLE hInstall);
 
 

@@ -77,7 +85,7 @@ FindTAPInterfaces(_In_ MSIHANDLE hInstall);
  * @return ERROR_SUCCESS on success; An error code otherwise
  *         See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa368072.aspx
  */
-__declspec(dllexport) UINT __stdcall
+DLLEXP_DECL UINT __stdcall
 EvaluateTAPInterfaces(_In_ MSIHANDLE hInstall);
 
 

@@ -89,7 +97,7 @@ EvaluateTAPInterfaces(_In_ MSIHANDLE hInstall);
  * @return ERROR_SUCCESS on success; An error code otherwise
  *         See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa368072.aspx
  */
-__declspec(dllexport) UINT __stdcall
+DLLEXP_DECL UINT __stdcall
 ProcessDeferredAction(_In_ MSIHANDLE hInstall);
 
 #ifdef __cplusplus

2.19.0.windows.1

Hi Simon,

This approach keeps 'goes in DLL' next to the function itself, which I like. If you're interested, another possible approach here is to use .DEF files with MSVC, which can also do symbol aliasing:
https://docs.microsoft.com/en-us/cpp/build/exporting-from-a-dll-using-def-files

I would expect mingw to support .DEF files as well, but I don't know.

Only x86 Win32 functions are actually __stdcall; x64, ARM32, and ARM64 all ignore the token because the architectural calling convention makes it meaningless:
https://msdn.microsoft.com/en-us/library/zxk0tw93.aspx

This is why the names don't get mangled. Of course, it's then inconsistent.

Export name consistency is needed so CustomActions can reference the export symbol name, right?

Jon

-----Original Message-----
From: Simon Rozman <[email protected]...>
Sent: Tuesday, October 16, 2018 3:26 AM
To: [email protected]
Subject: [Openvpn-devel] [PATCH 2/5] Prevent __stdcall name mangling of MSVC

Using extern "C" __declspec(dllexport) __stdcall, Win32 MSVC compiler exports the functions are as [email protected].... Exporting functions using /EXPORT linker flag allows us to specify exact function name.

Note: The 64-bit MSVC compiler does not exhibit __stdcall name- mangling.

---
 src/openvpnmsica/openvpnmsica.c | 12 ++++++++++++
 src/openvpnmsica/openvpnmsica.h | 14 +++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

index 82333991..3b90ce05 100644
@@ -145,6 +145,10 @@ openvpnmsica_setup_sequence_filename(
 UINT __stdcall
 FindTAPInterfaces(_In_ MSIHANDLE hInstall)
 {
+#ifdef _MSC_VER
+#pragma comment(linker, DLLEXP_EXPORT)
+#endif
+
 #ifdef _DEBUG
     MessageBox(NULL, TEXT("Attach debugger!"), TEXT(__FUNCTION__) TEXT(" v")  TEXT(PACKAGE_VERSION), MB_OK);
 #endif

@@ -247,6 +251,10 @@ cleanup_CoInitialize:
 UINT __stdcall
 EvaluateTAPInterfaces(_In_ MSIHANDLE hInstall)
 {
+#ifdef _MSC_VER
+#pragma comment(linker, DLLEXP_EXPORT)
+#endif
+
 #ifdef _DEBUG
     MessageBox(NULL, TEXT("Attach debugger!"), TEXT(__FUNCTION__) TEXT(" v")  TEXT(PACKAGE_VERSION), MB_OK);
 #endif

@@ -505,6 +513,10 @@ cleanup_exec_seq:
 UINT __stdcall
 ProcessDeferredAction(_In_ MSIHANDLE hInstall)
 {
+#ifdef _MSC_VER
+#pragma comment(linker, DLLEXP_EXPORT)
+#endif
+
 #ifdef _DEBUG
     MessageBox(NULL, TEXT("Attach debugger!"), TEXT(__FUNCTION__) TEXT(" v")  TEXT(PACKAGE_VERSION), MB_OK);
 #endif

index 3a64fbaa..bb8e28ec 100644
@@ -55,6 +55,14 @@ extern DWORD openvpnmsica_tlsidx_session;
 extern "C" {
 #endif
 
+#ifdef __GNUC__
+#define DLLEXP_DECL __declspec(dllexport)
+#else
+#define DLLEXP_DECL
+#define DLLEXP_EXPORT "/EXPORT:"__FUNCTION__"="__FUNCDNAME__
+#endif
+
+
 /**
  * Find existing TAP interfaces and set TAPINTERFACES property with semicolon delimited list
  * of installed TAP interface GUIDs.

@@ -64,7 +72,7 @@ extern "C" {
  * @return ERROR_SUCCESS on success; An error code otherwise
  *         See: https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmsdn.microsoft.com%2Fen-us%2Flibrary%2Fwindows%2Fdesktop%2Faa368072.aspx&data=02%7C01%7Cjkunkee%40microsoft.com%7C726e9ae5072942c989bf08d63351ec71%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636752824301952450&sdata=BXt41C1200ZAAQBQBS7vOiAZ3MzuQGCVHaUuFhYwMCk%3D&reserved=0
  */
-__declspec(dllexport) UINT __stdcall
+DLLEXP_DECL UINT __stdcall
 FindTAPInterfaces(_In_ MSIHANDLE hInstall);
 
 

@@ -77,7 +85,7 @@ FindTAPInterfaces(_In_ MSIHANDLE hInstall);
  * @return ERROR_SUCCESS on success; An error code otherwise
  *         See: https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmsdn.microsoft.com%2Fen-us%2Flibrary%2Fwindows%2Fdesktop%2Faa368072.aspx&data=02%7C01%7Cjkunkee%40microsoft.com%7C726e9ae5072942c989bf08d63351ec71%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636752824301962454&sdata=vMBtU2AQPrfZ9xnbN4nYZfcAb2wNlIxLPqqYLwDtvUc%3D&reserved=0
  */
-__declspec(dllexport) UINT __stdcall
+DLLEXP_DECL UINT __stdcall
 EvaluateTAPInterfaces(_In_ MSIHANDLE hInstall);
 
 

@@ -89,7 +97,7 @@ EvaluateTAPInterfaces(_In_ MSIHANDLE hInstall);
  * @return ERROR_SUCCESS on success; An error code otherwise
  *         See: https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmsdn.microsoft.com%2Fen-us%2Flibrary%2Fwindows%2Fdesktop%2Faa368072.aspx&data=02%7C01%7Cjkunkee%40microsoft.com%7C726e9ae5072942c989bf08d63351ec71%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636752824301962454&sdata=vMBtU2AQPrfZ9xnbN4nYZfcAb2wNlIxLPqqYLwDtvUc%3D&reserved=0
  */
-__declspec(dllexport) UINT __stdcall
+DLLEXP_DECL UINT __stdcall
 ProcessDeferredAction(_In_ MSIHANDLE hInstall);
 
 #ifdef __cplusplus

2.19.0.windows.1



_______________________________________________
Openvpn-devel mailing list
[email protected]
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.sourceforge.net%2Flists%2Flistinfo%2Fopenvpn-devel&data=02%7C01%7Cjkunkee%40microsoft.com%7C726e9ae5072942c989bf08d63351ec71%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636752824301962454&sdata=mUAulpo9rK%2BlLjtrCmPGlSXWplIoifrSQcC7wCokw7E%3D&reserved=0

Hi Jon,

I did use .DEF file initially. However, when adopting the project to build with MinGW too, my limited knowledge in authoring Linux build systems didn't find a way to use .DEF files there. After some research, I have put the export declarations into the source code instead. It's ugly but it keeps both building systems happy and produces consistent DLL file.

Exactly. That extra effort to prevent name mangling when compiling x86 DLL file pays off later when authoring MSI package. Otherwise, I would have had to introduce some "#if-s" in WiX to address different platform-specific function names.

I had to pick my poison, and I choose to fix issues as close to the source as possible.

Regards,
Simon

Hi Jon,

Fantastic. I'm sorry there isn't a more elegant solution, but I'm glad there's something that works.

Understood. Thanks for the explanation.

Samuli, LGTM.

Jon

Your patch has been applied to the master branch.

"LGTM" from Jon (plus patch and explanation seem to make sense :-) )

commit 7d08c33cfc58eeb7286446c8d1ffd02939782332

Author: Simon Rozman
Date:   Tue Oct 16 12:26:24 2018 +0200

Prevent __stdcall name mangling of MSVC

     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17765.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

This makes MSVC and MinGW build environments more alike.

---
 src/openvpnmsica/openvpnmsica.props | 1 +
 src/tapctl/tapctl.props             | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

index fa408296..074635d0 100644
@@ -8,6 +8,7 @@
   <ItemDefinitionGroup>
     <ClCompile>
       <AdditionalIncludeDirectories>..\compat;$(TAP_WINDOWS_HOME)/include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <PreprocessorDefinitions>_WIN32_WINNT=_WIN32_WINNT_VISTA;%(PreprocessorDefinitions)</PreprocessorDefinitions>
     </ClCompile>
     <Link>
       <SubSystem>Windows</SubSystem>

index 152954ed..0257b9ff 100644
@@ -7,7 +7,7 @@
   </PropertyGroup>
   <ItemDefinitionGroup>
     <ClCompile>
-      <PreprocessorDefinitions>_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions>_CONSOLE;_WIN32_WINNT=_WIN32_WINNT_VISTA;%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <AdditionalIncludeDirectories>..\compat;$(TAP_WINDOWS_HOME)/include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
     </ClCompile>
     <Link>

2.19.0.windows.1

LGTM, Samuli.

-----Original Message-----
From: Simon Rozman <[email protected]...>
Sent: Tuesday, October 16, 2018 3:26 AM
To: [email protected]
Subject: [Openvpn-devel] [PATCH 3/5] Define _WIN32_WINNT=_WIN32_WINNT_VISTA in MSVC

This makes MSVC and MinGW build environments more alike.

---
 src/openvpnmsica/openvpnmsica.props | 1 +
 src/tapctl/tapctl.props             | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

index fa408296..074635d0 100644
@@ -8,6 +8,7 @@
   <ItemDefinitionGroup>
     <ClCompile>
       <AdditionalIncludeDirectories>..\compat;$(TAP_WINDOWS_HOME)/include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <PreprocessorDefinitions>_WIN32_WINNT=_WIN32_WINNT_VISTA;%(PreprocessorDefinitions)</PreprocessorDefinitions>
     </ClCompile>
     <Link>
       <SubSystem>Windows</SubSystem>

index 152954ed..0257b9ff 100644
@@ -7,7 +7,7 @@
   </PropertyGroup>
   <ItemDefinitionGroup>
     <ClCompile>
-      <PreprocessorDefinitions>_CONSOLE;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <PreprocessorDefinitions>_CONSOLE;_WIN32_WINNT=_WIN32_WINNT_VISTA;%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <AdditionalIncludeDirectories>..\compat;$(TAP_WINDOWS_HOME)/include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
     </ClCompile>
     <Link>

2.19.0.windows.1



_______________________________________________
Openvpn-devel mailing list
[email protected]
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.sourceforge.net%2Flists%2Flistinfo%2Fopenvpn-devel&data=02%7C01%7Cjkunkee%40microsoft.com%7Cd4c1e876c17c480297d208d63351eb3e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636752824279734760&sdata=VNabZb0XFntX44P8XdBZdXbEkFUTr3md02hzQ%2Bto0Yo%3D&reserved=0

Your patch has been applied to the master branch.

LGTM from Jon.

commit fc74ac1e82b6727454f12580d52cf8cf5d80e503

Author: Simon Rozman
Date:   Tue Oct 16 12:26:25 2018 +0200

Define _WIN32_WINNT=_WIN32_WINNT_VISTA in MSVC

     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17764.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

This patch introduces a FindSystemInfo() MSI custom action to reliably detect Windows 10. The MSI built-in properties for Windows version detection depend on bootstrapper's manifest. We could provide our own Windows 10 compatible EXE bootstrapper, but that would cover the Windows 10 detection in the InstallUISequence only. The InstallExecuteSequence is launched by msiexec.exe which we cannot tamper with would still report VersionNT as Windows 8 (603).

---
 src/openvpnmsica/Makefile.am    |   2 +-
 src/openvpnmsica/openvpnmsica.c | 124 ++++++++++++++++++++++++++++++--
 src/openvpnmsica/openvpnmsica.h |  15 ++++
 3 files changed, 136 insertions(+), 5 deletions(-)

index d46170b4..ecca74bc 100644
@@ -41,7 +41,7 @@ libopenvpnmsica_la_CFLAGS = \
 	-municode -D_UNICODE \
 	-UNTDDI_VERSION -U_WIN32_WINNT \
 	-D_WIN32_WINNT=_WIN32_WINNT_VISTA
-libopenvpnmsica_la_LDFLAGS = -ladvapi32 -lole32 -lmsi -lsetupapi -lshlwapi -no-undefined -avoid-version
+libopenvpnmsica_la_LDFLAGS = -ladvapi32 -lole32 -lmsi -lsetupapi -lshlwapi -lversion -no-undefined -avoid-version
 endif
 
 libopenvpnmsica_la_SOURCES = \

index bb8e28ec..da145062 100644
@@ -63,6 +63,21 @@ extern "C" {
 #endif
 
 
+/**
+ * Determines Windows information:
+ * - Sets `DriverCertification` MSI property to "", "attsgn" or "whql"
+ *   according to the driver certification required by the running version of
+ *   Windows.
+ *
+ * @param hInstall      Handle to the installation provided to the DLL custom action
+ *
+ * @return ERROR_SUCCESS on success; An error code otherwise
+ *         See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa368072.aspx
+ */
+DLLEXP_DECL UINT __stdcall
+FindSystemInfo(_In_ MSIHANDLE hInstall);
+
+
 /**
  * Find existing TAP interfaces and set TAPINTERFACES property with semicolon delimited list
  * of installed TAP interface GUIDs.

2.19.0.windows.1

Hi Simon,

This is painful to read, and I bet it was even more painful to write. I am sorry it came to this, though it does look like it should(TM) work.

From what I gather, the OS version should be checked in the CustomAction's MSI execution conditions instead of in the CustomAction itself. The appropriate information can then be passed in based on that:

https://blogs.msdn.microsoft.com/cjacks/2009/05/06/why-custom-actions-get-a-windows-vista-version-lie-on-windows-7/

It might be somewhat more convenient to add the PID to the debug MessageBox call, but it is probably MUCH more convenient to use the CustomAction debugging facility built into the MSI service itself:

https://docs.microsoft.com/en-us/windows/desktop/Msi/debugging-custom-actions

Thanks,
Jon

-----Original Message-----
From: Simon Rozman <[email protected]...>
Sent: Tuesday, October 16, 2018 3:26 AM
To: [email protected]
Subject: [Openvpn-devel] [PATCH 4/5] Add MSI custom action for reliable Windows 10 detection

This patch introduces a FindSystemInfo() MSI custom action to reliably detect Windows 10. The MSI built-in properties for Windows version detection depend on bootstrapper's manifest. We could provide our own Windows 10 compatible EXE bootstrapper, but that would cover the Windows 10 detection in the InstallUISequence only. The InstallExecuteSequence is launched by msiexec.exe which we cannot tamper with would still report VersionNT as Windows 8 (603).

---
 src/openvpnmsica/Makefile.am    |   2 +-
 src/openvpnmsica/openvpnmsica.c | 124 ++++++++++++++++++++++++++++++--
 src/openvpnmsica/openvpnmsica.h |  15 ++++
 3 files changed, 136 insertions(+), 5 deletions(-)

index d46170b4..ecca74bc 100644
@@ -41,7 +41,7 @@ libopenvpnmsica_la_CFLAGS = \
 	-municode -D_UNICODE \
 	-UNTDDI_VERSION -U_WIN32_WINNT \
 	-D_WIN32_WINNT=_WIN32_WINNT_VISTA
-libopenvpnmsica_la_LDFLAGS = -ladvapi32 -lole32 -lmsi -lsetupapi -lshlwapi -no-undefined -avoid-version
+libopenvpnmsica_la_LDFLAGS = -ladvapi32 -lole32 -lmsi -lsetupapi -lshlwapi -lversion -no-undefined -avoid-version
 endif
 
 libopenvpnmsica_la_SOURCES = \

index bb8e28ec..da145062 100644
@@ -63,6 +63,21 @@ extern "C" {
 #endif
 
 
+/**
+ * Determines Windows information:
+ * - Sets `DriverCertification` MSI property to "", "attsgn" or "whql"
+ *   according to the driver certification required by the running version of
+ *   Windows.
+ *
+ * @param hInstall      Handle to the installation provided to the DLL custom action
+ *
+ * @return ERROR_SUCCESS on success; An error code otherwise
+ *         See: https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmsdn.microsoft.com%2Fen-us%2Flibrary%2Fwindows%2Fdesktop%2Faa368072.aspx&data=02%7C01%7Cjkunkee%40microsoft.com%7Cd520a31814634dced1dc08d63351eb58%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636752824275192379&sdata=2DLeMsL5JHIFmh07Fh6zqZTGt117xy%2Bu2TY3acNpaH8%3D&reserved=0
+ */
+DLLEXP_DECL UINT __stdcall
+FindSystemInfo(_In_ MSIHANDLE hInstall);
+
+
 /**
  * Find existing TAP interfaces and set TAPINTERFACES property with semicolon delimited list
  * of installed TAP interface GUIDs.

2.19.0.windows.1



_______________________________________________
Openvpn-devel mailing list
[email protected]
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.sourceforge.net%2Flists%2Flistinfo%2Fopenvpn-devel&data=02%7C01%7Cjkunkee%40microsoft.com%7Cd520a31814634dced1dc08d63351eb58%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636752824275192379&sdata=Upy1p7tESRCzuEPIBUpkxr3YnZpFFrr8zpQybDyaY1Q%3D&reserved=0

Hi,

I don't blame Microsoft for this mess. Version lies actually solve a lot of problems with legacy software.

The post on this link is true for Windows 7 (May 6, 2009). It's 2018 now and MSI's property VersionNT got stuck on Windows 8 (602 if I recall correctly). Even when run on Windows 10. Each time Microsoft ceils the version mechanism, they should have introduced a new one: e.g. Version9X >> VersionNT >> Version81. This would keep the legacy MSI packages that are using VersionNT think they're on Windows 8, and allow the new packages to test for Windows 10 in a clean way.

It would have been a lot easier to just have an MSI condition (e.g. install WHQL flavour of driver if Version81 AND Version81>=1000 AND MsiNTProductType>=2).

I am open to suggestions, how to make a "regular/attestation signed/WHQL" selection logic in the MSI. MSI packages have up to three driver flavours packed and only one must be installed:
- x86 MSI has regular+attestation signed
- x64 has regular+attestation+WHQL
- ARM64 will have attestation signed only

Many thanks for this. I will test the MsiBreak method shortly and remove the MessageBox calls completely for a cleaner code then.

Regards,
Simon

Quite right. I just talked to someone familiar with this here, and, as I understand it, MSI will never offer a way to do this Right on Windows 10. I'm not sure what the general messaging is from Microsoft around MSI, but my personal perception is that it is not moving forward anymore.

Fortunately, I *do* have a suggestion here...I think.

First, though, let's make sure I'm thinking straight: In your new system, there is an EXE that runs, detects the architecture, unpacks the right MSI, and runs the right MSI, right?

*If* I have that right, the EXE could be manifested as Windows-10-aware:

Manifest contents:
https://docs.microsoft.com/en-us/windows/desktop/SysInfo/targeting-your-application-at-windows-8-1
VS workflow:
https://docs.microsoft.com/en-us/cpp/build/how-to-embed-a-manifest-inside-a-c-cpp-application

Basically, you write an XML file and pack it in the EXE as a resource. Visual Studio will do this for you if the file is in your VS project source file list. I'm not sure how you'd do this with mingw_w64, but you'd probably invoke the equivalent of mt.exe:

https://stackoverflow.com/questions/1423492/how-do-i-add-a-manifest-to-an-executable-using-mt-exe

The parent installer could then call GetVersionEx without being lied to and pass it in to your CustomAction DLL through msiexec:

https://www.codeproject.com/articles/16767/how-to-pass-command-line-arguments-to-msi-installe

Let me know if that doesn't make sense or won't work for what you're doing.

Thanks,
Jon

Hi,

:( I agree it's an art to make an MSI, but being a known and documented standard, supporting commit/rollback, allowing sys-admins endless customizations of packages... (If only Microsoft offered more stock actions.) Well, I guess Microsoft is coming up with some awesome replacement in due time. :)

Note a side take-home message: msiexec.exe will never be Windows 10 aware.

I am familiar with EXE and manifests, thanks. Nevertheless, I really appreciate your time to extensively research and provide useful references in the debate.

In my professional career I make EXE (or WSH) "parent installers" only because end-users usually have no clue what platform is their Windows. You can't do one-MSI-fits-all-platforms for native apps, so the parent installer EXE takes platform choice away from users. All other decision logic is inside MSI packages making them fully autonomous - once you pick the right one for your platform of course.

Even if we do make a Windows 10 aware "parent installer" to inject DriverCertification property to the MSI, we get complications in one considerable use case...

<SideNote>
Actually, I wouldn't be on this boat at all if I wasn't interested into Group Policy deployment of OpenVPN professionally. I work for a SOHO-sized company with zero budget to run anything more than Group Policy MSI deploys, and I am totally fed with running from one employee to another to assist them with keeping OpenVPN up-to-date, since OpenVPN only has an EXE installer for the time being. Therefore, I was thinking of repackaging OpenVPN into MSI for my own needs, but later learned that OpenVPN community is interested in it too.
</SideNote>

When the MSI package is installed by Group Policy Client, the MSI is launched directly by msiexec.exe. There is no parent installer EXE in between. Therefore, the MSI command line parameter which TAP driver to install would need to be authored by sys-admins. This would make Group Policy deployment of OpenVPN more complicated on sys-admins as it should be.

Hence my design choice to select the TAP driver within MSI itself.

I would have used the VersionNT property, but it says "I am Windows 8" on Windows 10. I would have used a custom action and MSDN recommended Win32 API for Windows version detection, but when launched by Group Policy Client the parent process is msiexec.exe (which doesn't manifest as Windows 10 aware, remember take-home message above) thus the API says "I am Windows 8" on Windows 10 again.

Hence such a complex workaround to detect the MSI has been launched on Windows 10. Microsoft made me do it. ;)

Please, note one last thing... I totally agree with everybody saying "you shall not test for OS version, but on feature presence instead". That's why this custom action *does not* return the Windows version - not to tempt anybody else to use it for OS version detection. The FindSystemInfo custom action's only output is the "DriverCertification" property that is set to "", "attsgn", or "whql". The name "FindSystemInfo" was picked generic on purpose, to allow us to add other detections missing in MSI should we need any down the road.

After all this, I realized I mistitled the patch. It should have read "Add MSI custom action for reliable driver flavour selection".

Best regards,
Simon

Unfortunately, yes.

It looks like I erred widely on the side of overinforming. I appreciate your patience with that.

Makes sense. Fortunately x86 EXEs run on ARM64, so this even works there. :)

Ah, I see. That puts a bit of a damper in bolting intelligence on during the msiexec invocation, doesn't it?

I'm no expert with GP, and what little I know suggests that any methods for deploying custom EXEs through it are either much more complex than is feasible for you to support or otherwise not worth using. You're the expert between the two of us.

Without this ability and with the compat shimming of the MSI infrastructure, there's not much else to be done.

:)

Smart! I hadn't followed the function to its callers, so I missed this.

Samuli, LGTM.

Thanks,
Jon

Hi,

I gave the MsiBreak method a quick try, but I couldn't make it work. I have set MsiBreak as a system environment variable, restarted Windows Installer service, restarted the shell window from where I invoke msiexec calls to make sure the environment is updated. But it doesn't pop-up any prompt to attach debugger as advertised.

I haven't installed the Debugging Tools for Windows, as I run Visual Studio elevated to debug processes. I haven't restarted my computer: restart of a working machine with stage set to debug something is a royal PITA: the libopenvpnmsica.dll has more than one custom property to debug, restarting each time to switch MsiBreak to a different custom action is not viable.

Therefore, I'd prefer to keep own MessageBox() call in the beginning of each custom action. It works 100%.

Adding PID to the message is an option. I personally never needed it. When running Visual Studio elevated, you press Ctrl+Alt+P, followed by "msi" keystrokes to focus on "msiexec.exe" processes, then observe the MessageBox()'s title (which is deliberately set to function name) in the "Title" column of the available process list. Way faster than searching process by PID.

Anyway, I have extended the debug pop-up dialogs to be more informative and include PID. Patch follows...

Best regards,
Simon

That's sad to hear. I'd toss out ideas to try, but...

I've debugged using MsiBreak set in the admin CMD prompt I used to launch the MSI, so restarts aren't required. The article goes into some very nearly useful details about when this is, but...

...not only that, but the article even suggests this:

"To start debugging without MsiBreak, put a temporary message box at the beginning of the action's code. When the message box appears during the installation, attach the debugger to the process owning the message box."

If you ever need to debug say, the DLL loading, then MsiBreak is the way to go. As it stands, it's *REALLY* hard to beat 100%. :)

I've been wrong before about how easy different options are. :)

I took a look and it looks good to me, though I agree it's not strictly necessary.

Thanks,
Jon

Your patch has been applied to the master branch.

"LGTM" from Jon after quite some discussion. I'm not trying to understand the intricacies of the code, just some basic sanity checking ("no chance this will scribble over a registry entry or execute unwanted binaries by means of an uninitialized buffer").

commit e2faae87b49379ce8bcffb2f0ae67b0671996f64

Author: Simon Rozman
Date:   Tue Oct 16 12:26:26 2018 +0200

Add MSI custom action for reliable Windows 10 detection

     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17763.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

This patch extends the TAP interface enumerating to detect the TAP interfaces registered using "root\tap0901" hardware ID. Before, only TAP interfaces with legacy "tap0901" HWID were detected by openvpn.exe.

The openvpnmsica.dll and tapctl.exe install TAP interfaces using root- enumerated HWIDs, and were not detected by openvpn.exe.

---
 src/openvpn/tun.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 948fd17d..5fde2ab8 100644
@@ -3524,7 +3524,8 @@ get_tap_reg(struct gc_arena *gc)
 
                 if (status == ERROR_SUCCESS && data_type == REG_SZ)
                 {
-                    if (!strcmp(component_id, TAP_WIN_COMPONENT_ID))
+                    if (!strcmp(component_id, TAP_WIN_COMPONENT_ID) ||
+                        !strcmp(component_id, "root\\" TAP_WIN_COMPONENT_ID))
                     {
                         struct tap_reg *reg;
                         ALLOC_OBJ_CLEAR_GC(reg, struct tap_reg, gc);

2.19.0.windows.1

Samuli, LGTM.

-----Original Message-----
From: Simon Rozman <[email protected]...>
Sent: Tuesday, October 16, 2018 3:26 AM
To: [email protected]
Subject: [Openvpn-devel] [PATCH 5/5] Detect TAP interfaces with root-enumerated hardware ID

This patch extends the TAP interface enumerating to detect the TAP interfaces registered using "root\tap0901" hardware ID. Before, only TAP interfaces with legacy "tap0901" HWID were detected by openvpn.exe.

The openvpnmsica.dll and tapctl.exe install TAP interfaces using root- enumerated HWIDs, and were not detected by openvpn.exe.

---
 src/openvpn/tun.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 948fd17d..5fde2ab8 100644
@@ -3524,7 +3524,8 @@ get_tap_reg(struct gc_arena *gc)
 
                 if (status == ERROR_SUCCESS && data_type == REG_SZ)
                 {
-                    if (!strcmp(component_id, TAP_WIN_COMPONENT_ID))
+                    if (!strcmp(component_id, TAP_WIN_COMPONENT_ID) ||
+                        !strcmp(component_id, "root\\" TAP_WIN_COMPONENT_ID))
                     {
                         struct tap_reg *reg;
                         ALLOC_OBJ_CLEAR_GC(reg, struct tap_reg, gc);

2.19.0.windows.1



_______________________________________________
Openvpn-devel mailing list
[email protected]
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.sourceforge.net%2Flists%2Flistinfo%2Fopenvpn-devel&data=02%7C01%7Cjkunkee%40microsoft.com%7C4f51c53f4856475e3cab08d63351eb89%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636752824282687252&sdata=RkqbvXw2zxm00Np2Bb9C%2BeQstsTHkOHw25XtTyZYDII%3D&reserved=0

Hi,

I do not see a specific reason to *not* apply this patch, but I'm wondering if you could shed some background light on the changed behaviour in openvpnmsica.dll and tapctl.exe?

I'm not sure if we're going to care for someone who uses a 2.5 msi installer to set up a TAP interface and then runs a 2.3 openvpn.exe on top of it and wonders why it isn't finding the TAP driver - but the question might come up, and I like having good answers... :-)

gert

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...

Hi,

The tapctl.exe and openvpnmsica.dll use a simplified interface installation compared to devcon.exe/tapinstall.exe. They assume the driver is already installed and they don't require INF file to create a TAP interface. However, when the interface is installed this way, Windows reports its hardware ID as "root\\tap0901". Whereas, tapinstall.exe installed TAP interfaces report their HWID as "tap0901". That's about the only difference I noticed.

(Tested with driver versions 9.21.x and later.)

Rather than exploring, what causes the difference and fuelled by Microsoft's recommendation to use root-enumerated HWIDs anyway, I proposed to extend the openvpn.exe's TAP interface detection to accept both: "tap0901" _and_ "root\\tap0901" interfaces.

Suggest them to remove the TAP interface and reinstall it using tapinstall.exe. The tapinstall.exe should still be able to install 9.22 and later TUN interfaces.

Or even better: apply this patch to 2.4 and 2.3 branches (it's a really minor change) and ask them to compile and use the patched version of 2.3. :)

Best regards,
Simon

Hi,

O-kay... :-) - I do not know enough about driver *installation* on Windows to know whether this is all good or there might be dragons lurking. We'll have to test quite a bit, it seems :-)

Works for me. Which is why I ...

... applied it to 2.4 as well.

If someone struggles with 2.3 on windows (beyond XP), my empathy for them will be close to zero. 2.4 is so much better, especially on windows, that there is no reason to stick to 2.3... (on Linux distributions, sticking to the "distro provided package" might give people 2.3.x for years to come :-( - but this is no argument on Windows).

gert

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...

We have a LGTM from Jon as well, but I'm taking this on me - because I decided I want this in 2.4 as well. Unlikely as it may be, we *will* have people that happen to have a tap adapter on their system and try to run 2.4.x binaries on it... so this is long-term compatibility.

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

commit 6e03336d8a4aada12c4950a9683a483470fe4f15 (master) commit eef040cc909bb0e9bae793b61c2f4f0da7e109d5 (release/2.4)

Author: Simon Rozman
Date:   Tue Oct 16 12:26:27 2018 +0200

     Detect TAP interfaces with root-enumerated hardware ID

     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17762.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Hi,

I build-tested with all of these patches using the openvpn-build VM in Vagrant[1]. After the build libopenvpnmsica.dll and tapctl.exe show up under tmp/installer/<arch>. So the build part is definitely working correctly.

How would I test that both of the above are operating as intended?

Also, could a (Windows-oriented) person do a code review?

Samuli Seppänen
Community Manager
OpenVPN Technologies, Inc

irc freenode net: mattock

[1] https://github.com/OpenVPN/openvpn-vagrant


Il 16/10/18 13:26, Simon Rozman ha scritto:

Hi,

For the libopenvpnmsica.dll, testing could have been introduced using some unit test suite. Unfortunately, it so tightly integrates with Microsoft Installer, I see no feasible way of mocking it. The way I test is to use debug version inside MSI package. The debug version raises MessageBox() on each custom action function invocation. This allows me to use elevated Visual Studio, locate the msiexec.exe process, attach on it using remote debugging, set breakpoints, then click "Ok" in the MessageBox() pop-up to let it run up to the first breakpoint. If somebody knows of a better way to test and debug MSI custom actions I would be most happy to learn it.

As for the tapctl.exe: this utility is already usable with existing TAP-Windows 9.21.2. Just open an elevated command line and play with it on a Windows computer that has TAP drivers installed.

Best regards,
Simon

This looks pretty straightforward to me.

-----Original Message-----
From: Simon Rozman <[email protected]...>
Sent: Tuesday, October 16, 2018 3:26 AM
To: [email protected]
Subject: [Openvpn-devel] [PATCH 1/5] Set output name to libopenvpnmsica.dll in MSVC builds too

On MinGW builds, the Libtool produces libopenvpnmsica.dll. The MSVC properties were updated to match this.

---
 src/openvpnmsica/openvpnmsica.props        | 4 +++-
 src/openvpnmsica/openvpnmsica_resources.rc | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

index 0e31bc4f..fa408296 100644
@@ -2,7 +2,9 @@
 <Project ToolsVersion="4.0" xmlns="https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fschemas.microsoft.com%2Fdeveloper%2Fmsbuild%2F2003&amp;data=02%7C01%7Cjkunkee%40microsoft.com%7C6dd3259ae2044b779b9a08d63351edc6%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636752824311161400&amp;sdata=grtBK2tQpY7ZYBNWI%2FaHTJj7RP8JHQ0v%2BaoX1YwLi6o%3D&amp;reserved=0">
   <ImportGroup Label="PropertySheets" />
   <PropertyGroup Label="UserMacros" />
-  <PropertyGroup />
+  <PropertyGroup>
+    <TargetName>lib$(ProjectName)</TargetName>
+  </PropertyGroup>
   <ItemDefinitionGroup>
     <ClCompile>
       <AdditionalIncludeDirectories>..\compat;$(TAP_WINDOWS_HOME)/include;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>

index ce60b4a1..9e8069f4 100644
@@ -50,7 +50,7 @@ BEGIN
             VALUE "FileVersion", PACKAGE_VERSION ".0"
             VALUE "InternalName", "OpenVPN"
             VALUE "LegalCopyright", "Copyright © The OpenVPN Project" 
-            VALUE "OriginalFilename", "openvpnmsica.dll"
+            VALUE "OriginalFilename", "libopenvpnmsica.dll"
             VALUE "ProductName", "OpenVPN"
             VALUE "ProductVersion", PACKAGE_VERSION ".0"
         END

2.19.0.windows.1



_______________________________________________
Openvpn-devel mailing list
[email protected]
https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.sourceforge.net%2Flists%2Flistinfo%2Fopenvpn-devel&data=02%7C01%7Cjkunkee%40microsoft.com%7C6dd3259ae2044b779b9a08d63351edc6%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636752824311161400&sdata=6w%2FKO94p9LAx0%2FlamH4McJrCbJnDPwi%2F%2B6PDA6Ax4vY%3D&reserved=0

Taking Jon's "this looks pretty straightforward to me" as an ACK (plus, it really is, and Samuli has also tested this already :-) ).

Your patch has been applied to the master branch.

commit 864f95c371813a2cd07b886cdc8696cc8b732ee0

Author: Simon Rozman
Date:   Tue Oct 16 12:26:23 2018 +0200

Set output name to libopenvpnmsica.dll in MSVC builds too

     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg17766.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

This is not a truly formal windows code review, but I'm taking this in as the resulting builds have been tested by various people and seem to work well, and we have so many patches on top of this already queued that patchwork needs to go to multipage display...

(I *have* glanced over the code to see that there isn't anything that looks "suspicious" - and of course it does not modify openvpn code, so there is no risk for introducing remote exploits, crypto breaches, etc.)

I found a few things I'd like to see cleaned up eventually...

+            /* Create TLS data. */
+            struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data*)malloc(sizeof(struct openvpnmsica_tls_data));
+            memset(s, 0, sizeof(struct openvpnmsica_tls_data));

... this could be done with calloc() to allocate-and-clear, but it certainly needs a NULL ptr check before using "s"...

(As a side note: the use of "TLS" for "thread local storage" is somewhat confusing when you've been staring at TLS = "transport layer security" before that)

+ /* Prepare the message record. The record will contain up to four fields. *
/
+ MSIHANDLE hRecordProg = MsiCreateRecord(4);
+
+ {
+ /* Field 2: The message string. */
+ char szBufStack[128];

do we still need extra nesting just to get a "local" variable declaration? I thought we can have & use C99 everywhere today, which avoids this extra nesting level (in FindTAPInterfaces(), no extra brackets are needed).

+    /* Create and fill operation struct. */
+    size_t value_size = (_tcslen(value) + 1) * sizeof(TCHAR);
+    struct msica_op_string *op = (struct msica_op_string*)malloc(sizeof(struct 

msica_op_string) + value_size);

more malloc()ing with no return value check... unless there are guarantees that "malloc() smaller <x> bytes will always succeed in MSI context", this does not look like good practice.

Then, it would be nice if file headers had a short comment "what is this doing" (like, I just read through "msiex.c", and having an idea what I can find in there makes life easier).

Does some other patch in the series have an "overall picture" document that explains how the "large picture" in MSI custom DLL works? Like, first happens <this>, than <that>, and for a silent installer <there> will be a change? Maybe just a pointer to a MS document?

Test compiled on ubuntu 16.04/mingw ("so it compiles for mingw") and on Linux ("so it does not cause any autoconf-induced funkiness elsewhere").

I have test-run the mingw-compilede tapctl on Win10, and "tapctl list" "did something" (= it did not crash or complain about missing DLLs) - but it did not actually *work*. The machine has no TAP interfaces, just one LAN card - and "tapctl list" prints out 2 or 10 different "LAN" adapters (see attached image). Not sure this is how it should be... after I installed OpenVPN & had it create a TAP interface, I still have 9 times "LAN-Verbindung" but I also get an "Ethernet 2" now (which is correct).

Your patch has been applied to the master branch.

commit ce68686f1e2d6f9a78ea7395560af78f81234da2

Author: Simon Rozman
Date:   Wed Oct 10 21:23:37 2018 +0200

     Introduce tapctl.exe utility and openvpnmsica.dll MSI CA

     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/search?l=mid&[email protected]...
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Hi,

Thank you. I am reviewing your suggestions and preparing patches for them. Stay tuned.

Good idea. I have put this on my TODO list. I still owe you "Administrators Guide for OpenVPN MSI Deployment". This would make a second document:
"Developers Guide to OpenVPN MSI Setup". I should have done the later first, to make the code review easier for you, sorry. I have so much to explain you and so little time. :(

IIRC, we agreed, I prepare documentation at OpenVPN Wiki. After it's published, remind me to URL it in the source code.

"tapctl list" lists all NICs found. Including the non-TAP ones. On my computer it shows 12, where I see only 2 in the Network Connections window. That's normal, as Windows have separate NICs for RAS: WAN, WAN IPv6, PPPoE, PPTP, L2TP, GRE, SSTP, etc. They all have silly names like "Local Area Connection* <n>" (note the asterisk in their name). They don't display elsewhere normally.

Don't delete them with "tapctl delete" - like the "list" command lists _all_ interfaces, the "delete" can delete _any_ interface on your computer. Even with no TAP-Windows6 driver installed.

On the other hand: "tapctl create" supports creating a TAP-Windows6 interface only.

Shall I limit the "tapctl list" and "tapctl delete" commands to the TAP-Windows6 interfaces too? The utility is called *tap*ctl after all.

Best regards,
Simon

Hi,

I am :-)

[..]

Works for me!

Ah. This is very confusing. I can see the benefit of a diagnostic utility to tell me "show me *all* network adapters in the system!", but maybe it should not be the default output of something called "*tap* ctl" :-)

Yes, this sounds like "less room for confusion, accidents and user frustration".

"tapctl list" could grow a "--all" parameter to "really display all" (including some sort of flaggin "this is is a tap adapter", maybe even displaying the "tap901.sys" vs "root\\tap901.sys" difference), but "delete" really should not touch not-ours interfaces.

thanks :)

gert

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...

Note: Hardware ID check is used selectively. When naming the adapter, we still need to check all existing adapters to prevent duplicate names. When listing or removing adapters by name, the operation is limited to TUN-Windows6 adapters only.

This patch follows Gert's recommendations from [openvpn-devel].

Message-ID: <[email protected]...>

---
 src/openvpnmsica/msica_op.c     | 16 ++++----
 src/openvpnmsica/openvpnmsica.c | 69 +++++--------------------------
 src/tapctl/main.c               | 20 ++++-----
 src/tapctl/tap.c                | 72 ++++++++++++++++++++++++---------
 src/tapctl/tap.h                |  8 +++-
 5 files changed, 89 insertions(+), 96 deletions(-)

index fec1ef8a..8912e92a 100644
@@ -443,9 +443,9 @@ msica_op_tap_interface_create_exec(
         }
     }
 
-    /* Get available network interfaces. */
+    /* Get all available network interfaces. */
     struct tap_interface_node *pInterfaceList = NULL;
-    DWORD dwResult = tap_list_interfaces(NULL, &pInterfaceList);
+    DWORD dwResult = tap_list_interfaces(NULL, &pInterfaceList, TRUE);
     if (dwResult == ERROR_SUCCESS)
     {
         /* Does interface exist? */

@@ -598,9 +598,9 @@ msica_op_tap_interface_delete_by_name_exec(
         }
     }
 
-    /* Get available network interfaces. */
+    /* Get available TUN/TAP interfaces. */
     struct tap_interface_node *pInterfaceList = NULL;
-    DWORD dwResult = tap_list_interfaces(NULL, &pInterfaceList);
+    DWORD dwResult = tap_list_interfaces(NULL, &pInterfaceList, FALSE);
     if (dwResult == ERROR_SUCCESS)
     {
         /* Does interface exist? */

@@ -656,9 +656,9 @@ msica_op_tap_interface_delete_by_guid_exec(
         }
     }
 
-    /* Get available network interfaces. */
+    /* Get all available interfaces. */
     struct tap_interface_node *pInterfaceList = NULL;
-    DWORD dwResult = tap_list_interfaces(NULL, &pInterfaceList);
+    DWORD dwResult = tap_list_interfaces(NULL, &pInterfaceList, TRUE);
     if (dwResult == ERROR_SUCCESS)
     {
         /* Does interface exist? */

@@ -715,9 +715,9 @@ msica_op_tap_interface_set_name_exec(
         }
     }
 
-    /* Get available network interfaces. */
+    /* Get all available network interfaces. */
     struct tap_interface_node *pInterfaceList = NULL;
-    DWORD dwResult = tap_list_interfaces(NULL, &pInterfaceList);
+    DWORD dwResult = tap_list_interfaces(NULL, &pInterfaceList, TRUE);
     if (dwResult == ERROR_SUCCESS)
     {
         /* Does interface exist? */

index 295366b4..04c03ddc 100644
@@ -58,7 +58,7 @@ static const TCHAR usage_message[] =
     TEXT("Commands:\n")
     TEXT("\n")
     TEXT("create     Create a new TUN/TAP interface\n")
-    TEXT("list       List network interfaces\n")
+    TEXT("list       List TUN/TAP interfaces\n")
     TEXT("delete     Delete specified network interface\n")
     TEXT("help       Display this text\n")
     TEXT("\n")

@@ -90,7 +90,7 @@ static const TCHAR usage_message_create[] =
 static const TCHAR usage_message_list[] =
     TEXT("%s\n")
     TEXT("\n")
-    TEXT("Lists network interfaces\n")
+    TEXT("Lists TUN/TAP interfaces\n")
     TEXT("\n")
     TEXT("Usage:\n")
     TEXT("\n")

@@ -98,7 +98,7 @@ static const TCHAR usage_message_list[] =
     TEXT("\n")
     TEXT("Output:\n")
     TEXT("\n")
-    TEXT("This command prints all network interfaces to stdout.                          \n")
+    TEXT("This command prints all TUN/TAP interfaces to stdout.                          \n")
 ;
 
 static const TCHAR usage_message_delete[] =

@@ -200,9 +200,9 @@ _tmain(int argc, LPCTSTR argv[])
 
         if (szName)
         {
-            /* Get the list of available interfaces. */
+            /* Get the list of all available interfaces. */
             struct tap_interface_node *pInterfaceList = NULL;
-            dwResult = tap_list_interfaces(NULL, &pInterfaceList);
+            dwResult = tap_list_interfaces(NULL, &pInterfaceList, TRUE);
             if (dwResult != ERROR_SUCCESS)
             {
                 _ftprintf(stderr, TEXT("Enumerating interfaces failed (error 0x%x).\n"), dwResult);

@@ -257,12 +257,12 @@ create_delete_interface:
     }
     else if (_tcsicmp(argv[1], TEXT("list")) == 0)
     {
-        /* Output list of network interfaces. */
+        /* Output list of TUN/TAP interfaces. */
         struct tap_interface_node *pInterfaceList = NULL;
-        DWORD dwResult = tap_list_interfaces(NULL, &pInterfaceList);
+        DWORD dwResult = tap_list_interfaces(NULL, &pInterfaceList, FALSE);
         if (dwResult != ERROR_SUCCESS)
         {
-            _ftprintf(stderr, TEXT("Enumerating interfaces failed (error 0x%x).\n"), dwResult);
+            _ftprintf(stderr, TEXT("Enumerating TUN/TAP interfaces failed (error 0x%x).\n"), dwResult);
             iResult = 1; goto quit;
         }
 

@@ -290,10 +290,10 @@ create_delete_interface:
         {
             /* The argument failed to covert to GUID. Treat it as the interface name. */
             struct tap_interface_node *pInterfaceList = NULL;
-            DWORD dwResult = tap_list_interfaces(NULL, &pInterfaceList);
+            DWORD dwResult = tap_list_interfaces(NULL, &pInterfaceList, FALSE);
             if (dwResult != ERROR_SUCCESS)
             {
-                _ftprintf(stderr, TEXT("Enumerating interfaces failed (error 0x%x).\n"), dwResult);
+                _ftprintf(stderr, TEXT("Enumerating TUN/TAP interfaces failed (error 0x%x).\n"), dwResult);
                 iResult = 1; goto quit;
             }
 

index c09c4766..2437d056 100644
@@ -55,7 +55,7 @@ tap_create_interface(
 
 
 /**
- * Deletes a TUN/TAP interface.
+ * Deletes an interface.
  *
  * @param hwndParent    A handle to the top-level window to use for any user interface that is
  *                      related to non-device-specific actions (such as a select-device dialog

@@ -120,12 +120,16 @@ struct tap_interface_node
  *                      the list. After the list is no longer required, free it using
  *                      tap_free_interface_list().
  *
+ * @param bAll          When TRUE, all network interfaces found are added to the list. When
+ *                      FALSE, only TUN/TAP interfaces found are added.
+ *
  * @return ERROR_SUCCESS on success; Win32 error code otherwise
  */
 DWORD
 tap_list_interfaces(
     _In_opt_ HWND hwndParent,
-    _Out_ struct tap_interface_node **ppInterfaceList);
+    _Out_ struct tap_interface_node **ppInterfaceList,
+    _In_ BOOL bAll);
 
 
 /**

2.19.0.windows.1

Thanks for the change. I have *not* tested it, just test compiled it (the usual ubuntu 16.04/mingw combo), but staring at the code, this look reasonable. Actually the change seems to make the code *more simple* (in FindTAPInterfaces() ) which is always welcome...

Your patch has been applied to the master branch.

commit f20dd0ee77c37c5d99106d0cbf29658b3c57fd3f

Author: Simon Rozman
Date:   Sun Feb 24 19:16:21 2019 +0100

     Limit tapctl.exe and openvpnmsica.dll to TAP-Windows6 adapters only

     Signed-off-by: Simon Rozman <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg18234.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Since OpenVPN is security software, "TLS" usually stands for Transport Layer Security.

Furthermore, repetitive copy&paste code was refactored using a macro.

This patch follows Gert's recommendations from [openvpn-devel].

Message-ID: <[email protected]...>

---
 src/openvpnmsica/dllmain.c      | 30 +++++++++++++++---------------
 src/openvpnmsica/openvpnmsica.c | 20 +++++---------------
 src/openvpnmsica/openvpnmsica.h | 18 ++++++++++++++----
 3 files changed, 34 insertions(+), 34 deletions(-)

index 50231e4c..5f5092a0 100644
@@ -36,7 +36,7 @@
 #include <tchar.h>
 
 
-DWORD openvpnmsica_tlsidx_session = TLS_OUT_OF_INDEXES;
+DWORD openvpnmsica_thread_data_idx = TLS_OUT_OF_INDEXES;
 
 
 /**

@@ -54,9 +54,9 @@ DllMain(
     switch (dwReason)
     {
         case DLL_PROCESS_ATTACH:
-            /* Allocate TLS index. */
-            openvpnmsica_tlsidx_session = TlsAlloc();
-            if (openvpnmsica_tlsidx_session == TLS_OUT_OF_INDEXES)
+            /* Allocate thread local storage index. */
+            openvpnmsica_thread_data_idx = TlsAlloc();
+            if (openvpnmsica_thread_data_idx == TLS_OUT_OF_INDEXES)
             {
                 return FALSE;
             }

@@ -64,25 +64,25 @@ DllMain(
 
         case DLL_THREAD_ATTACH:
         {
-            /* Create TLS data. */
-            struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)malloc(sizeof(struct openvpnmsica_tls_data));
-            memset(s, 0, sizeof(struct openvpnmsica_tls_data));
-            TlsSetValue(openvpnmsica_tlsidx_session, s);
+            /* Create thread local storage data. */
+            struct openvpnmsica_thread_data *s = (struct openvpnmsica_thread_data *)malloc(sizeof(struct openvpnmsica_thread_data));
+            memset(s, 0, sizeof(struct openvpnmsica_thread_data));
+            TlsSetValue(openvpnmsica_thread_data_idx, s);
             break;
         }
 
         case DLL_PROCESS_DETACH:
-            if (openvpnmsica_tlsidx_session != TLS_OUT_OF_INDEXES)
+            if (openvpnmsica_thread_data_idx != TLS_OUT_OF_INDEXES)
             {
-                /* Free TLS data and TLS index. */
-                free(TlsGetValue(openvpnmsica_tlsidx_session));
-                TlsFree(openvpnmsica_tlsidx_session);
+                /* Free thread local storage data and index. */
+                free(TlsGetValue(openvpnmsica_thread_data_idx));
+                TlsFree(openvpnmsica_thread_data_idx);
             }
             break;
 
         case DLL_THREAD_DETACH:
-            /* Free TLS data. */
-            free(TlsGetValue(openvpnmsica_tlsidx_session));
+            /* Free thread local storage data. */
+            free(TlsGetValue(openvpnmsica_thread_data_idx));
             break;
     }
 

@@ -105,7 +105,7 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist)
     /* Secure last error before it is overridden. */
     DWORD dwResult = (flags & M_ERRNO) != 0 ? GetLastError() : ERROR_SUCCESS;
 
-    struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)TlsGetValue(openvpnmsica_tlsidx_session);
+    struct openvpnmsica_thread_data *s = (struct openvpnmsica_thread_data *)TlsGetValue(openvpnmsica_thread_data_idx);
     if (s->hInstall == 0)
     {
         /* No MSI session, no fun. */

index 3232a47c..b8108b99 100644
@@ -435,9 +435,7 @@ FindSystemInfo(_In_ MSIHANDLE hInstall)
 
     BOOL bIsCoInitialized = SUCCEEDED(CoInitialize(NULL));
 
-    /* Set MSI session handle in TLS. */
-    struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)TlsGetValue(openvpnmsica_tlsidx_session);
-    s->hInstall = hInstall;
+    OPENVPNMSICA_SAVE_MSI_SESSION(hInstall);
 
     openvpnmsica_set_driver_certification(hInstall);
     openvpnmsica_set_openvpnserv_state(hInstall);

@@ -462,9 +460,7 @@ FindTAPInterfaces(_In_ MSIHANDLE hInstall)
     UINT uiResult;
     BOOL bIsCoInitialized = SUCCEEDED(CoInitialize(NULL));
 
-    /* Set MSI session handle in TLS. */
-    struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)TlsGetValue(openvpnmsica_tlsidx_session);
-    s->hInstall = hInstall;
+    OPENVPNMSICA_SAVE_MSI_SESSION(hInstall);
 
     /* Get available network interfaces. */
     struct tap_interface_node *pInterfaceList = NULL;

@@ -677,9 +673,7 @@ StartOpenVPNGUI(_In_ MSIHANDLE hInstall)
     UINT uiResult;
     BOOL bIsCoInitialized = SUCCEEDED(CoInitialize(NULL));
 
-    /* Set MSI session handle in TLS. */
-    struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)TlsGetValue(openvpnmsica_tlsidx_session);
-    s->hInstall = hInstall;
+    OPENVPNMSICA_SAVE_MSI_SESSION(hInstall);
 
     /* Create and populate a MSI record. */
     MSIHANDLE hRecord = MsiCreateRecord(1);

@@ -759,9 +753,7 @@ EvaluateTAPInterfaces(_In_ MSIHANDLE hInstall)
     UINT uiResult;
     BOOL bIsCoInitialized = SUCCEEDED(CoInitialize(NULL));
 
-    /* Set MSI session handle in TLS. */
-    struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)TlsGetValue(openvpnmsica_tlsidx_session);
-    s->hInstall = hInstall;
+    OPENVPNMSICA_SAVE_MSI_SESSION(hInstall);
 
     /* List of deferred custom actions EvaluateTAPInterfaces prepares operation sequence for. */
     static const LPCTSTR szActionNames[] =

@@ -1052,9 +1044,7 @@ ProcessDeferredAction(_In_ MSIHANDLE hInstall)
     UINT uiResult;
     BOOL bIsCoInitialized = SUCCEEDED(CoInitialize(NULL));
 
-    /* Set MSI session handle in TLS. */
-    struct openvpnmsica_tls_data *s = (struct openvpnmsica_tls_data *)TlsGetValue(openvpnmsica_tlsidx_session);
-    s->hInstall = hInstall;
+    OPENVPNMSICA_SAVE_MSI_SESSION(hInstall);
 
     BOOL bIsCleanup = MsiGetMode(hInstall, MSIRUNMODE_COMMIT) || MsiGetMode(hInstall, MSIRUNMODE_ROLLBACK);
 

index 382064b6..d6a09127 100644
@@ -33,18 +33,28 @@
 
 
 /**
- * TLS data
+ * Thread local storage data
  */
-struct openvpnmsica_tls_data
+struct openvpnmsica_thread_data
 {
     MSIHANDLE hInstall; /** Handle to the installation session. */
 };
 
 
 /**
- * MSI session handle TLS index
+ * MSI session handle thread local storage index
  */
-extern DWORD openvpnmsica_tlsidx_session;
+extern DWORD openvpnmsica_thread_data_idx;
+
+
+/**
+ * Set MSI session handle in thread local storage.
+ */
+#define OPENVPNMSICA_SAVE_MSI_SESSION(hInstall) \
+{ \
+    struct openvpnmsica_thread_data *s = (struct openvpnmsica_thread_data *)TlsGetValue(openvpnmsica_thread_data_idx); \
+    s->hInstall = (hInstall); \
+}
 
 
 /*

2.19.0.windows.1

[email protected]...>
References: <[email protected]... [email protected]...>

"Does what I asked for and looks reasonable" ;-) - thanks.

Your patch has been applied to the master branch.

commit f75351da2974dac10baca5fa83c18be7f27c73cf

Author: Simon Rozman
Date:   Sun Feb 24 19:12:52 2019 +0100

     Disambiguate thread local storage references from TLS

     Signed-off-by: Simon Rozman <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...
     URL: https://www.mail-archive.com/[email protected]/msg18230.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Hi,

"whatever happened here", this was supposed to thread...

gert

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...

(Re-send with proper references - my mailer and my git tools hate each other today)

"Does what I asked for and looks reasonable" ;-) - thanks.

Your patch has been applied to the master branch.

commit f75351da2974dac10baca5fa83c18be7f27c73cf

Author: Simon Rozman
Date:   Sun Feb 24 19:12:52 2019 +0100

     Disambiguate thread local storage references from TLS

     Signed-off-by: Simon Rozman <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...
     URL: https://www.mail-archive.com/[email protected]/msg18230.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Extra NULL checks were added after malloc() calls to display out-of- memory error and try to exit gracefully.

Function msica_op_create_*() now return NULL in out-of-memory condition too. Since their output is directly used in msica_op_seq_add_head() and msica_op_seq_add_tail() functions, later were extended to check for NULL pointer arguments.

This patch follows Gert's recommendations from [openvpn-devel].

Message-ID: <[email protected]...>

---
 src/openvpnmsica/dllmain.c      | 23 ++++++++++---
 src/openvpnmsica/msica_op.c     | 60 +++++++++++++++++++++++++++++++++
 src/openvpnmsica/msiex.c        | 36 ++++++++++++++++++++
 src/openvpnmsica/openvpnmsica.c | 48 +++++++++++++++++++++++---
 src/tapctl/tap.c                | 50 +++++++++++++++++++++++++++
 5 files changed, 207 insertions(+), 10 deletions(-)

index 5f5092a0..5e29f44c 100644
@@ -65,8 +65,12 @@ DllMain(
         case DLL_THREAD_ATTACH:
         {
             /* Create thread local storage data. */
-            struct openvpnmsica_thread_data *s = (struct openvpnmsica_thread_data *)malloc(sizeof(struct openvpnmsica_thread_data));
-            memset(s, 0, sizeof(struct openvpnmsica_thread_data));
+            struct openvpnmsica_thread_data *s = (struct openvpnmsica_thread_data *)calloc(1, sizeof(struct openvpnmsica_thread_data));
+            if (s == NULL)
+            {
+                return FALSE;
+            }
+
             TlsSetValue(openvpnmsica_thread_data_idx, s);
             break;
         }

@@ -128,9 +132,18 @@ x_msg_va(const unsigned int flags, const char *format, va_list arglist)
         {
             /* Allocate on heap and retry. */
             char *szMessage = (char *)malloc(++iResultLen * sizeof(char));
-            vsnprintf(szMessage, iResultLen, format, arglist);
-            MsiRecordSetStringA(hRecordProg, 2, szMessage);
-            free(szMessage);
+            if (szMessage != NULL)
+            {
+                vsnprintf(szMessage, iResultLen, format, arglist);
+                MsiRecordSetStringA(hRecordProg, 2, szMessage);
+                free(szMessage);
+            }
+            else
+            {
+                /* Use stack variant anyway, but make sure it's zero-terminated. */
+                szBufStack[_countof(szBufStack) - 1] = 0;
+                MsiRecordSetStringA(hRecordProg, 2, szBufStack);
+            }
         }
     }
 

index 9b95f291..7e2bed3e 100644
@@ -54,6 +54,12 @@ msi_get_string(
     {
         /* Copy from stack. */
         *pszValue = (LPTSTR)malloc(++dwLength * sizeof(TCHAR));
+        if (*pszValue == NULL)
+        {
+            msg(M_FATAL, "%s: malloc(%u) failed", dwLength * sizeof(TCHAR));
+            return ERROR_OUTOFMEMORY;
+        }
+
         memcpy(*pszValue, szBufStack, dwLength * sizeof(TCHAR));
         return ERROR_SUCCESS;
     }

@@ -61,6 +67,12 @@ msi_get_string(
     {
         /* Allocate on heap and retry. */
         LPTSTR szBufHeap = (LPTSTR)malloc(++dwLength * sizeof(TCHAR));
+        if (szBufHeap == NULL)
+        {
+            msg(M_FATAL, "%s: malloc(%u) failed", __FUNCTION__, dwLength * sizeof(TCHAR));
+            return ERROR_OUTOFMEMORY;
+        }
+
         uiResult = MsiGetProperty(hInstall, szName, szBufHeap, &dwLength);
         if (uiResult == ERROR_SUCCESS)
         {

@@ -100,6 +112,12 @@ msi_get_record_string(
     {
         /* Copy from stack. */
         *pszValue = (LPTSTR)malloc(++dwLength * sizeof(TCHAR));
+        if (*pszValue == NULL)
+        {
+            msg(M_FATAL, "%s: malloc(%u) failed", __FUNCTION__, dwLength * sizeof(TCHAR));
+            return ERROR_OUTOFMEMORY;
+        }
+
         memcpy(*pszValue, szBufStack, dwLength * sizeof(TCHAR));
         return ERROR_SUCCESS;
     }

@@ -107,6 +125,12 @@ msi_get_record_string(
     {
         /* Allocate on heap and retry. */
         LPTSTR szBufHeap = (LPTSTR)malloc(++dwLength * sizeof(TCHAR));
+        if (szBufHeap == NULL)
+        {
+            msg(M_FATAL, "%s: malloc(%u) failed", __FUNCTION__, dwLength * sizeof(TCHAR));
+            return ERROR_OUTOFMEMORY;
+        }
+
         uiResult = MsiRecordGetString(hRecord, iField, szBufHeap, &dwLength);
         if (uiResult == ERROR_SUCCESS)
         {

@@ -146,6 +170,12 @@ msi_format_record(
     {
         /* Copy from stack. */
         *pszValue = (LPTSTR)malloc(++dwLength * sizeof(TCHAR));
+        if (*pszValue == NULL)
+        {
+            msg(M_FATAL, "%s: malloc(%u) failed", __FUNCTION__, dwLength * sizeof(TCHAR));
+            return ERROR_OUTOFMEMORY;
+        }
+
         memcpy(*pszValue, szBufStack, dwLength * sizeof(TCHAR));
         return ERROR_SUCCESS;
     }

@@ -153,6 +183,12 @@ msi_format_record(
     {
         /* Allocate on heap and retry. */
         LPTSTR szBufHeap = (LPTSTR)malloc(++dwLength * sizeof(TCHAR));
+        if (szBufHeap == NULL)
+        {
+            msg(M_FATAL, "%s: malloc(%u) failed", __FUNCTION__, dwLength * sizeof(TCHAR));
+            return ERROR_OUTOFMEMORY;
+        }
+
         uiResult = MsiFormatRecord(hInstall, hRecord, szBufHeap, &dwLength);
         if (uiResult == ERROR_SUCCESS)
         {

2.19.0.windows.1

[email protected]...>
References: <[email protected]... [email protected]...>

Thanks :-) (not only NULL checks, but calloc() as well!)

Test built on ubuntu1604/mingw.

Your patch has been applied to the master branch.

commit fbb5d6c56b5586ffd3731fc76e706131b635be58

Author: Simon Rozman
Date:   Sun Feb 24 19:14:00 2019 +0100

     Add NULL checks

     Signed-off-by: Simon Rozman <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...
     URL: https://www.mail-archive.com/[email protected]/msg18231.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

(Re-send with proper references - my mailer and my git tools hate each other today)

Thanks :-) (not only NULL checks, but calloc() as well!)

Test built on ubuntu1604/mingw.

Your patch has been applied to the master branch.

commit fbb5d6c56b5586ffd3731fc76e706131b635be58

Author: Simon Rozman
Date:   Sun Feb 24 19:14:00 2019 +0100

     Add NULL checks

     Signed-off-by: Simon Rozman <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...
     URL: https://www.mail-archive.com/[email protected]/msg18231.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

This patch follows Gert's recommendations from [openvpn-devel].

Message-ID: <[email protected]...>

---
 src/tapctl/basic.h | 1 +
 src/tapctl/error.c | 1 +
 src/tapctl/error.h | 1 +
 src/tapctl/main.c  | 1 +
 src/tapctl/tap.c   | 1 +
 src/tapctl/tap.h   | 1 +
 6 files changed, 6 insertions(+)

index 3469cb1c..bfbcc30d 100644
@@ -1,5 +1,6 @@
 /*
  *  basic -- Basic macros
+ *           https://community.openvpn.net/openvpn/wiki/Tapctl
  *
  *  Copyright (C) 2002-2018 OpenVPN Inc <[email protected]...>
  *  Copyright (C) 2018 Simon Rozman <[email protected]...>

index 42ab6ccb..d1f77d20 100644
@@ -1,5 +1,6 @@
 /*
  *  error -- OpenVPN compatible error reporting API
+ *           https://community.openvpn.net/openvpn/wiki/Tapctl
  *
  *  Copyright (C) 2002-2018 OpenVPN Inc <[email protected]...>
  *  Copyright (C) 2018 Simon Rozman <[email protected]...>

index 1ac9b835..924cbbe8 100644
@@ -1,5 +1,6 @@
 /*
  *  error -- OpenVPN compatible error reporting API
+ *           https://community.openvpn.net/openvpn/wiki/Tapctl
  *
  *  Copyright (C) 2002-2018 OpenVPN Inc <[email protected]...>
  *  Copyright (C) 2018 Simon Rozman <[email protected]...>

index 5e6e1752..295366b4 100644
@@ -1,5 +1,6 @@
 /*
  *  tapctl -- Utility to manipulate TUN/TAP interfaces on Windows
+ *            https://community.openvpn.net/openvpn/wiki/Tapctl
  *
  *  Copyright (C) 2002-2018 OpenVPN Inc <[email protected]...>
  *  Copyright (C) 2008-2013 David Sommerseth <[email protected]...>

index ba80237f..b2cb2dca 100644
@@ -1,5 +1,6 @@
 /*
  *  tapctl -- Utility to manipulate TUN/TAP interfaces on Windows
+ *            https://community.openvpn.net/openvpn/wiki/Tapctl
  *
  *  Copyright (C) 2018 Simon Rozman <[email protected]...>
  *

index 39dc28b0..c09c4766 100644
@@ -1,5 +1,6 @@
 /*
  *  tapctl -- Utility to manipulate TUN/TAP interfaces on Windows
+ *            https://community.openvpn.net/openvpn/wiki/Tapctl
  *
  *  Copyright (C) 2018 Simon Rozman <[email protected]...>
  *

2.19.0.windows.1

[email protected]...>
References: <[email protected]... [email protected]...>

Trival but useful :)

Your patch has been applied to the master branch.

commit 890f9a85ca86fbba678fddb2e589a0180a6f1784

Author: Simon Rozman
Date:   Sun Feb 24 19:15:04 2019 +0100

     Add user manual and developer notes URL for tapctl.exe

     Signed-off-by: Simon Rozman <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...
     URL: https://www.mail-archive.com/[email protected]/msg18232.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

(Re-send with proper references - my mailer and my git tools hate each other today)

Trival but useful :)

Your patch has been applied to the master branch.

commit 890f9a85ca86fbba678fddb2e589a0180a6f1784

Author: Simon Rozman
Date:   Sun Feb 24 19:15:04 2019 +0100

     Add user manual and developer notes URL for tapctl.exe

     Signed-off-by: Simon Rozman <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...
     URL: https://www.mail-archive.com/[email protected]/msg18232.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering