[Openvpn-devel,11/12] openvpnmsica: Merge FindTUNTAPAdapters into FindSystemInfo

Message ID 20200309131728.380-11-simon@rozman.si
State Accepted
Headers show
Series [Openvpn-devel,01/12] openvpnmsica: Remove required Windows driver certification detection | expand

Commit Message

Simon Rozman March 9, 2020, 2:17 a.m. UTC
1. We don't need two custom actions to evaluate the system state, do we?

2. FindTUNTAPAdapters was actually broken. It enumerated all existing
   network adapters, rather than just the ones we are interested in:
   TAP-Windows6 and Wintun.

3. TUNTAPADAPTER and ACTIVETUNTAPADAPTERS were split into
   TAPWINDOWS6ADAPTERS, ACTIVETAPWINDOWS6ADAPTERS, WINTUNADAPTERS and
   ACTIVEWINTUNADAPTERS to allow finer control.

Signed-off-by: Simon Rozman <simon@rozman.si>
---
 src/openvpnmsica/openvpnmsica.c | 235 ++++++++++++++++----------------
 src/openvpnmsica/openvpnmsica.h |  26 ++--
 2 files changed, 125 insertions(+), 136 deletions(-)

Comments

Lev Stipakov March 24, 2020, 1:07 a.m. UTC | #1
Hi,

Compiled with msvc, smoke-tested with rundll32.

One thing:

> +    set_openvpnserv_state(hInstall);
> +    find_adapters(
> +        hInstall,
> +        TEXT("root\\") TEXT(TAP_WIN_COMPONENT_ID),
> +        TEXT("TAPWINDOWS6ADAPTERS"),
> +        TEXT("ACTIVETAPWINDOWS6ADAPTERS"));

Both methods return error codes which we ignore.
Simon Rozman March 30, 2020, 7:35 a.m. UTC | #2
Hi Lev,

I'm struggling with family duties now that schools are closed. This makes it hard to find any time for computers.

Nevertheless, should find_adapters() fail for some reason, it is not critical to bail out of FindSystemInfo() custom action.

The find_adapters() itself already displays a resumable error message (MSI also writes it to the log) on all of the error return paths:
- tap_list_adapters() calls msg(M_NONFATAL...) on error returns
- other returns have msg(M_NONFATAL...)

Mind that msg() is using MSI error messaging [https://github.com/rozmansi/openvpn/blob/feature/msi/src/openvpnmsica/dllmain.c#L108]. MsiProcessMessage(s->hInstall, INSTALLMESSAGE_ERROR, hRecordProg); will popup an error dialog in interactive MSI sessions, and write error message to the log in interactive and non-interactive sessions.

To summarize: the return value of find_adapters() call is ignored on purpose.

Regards,
Simon

-----Original Message-----
From: Lev Stipakov <lstipakov@gmail.com>
Date: Tuesday, 24 March 2020 at 13:07
To: Simon Rozman <simon@rozman.si>
Cc: "openvpn-devel@lists.sourceforge.net" <openvpn-devel@lists.sourceforge.net>
Subject: Re: [Openvpn-devel] [PATCH 11/12] openvpnmsica: Merge FindTUNTAPAdapters into FindSystemInfo

    Hi,
    
    Compiled with msvc, smoke-tested with rundll32.
    
    One thing:
    
    > +    set_openvpnserv_state(hInstall);
    > +    find_adapters(
    > +        hInstall,
    > +        TEXT("root\\") TEXT(TAP_WIN_COMPONENT_ID),
    > +        TEXT("TAPWINDOWS6ADAPTERS"),
    > +        TEXT("ACTIVETAPWINDOWS6ADAPTERS"));
    
    Both methods return error codes which we ignore.
Lev Stipakov March 30, 2020, 8:39 p.m. UTC | #3
Hi,

> To summarize: the return value of find_adapters() call is ignored on purpose.

Shouldn't return type be void if return value is not used?

I'll ack it to not to further delay 2.5 release, but it would be good
to get a follow-up
patch with either void or return value being not ignored.

Acked-by: Lev Stipakov <lstipakov@gmail.com>
Gert Doering March 31, 2020, 11:44 p.m. UTC | #4
Your patch has been applied to the master branch.

(I've read the discussion, and agree with Lev that a followup patch
might be a good thing - "either void or return value being not ignored")

Tested on a mingw compile.

commit c6f8d1a7a3dcb3c8608126ebb34b9087c6c9fb1e
Author: Simon Rozman
Date:   Mon Mar 9 14:17:27 2020 +0100

     openvpnmsica: Merge FindTUNTAPAdapters into FindSystemInfo

     Signed-off-by: Simon Rozman <simon@rozman.si>
     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Message-Id: <20200309131728.380-11-simon@rozman.si>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19531.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpnmsica/openvpnmsica.c b/src/openvpnmsica/openvpnmsica.c
index ae9b007f..28cf16b5 100644
--- a/src/openvpnmsica/openvpnmsica.c
+++ b/src/openvpnmsica/openvpnmsica.c
@@ -248,49 +248,26 @@  cleanup_OpenSCManager:
 }
 
 
-UINT __stdcall
-FindSystemInfo(_In_ MSIHANDLE hInstall)
-{
-#ifdef _MSC_VER
-#pragma comment(linker, DLLEXP_EXPORT)
-#endif
-
-    debug_popup(TEXT(__FUNCTION__));
-
-    BOOL bIsCoInitialized = SUCCEEDED(CoInitialize(NULL));
-
-    OPENVPNMSICA_SAVE_MSI_SESSION(hInstall);
-
-    set_openvpnserv_state(hInstall);
-
-    if (bIsCoInitialized)
-    {
-        CoUninitialize();
-    }
-    return ERROR_SUCCESS;
-}
-
-
-UINT __stdcall
-FindTUNTAPAdapters(_In_ MSIHANDLE hInstall)
+static UINT
+find_adapters(
+    _In_ MSIHANDLE hInstall,
+    _In_z_ LPCTSTR szHardwareId,
+    _In_z_ LPCTSTR szAdaptersPropertyName,
+    _In_z_ LPCTSTR szActiveAdaptersPropertyName)
 {
-#ifdef _MSC_VER
-#pragma comment(linker, DLLEXP_EXPORT)
-#endif
-
-    debug_popup(TEXT(__FUNCTION__));
-
     UINT uiResult;
-    BOOL bIsCoInitialized = SUCCEEDED(CoInitialize(NULL));
-
-    OPENVPNMSICA_SAVE_MSI_SESSION(hInstall);
 
-    /* Get existing network adapters. */
+    /* Get network adapters with given hardware ID. */
     struct tap_adapter_node *pAdapterList = NULL;
-    uiResult = tap_list_adapters(NULL, NULL, &pAdapterList);
+    uiResult = tap_list_adapters(NULL, szHardwareId, &pAdapterList);
     if (uiResult != ERROR_SUCCESS)
     {
-        goto cleanup_CoInitialize;
+        return uiResult;
+    }
+    else if (pAdapterList == NULL)
+    {
+        /* No adapters - no fun. */
+        return ERROR_SUCCESS;
     }
 
     /* Get IPv4/v6 info for all network adapters. Actually, we're interested in link status only: up/down? */
@@ -302,7 +279,7 @@  FindTUNTAPAdapters(_In_ MSIHANDLE hInstall)
         if (pAdapterAdresses == NULL)
         {
             msg(M_NONFATAL, "%s: malloc(%u) failed", __FUNCTION__, ulAdapterAdressesSize);
-            uiResult = ERROR_OUTOFMEMORY; goto cleanup_tap_list_adapters;
+            uiResult = ERROR_OUTOFMEMORY; goto cleanup_pAdapterList;
         }
 
         ULONG ulResult = GetAdaptersAddresses(
@@ -322,117 +299,135 @@  FindTUNTAPAdapters(_In_ MSIHANDLE hInstall)
         {
             SetLastError(ulResult); /* MSDN does not mention GetAdaptersAddresses() to set GetLastError(). But we do have an error code. Set last error manually. */
             msg(M_NONFATAL | M_ERRNO, "%s: GetAdaptersAddresses() failed", __FUNCTION__);
-            uiResult = ulResult; goto cleanup_tap_list_adapters;
+            uiResult = ulResult; goto cleanup_pAdapterList;
         }
     }
 
-    if (pAdapterList != NULL)
+    /* Count adapters. */
+    size_t adapter_count = 0;
+    for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext)
     {
-        /* Count adapters. */
-        size_t adapter_count = 0;
-        for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext)
-        {
-            adapter_count++;
-        }
+        adapter_count++;
+    }
 
-        /* Prepare semicolon delimited list of TAP adapter ID(s) and active TAP adapter ID(s). */
-        LPTSTR
-            szAdapters     = (LPTSTR)malloc(adapter_count * (38 /*GUID*/ + 1 /*separator/terminator*/) * sizeof(TCHAR)),
-            szAdaptersTail = szAdapters;
-        if (szAdapters == NULL)
-        {
-            msg(M_FATAL, "%s: malloc(%u) failed", __FUNCTION__, adapter_count * (38 /*GUID*/ + 1 /*separator/terminator*/) * sizeof(TCHAR));
-            uiResult = ERROR_OUTOFMEMORY; goto cleanup_pAdapterAdresses;
-        }
+    /* Prepare semicolon delimited list of TAP adapter ID(s) and active TAP adapter ID(s). */
+    LPTSTR
+        szAdapters     = (LPTSTR)malloc(adapter_count * (38 /*GUID*/ + 1 /*separator/terminator*/) * sizeof(TCHAR)),
+        szAdaptersTail = szAdapters;
+    if (szAdapters == NULL)
+    {
+        msg(M_FATAL, "%s: malloc(%u) failed", __FUNCTION__, adapter_count * (38 /*GUID*/ + 1 /*separator/terminator*/) * sizeof(TCHAR));
+        uiResult = ERROR_OUTOFMEMORY; goto cleanup_pAdapterAdresses;
+    }
+
+    LPTSTR
+        szAdaptersActive     = (LPTSTR)malloc(adapter_count * (38 /*GUID*/ + 1 /*separator/terminator*/) * sizeof(TCHAR)),
+        szAdaptersActiveTail = szAdaptersActive;
+    if (szAdaptersActive == NULL)
+    {
+        msg(M_FATAL, "%s: malloc(%u) failed", __FUNCTION__, adapter_count * (38 /*GUID*/ + 1 /*separator/terminator*/) * sizeof(TCHAR));
+        uiResult = ERROR_OUTOFMEMORY; goto cleanup_szAdapters;
+    }
 
-        LPTSTR
-            szAdaptersActive     = (LPTSTR)malloc(adapter_count * (38 /*GUID*/ + 1 /*separator/terminator*/) * sizeof(TCHAR)),
-            szAdaptersActiveTail = szAdaptersActive;
-        if (szAdaptersActive == NULL)
+    for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext)
+    {
+        /* Convert adapter GUID to UTF-16 string. (LPOLESTR defaults to LPWSTR) */
+        LPOLESTR szAdapterId = NULL;
+        StringFromIID((REFIID)&pAdapter->guid, &szAdapterId);
+
+        /* Append to the list of TAP adapter ID(s). */
+        if (szAdapters < szAdaptersTail)
         {
-            msg(M_FATAL, "%s: malloc(%u) failed", __FUNCTION__, adapter_count * (38 /*GUID*/ + 1 /*separator/terminator*/) * sizeof(TCHAR));
-            uiResult = ERROR_OUTOFMEMORY; goto cleanup_szAdapters;
+            *(szAdaptersTail++) = TEXT(';');
         }
+        memcpy(szAdaptersTail, szAdapterId, 38 * sizeof(TCHAR));
+        szAdaptersTail += 38;
 
-        for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext)
+        /* If this adapter is active (connected), add it to the list of active TAP adapter ID(s). */
+        for (PIP_ADAPTER_ADDRESSES p = pAdapterAdresses; p; p = p->Next)
         {
-            /* Convert adapter GUID to UTF-16 string. (LPOLESTR defaults to LPWSTR) */
-            LPOLESTR szAdapterId = NULL;
-            StringFromIID((REFIID)&pAdapter->guid, &szAdapterId);
-
-            /* Append to the list of TAP adapter ID(s). */
-            if (szAdapters < szAdaptersTail)
-            {
-                *(szAdaptersTail++) = TEXT(';');
-            }
-            memcpy(szAdaptersTail, szAdapterId, 38 * sizeof(TCHAR));
-            szAdaptersTail += 38;
-
-            /* If this adapter is active (connected), add it to the list of active TAP adapter ID(s). */
-            for (PIP_ADAPTER_ADDRESSES p = pAdapterAdresses; p; p = p->Next)
+            OLECHAR szId[38 /*GUID*/ + 1 /*terminator*/];
+            GUID guid;
+            if (MultiByteToWideChar(CP_ACP, MB_PRECOMPOSED, p->AdapterName, -1, szId, _countof(szId)) > 0
+                && SUCCEEDED(IIDFromString(szId, &guid))
+                && memcmp(&guid, &pAdapter->guid, sizeof(GUID)) == 0)
             {
-                OLECHAR szId[38 /*GUID*/ + 1 /*terminator*/];
-                GUID guid;
-                if (MultiByteToWideChar(CP_ACP, MB_PRECOMPOSED, p->AdapterName, -1, szId, _countof(szId)) > 0
-                    && SUCCEEDED(IIDFromString(szId, &guid))
-                    && memcmp(&guid, &pAdapter->guid, sizeof(GUID)) == 0)
+                if (p->OperStatus == IfOperStatusUp)
                 {
-                    if (p->OperStatus == IfOperStatusUp)
+                    /* This TAP adapter is active (connected). */
+                    if (szAdaptersActive < szAdaptersActiveTail)
                     {
-                        /* This TAP adapter is active (connected). */
-                        if (szAdaptersActive < szAdaptersActiveTail)
-                        {
-                            *(szAdaptersActiveTail++) = TEXT(';');
-                        }
-                        memcpy(szAdaptersActiveTail, szAdapterId, 38 * sizeof(TCHAR));
-                        szAdaptersActiveTail += 38;
+                        *(szAdaptersActiveTail++) = TEXT(';');
                     }
-                    break;
+                    memcpy(szAdaptersActiveTail, szAdapterId, 38 * sizeof(TCHAR));
+                    szAdaptersActiveTail += 38;
                 }
+                break;
             }
-            CoTaskMemFree(szAdapterId);
-        }
-        szAdaptersTail      [0] = 0;
-        szAdaptersActiveTail[0] = 0;
-
-        /* Set Installer TUNTAPADAPTERS property. */
-        uiResult = MsiSetProperty(hInstall, TEXT("TUNTAPADAPTERS"), szAdapters);
-        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(\"TUNTAPADAPTERS\") failed", __FUNCTION__);
-            goto cleanup_szAdaptersActive;
-        }
-
-        /* Set Installer ACTIVETUNTAPADAPTERS property. */
-        uiResult = MsiSetProperty(hInstall, TEXT("ACTIVETUNTAPADAPTERS"), szAdaptersActive);
-        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(\"ACTIVETUNTAPADAPTERS\") failed", __FUNCTION__);
-            goto cleanup_szAdaptersActive;
         }
+        CoTaskMemFree(szAdapterId);
+    }
+    szAdaptersTail      [0] = 0;
+    szAdaptersActiveTail[0] = 0;
 
-cleanup_szAdaptersActive:
-        free(szAdaptersActive);
-cleanup_szAdapters:
-        free(szAdapters);
+    /* Set Installer properties. */
+    uiResult = MsiSetProperty(hInstall, szAdaptersPropertyName, szAdapters);
+    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(\"%s\") failed", __FUNCTION__, szAdaptersPropertyName);
+        goto cleanup_szAdaptersActive;
     }
-    else
+    uiResult = MsiSetProperty(hInstall, szActiveAdaptersPropertyName, szAdaptersActive);
+    if (uiResult != ERROR_SUCCESS)
     {
-        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(\"%s\") failed", __FUNCTION__, szActiveAdaptersPropertyName);
+        goto cleanup_szAdaptersActive;
     }
 
+cleanup_szAdaptersActive:
+    free(szAdaptersActive);
+cleanup_szAdapters:
+    free(szAdapters);
 cleanup_pAdapterAdresses:
     free(pAdapterAdresses);
-cleanup_tap_list_adapters:
+cleanup_pAdapterList:
     tap_free_adapter_list(pAdapterList);
-cleanup_CoInitialize:
+    return uiResult;
+}
+
+
+UINT __stdcall
+FindSystemInfo(_In_ MSIHANDLE hInstall)
+{
+#ifdef _MSC_VER
+#pragma comment(linker, DLLEXP_EXPORT)
+#endif
+
+    debug_popup(TEXT(__FUNCTION__));
+
+    BOOL bIsCoInitialized = SUCCEEDED(CoInitialize(NULL));
+
+    OPENVPNMSICA_SAVE_MSI_SESSION(hInstall);
+
+    set_openvpnserv_state(hInstall);
+    find_adapters(
+        hInstall,
+        TEXT("root\\") TEXT(TAP_WIN_COMPONENT_ID),
+        TEXT("TAPWINDOWS6ADAPTERS"),
+        TEXT("ACTIVETAPWINDOWS6ADAPTERS"));
+    find_adapters(
+        hInstall,
+        TEXT("Wintun"),
+        TEXT("WINTUNADAPTERS"),
+        TEXT("ACTIVEWINTUNADAPTERS"));
+
     if (bIsCoInitialized)
     {
         CoUninitialize();
     }
-    return uiResult;
+    return ERROR_SUCCESS;
 }
 
 
diff --git a/src/openvpnmsica/openvpnmsica.h b/src/openvpnmsica/openvpnmsica.h
index 5d140930..221d03ca 100644
--- a/src/openvpnmsica/openvpnmsica.h
+++ b/src/openvpnmsica/openvpnmsica.h
@@ -76,23 +76,17 @@  extern "C" {
 
 /**
  * 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
+ * - Sets `OPENVPNSERVICE` MSI property to PID of OpenVPN Service if running, or its EXE path if
+ *   configured for auto-start.
  *
- * @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 adapters and set TUNTAPADAPTERS and ACTIVETUNTAPADAPTERS properties with
- * semicolon delimited list of all installed TAP adapter GUIDs and active adapter GUIDs
- * respectively.
+ * - Finds existing TAP-Windows6 adapters and set TAPWINDOWS6ADAPTERS and
+ *   ACTIVETAPWINDOWS6ADAPTERS properties with semicolon delimited list of all installed adapter
+ *   GUIDs and active adapter GUIDs respectively.
+ *
+ * - Finds existing Wintun adapters and set WINTUNADAPTERS and ACTIVEWINTUNADAPTERS properties
+ *   with semicolon delimited list of all installed adapter GUIDs and active adapter GUIDs
+ *   respectively.
  *
  * @param hInstall      Handle to the installation provided to the DLL custom action
  *
@@ -100,7 +94,7 @@  FindSystemInfo(_In_ MSIHANDLE hInstall);
  *         See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa368072.aspx
  */
 DLLEXP_DECL UINT __stdcall
-FindTUNTAPAdapters(_In_ MSIHANDLE hInstall);
+FindSystemInfo(_In_ MSIHANDLE hInstall);
 
 
 /**