[Openvpn-devel,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(-)

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 <simon@rozman.si> 
Sent: Tuesday, October 16, 2018 3:26 AM
To: openvpn-devel@lists.sourceforge.net
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(-)

diff --git a/src/openvpnmsica/Makefile.am b/src/openvpnmsica/Makefile.am
index d46170b4..ecca74bc 100644
--- a/src/openvpnmsica/Makefile.am
+++ b/src/openvpnmsica/Makefile.am
@@ -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 = \
diff --git a/src/openvpnmsica/openvpnmsica.c b/src/openvpnmsica/openvpnmsica.c
index 3b90ce05..d1642d6a 100644
--- a/src/openvpnmsica/openvpnmsica.c
+++ b/src/openvpnmsica/openvpnmsica.c
@@ -36,13 +36,15 @@
 #include <memory.h>
 #include <msiquery.h>
 #include <shlwapi.h>
-#ifdef _MSC_VER
-#pragma comment(lib, "shlwapi.lib")
-#endif
 #include <stdbool.h>
 #include <stdlib.h>
 #include <tchar.h>
 
+#ifdef _MSC_VER
+#pragma comment(lib, "shlwapi.lib")
+#pragma comment(lib, "version.lib")
+#endif
+
 
 /**
  * Local constants
@@ -119,7 +121,7 @@ openvpnmsica_setup_sequence_filename(
     {
         size_t len_action_name_z = _tcslen(openvpnmsica_cleanup_action_seqs[i].szName) + 1;
         TCHAR *szPropertyEx = (TCHAR*)malloc((len_property_name + len_action_name_z) * sizeof(TCHAR));
-        memcpy(szPropertyEx                    , szProperty                         , len_property_name * sizeof(TCHAR));
+        memcpy(szPropertyEx                    , szProperty                                , len_property_name * sizeof(TCHAR));
         memcpy(szPropertyEx + len_property_name, openvpnmsica_cleanup_action_seqs[i].szName, len_action_name_z * sizeof(TCHAR));
         _stprintf_s(
             szFilenameEx, _countof(szFilenameEx),
@@ -142,6 +144,120 @@ openvpnmsica_setup_sequence_filename(
 }
 
 
+UINT __stdcall
+FindSystemInfo(_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
+
+    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;
+
+    // Get Windows version.
+    OSVERSIONINFOEX ver_info = { .dwOSVersionInfoSize = sizeof(OSVERSIONINFOEX) };
+    if (!GetVersionEx((LPOSVERSIONINFO)&ver_info)) {
+        uiResult = GetLastError();
+        msg(M_NONFATAL | M_ERRNO, "%s: GetVersionEx() failed", __FUNCTION__);
+        goto cleanup_CoInitialize;
+    }
+
+    // The Windows version is usually spoofed, check using RtlGetVersion().
+    TCHAR szDllPath[0x1000];
+    ExpandEnvironmentStrings(TEXT("%SystemRoot%\\System32\\ntdll.dll"), szDllPath,
+#ifdef UNICODE
+        _countof(szDllPath)
+#else
+        _countof(szDllPath) - 1
+#endif
+    );
+    HMODULE hNtDllModule = LoadLibrary(szDllPath);
+    if (hNtDllModule)
+    {
+        typedef NTSTATUS (WINAPI* fnRtlGetVersion)(PRTL_OSVERSIONINFOW);
+        fnRtlGetVersion RtlGetVersion = (fnRtlGetVersion)GetProcAddress(hNtDllModule, "RtlGetVersion");
+        if (RtlGetVersion)
+        {
+            RTL_OSVERSIONINFOW rtl_ver_info = { .dwOSVersionInfoSize = sizeof(RTL_OSVERSIONINFOW) };
+            if (RtlGetVersion(&rtl_ver_info) == 0)
+                if (
+                    rtl_ver_info.dwMajorVersion >  ver_info.dwMajorVersion ||
+                    rtl_ver_info.dwMajorVersion == ver_info.dwMajorVersion && rtl_ver_info.dwMinorVersion >  ver_info.dwMinorVersion ||
+                    rtl_ver_info.dwMajorVersion == ver_info.dwMajorVersion && rtl_ver_info.dwMinorVersion == ver_info.dwMinorVersion && rtl_ver_info.dwBuildNumber > ver_info.dwBuildNumber)
+                {
+                    // We got RtlGetVersion() and it reported newer version than GetVersionEx().
+                    ver_info.dwMajorVersion = rtl_ver_info.dwMajorVersion;
+                    ver_info.dwMinorVersion = rtl_ver_info.dwMinorVersion;
+                    ver_info.dwBuildNumber  = rtl_ver_info.dwBuildNumber;
+                    ver_info.dwPlatformId   = rtl_ver_info.dwPlatformId;
+                }
+        }
+
+        FreeLibrary(hNtDllModule);
+    }
+
+    // We don't trust RtlGetVersion() either. Check the version resource of kernel32.dll.
+    ExpandEnvironmentStrings(TEXT("%SystemRoot%\\System32\\kernel32.dll"), szDllPath,
+#ifdef UNICODE
+        _countof(szDllPath)
+#else
+        _countof(szDllPath)-1
+#endif
+    );
+
+    DWORD dwHandle;
+    DWORD dwVerInfoSize = GetFileVersionInfoSize(szDllPath, &dwHandle);
+    if (dwVerInfoSize)
+    {
+        LPVOID pVersionInfo = malloc(dwVerInfoSize);
+        if (pVersionInfo)
+        {
+            // Read version info.
+            if (GetFileVersionInfo(szDllPath, dwHandle, dwVerInfoSize, pVersionInfo))
+            {
+                // Get the value for the root block.
+                UINT uiSize = 0;
+                VS_FIXEDFILEINFO *pVSFixedFileInfo = NULL;
+                if (VerQueryValue(pVersionInfo, TEXT("\\"), &pVSFixedFileInfo, &uiSize) && uiSize && pVSFixedFileInfo)
+                    if (HIWORD(pVSFixedFileInfo->dwProductVersionMS) >  ver_info.dwMajorVersion ||
+                        HIWORD(pVSFixedFileInfo->dwProductVersionMS) == ver_info.dwMajorVersion && LOWORD(pVSFixedFileInfo->dwProductVersionMS) >  ver_info.dwMinorVersion ||
+                        HIWORD(pVSFixedFileInfo->dwProductVersionMS) == ver_info.dwMajorVersion && LOWORD(pVSFixedFileInfo->dwProductVersionMS) == ver_info.dwMinorVersion && HIWORD(pVSFixedFileInfo->dwProductVersionLS) > ver_info.dwBuildNumber)
+                    {
+                        // We got kernel32.dll version and it is newer.
+                        ver_info.dwMajorVersion = HIWORD(pVSFixedFileInfo->dwProductVersionMS);
+                        ver_info.dwMinorVersion = LOWORD(pVSFixedFileInfo->dwProductVersionMS);
+                        ver_info.dwBuildNumber  = HIWORD(pVSFixedFileInfo->dwProductVersionLS);
+                    }
+            }
+
+            free(pVersionInfo);
+        }
+    }
+
+    uiResult = MsiSetProperty(hInstall, TEXT("DriverCertification"), ver_info.dwMajorVersion >= 10 ? ver_info.wProductType > VER_NT_WORKSTATION ? TEXT("whql") : TEXT("attsgn") : TEXT(""));
+    if (uiResult != ERROR_SUCCESS)
+    {
+        SetLastError(uiResult); /* MSDN does not mention MsiSetProperty() to set GetLastError(). But we do have an error code. Set last error manually. */
+        msg(M_NONFATAL | M_ERRNO, "%s: MsiSetProperty(\"TAPINTERFACES\") failed", __FUNCTION__);
+        goto cleanup_CoInitialize;
+    }
+
+    uiResult = ERROR_SUCCESS;
+
+cleanup_CoInitialize:
+    if (bIsCoInitialized) CoUninitialize();
+    return uiResult;
+}
+
+
 UINT __stdcall
 FindTAPInterfaces(_In_ MSIHANDLE hInstall)
 {
diff --git a/src/openvpnmsica/openvpnmsica.h b/src/openvpnmsica/openvpnmsica.h
index bb8e28ec..da145062 100644
--- a/src/openvpnmsica/openvpnmsica.h
+++ b/src/openvpnmsica/openvpnmsica.h
@@ -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&amp;data=02%7C01%7Cjkunkee%40microsoft.com%7Cd520a31814634dced1dc08d63351eb58%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636752824275192379&amp;sdata=2DLeMsL5JHIFmh07Fh6zqZTGt117xy%2Bu2TY3acNpaH8%3D&amp;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.

Hi,

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

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

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

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

> 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

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

Regards,
Simon

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

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

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.

> 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

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,

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

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

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

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

Hi,

> > 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
> 
> Many thanks for this. I will test the MsiBreak method shortly and remove
> the MessageBox calls completely for a cleaner code then.

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

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

Unfortunately, yes.

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

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

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

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

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

> When the MSI package is installed by Group Policy Client, the MSI is
> launched directly by msiexec.exe. 

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. 

> 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
> 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 name "FindSystemInfo" was picked generic on
> purpose, to allow us to add other detections missing in MSI should we need
> any down the road.

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

Samuli, LGTM.

Thanks,
Jon

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

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

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

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

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

...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%. :)

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

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

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

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

Acked-by: Jon Kunkee <jkunkee@microsoft.com>
     Message-Id: <20181016102627.18676-4-simon@rozman.si>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg17763.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering