diff mbox series

[Openvpn-devel,v3] tapctl: generate driver-specific adapter names

Message ID 20230519082548.1714-1-lstipakov@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,v3] tapctl: generate driver-specific adapter names | expand

Commit Message

Lev Stipakov May 19, 2023, 8:25 a.m. UTC 
From: Lev Stipakov <lev@openvpn.net>

At the moment if --name is not specified, adapter names
are generated by Windows and they look a bit confusing
like "Local Area Connection 2".

This is also behavior of "Add a new <driver-name> virtual network
adapter" shortcuts.

This makes tapctl generate driver-specific names for adapters
if --name is missing, inclusing resolving duplicates. For instance
following commands:

  tapctl.exe create --hwid ovpn-dco

will create an adapter named

  OpenVPN Data Channel Offload

If the name is taken, the next one will be

  OpenVPN Data Channel Offload #1

and so on up to 100.

Fixes https://github.com/OpenVPN/openvpn/issues/337

Change-Id: Ic5afb470d14ac7b231d91f0f5de0a0046043a7e0
Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 v3:
  - use _stprintf_s instead of _tcscat_s
  - make functions static
  - ensure iResult is always assigned

 v2: fix MinGW compilation

 src/tapctl/main.c | 132 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 105 insertions(+), 27 deletions(-)

Comments

Selva Nair May 19, 2023, 3:05 p.m. UTC | #1 
Acked-by: Selva Nair <selva.nair@gmail.com>

On Fri, May 19, 2023 at 4:27 AM Lev Stipakov <lstipakov@gmail.com> wrote:

> From: Lev Stipakov <lev@openvpn.net>
>
> At the moment if --name is not specified, adapter names
> are generated by Windows and they look a bit confusing
> like "Local Area Connection 2".
>
> This is also behavior of "Add a new <driver-name> virtual network
> adapter" shortcuts.
>
> This makes tapctl generate driver-specific names for adapters
> if --name is missing, inclusing resolving duplicates. For instance
> following commands:
>
>   tapctl.exe create --hwid ovpn-dco
>
> will create an adapter named
>
>   OpenVPN Data Channel Offload
>
> If the name is taken, the next one will be
>
>   OpenVPN Data Channel Offload #1
>
> and so on up to 100.
>
> Fixes https://github.com/OpenVPN/openvpn/issues/337
>
> Change-Id: Ic5afb470d14ac7b231d91f0f5de0a0046043a7e0
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
>  v3:
>   - use _stprintf_s instead of _tcscat_s
>   - make functions static
>   - ensure iResult is always assigned
>
>  v2: fix MinGW compilation
>
>  src/tapctl/main.c | 132 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 105 insertions(+), 27 deletions(-)
>
> diff --git a/src/tapctl/main.c b/src/tapctl/main.c
> index 1194036f..d76d553c 100644
> --- a/src/tapctl/main.c
> +++ b/src/tapctl/main.c
> @@ -126,6 +126,85 @@ usage(void)
>                title_string);
>  }
>
> +/**
> + * Checks if adapter with given name doesn't already exist
> + */
> +static BOOL
> +is_adapter_name_available(LPCTSTR name, struct tap_adapter_node
> *adapter_list, BOOL log)
> +{
> +    for (struct tap_adapter_node *a = adapter_list; a; a = a->pNext)
> +    {
> +        if (_tcsicmp(name, a->szName) == 0)
> +        {
> +            if (log)
> +            {
> +                LPOLESTR adapter_id = NULL;
> +                StringFromIID((REFIID)&a->guid, &adapter_id);
> +                _ftprintf(stderr, TEXT("Adapter \"%") TEXT(PRIsLPTSTR)
> TEXT("\" already exists (GUID %")
> +                          TEXT(PRIsLPOLESTR) TEXT(").\n"), a->szName,
> adapter_id);
> +                CoTaskMemFree(adapter_id);
> +            }
> +
> +            return FALSE;
> +        }
> +    }
> +
> +    return TRUE;
> +}
> +
> +/**
> + * Returns unique adapter name based on hwid or NULL if name cannot be
> generated.
> + * Caller is responsible for freeing it.
> + */
> +static LPTSTR
> +get_unique_adapter_name(LPCTSTR hwid, struct tap_adapter_node
> *adapter_list)
> +{
> +    if (hwid == NULL)
> +    {
> +        return NULL;
> +    }
> +
> +    LPCTSTR base_name;
> +    if (_tcsicmp(hwid, TEXT("ovpn-dco")) == 0)
> +    {
> +        base_name = TEXT("OpenVPN Data Channel Offload");
> +    }
> +    else if (_tcsicmp(hwid, TEXT("wintun")) == 0)
> +    {
> +        base_name = TEXT("OpenVPN Wintun");
> +    }
> +    else if (_tcsicmp(hwid, TEXT("root\\") TEXT(TAP_WIN_COMPONENT_ID)) ==
> 0)
> +    {
> +        base_name = TEXT("OpenVPN TAP-Windows6");
> +    }
> +    else
> +    {
> +        return NULL;
> +    }
> +
> +    if (is_adapter_name_available(base_name, adapter_list, FALSE))
> +    {
> +        return _tcsdup(base_name);
> +    }
> +
> +    size_t name_len = _tcslen(base_name) + 10;
> +    LPTSTR name = malloc(name_len * sizeof(TCHAR));
> +    if (name == NULL)
> +    {
> +        return NULL;
> +    }
> +    for (int i = 1; i < 100; ++i)
> +    {
> +        _stprintf_s(name, name_len, TEXT("%ls #%d"), base_name, i);
> +
> +        if (is_adapter_name_available(name, adapter_list, FALSE))
> +        {
> +            return name;
> +        }
> +    }
> +
> +    return NULL;
> +}
>
>  /**
>   * Program entry point
> @@ -210,50 +289,49 @@ _tmain(int argc, LPCTSTR argv[])
>              iResult = 1; goto quit;
>          }
>
> -        if (szName)
> +        /* Get existing network adapters. */
> +        struct tap_adapter_node *pAdapterList = NULL;
> +        dwResult = tap_list_adapters(NULL, NULL, &pAdapterList);
> +        if (dwResult != ERROR_SUCCESS)
>          {
> -            /* Get existing network adapters. */
> -            struct tap_adapter_node *pAdapterList = NULL;
> -            dwResult = tap_list_adapters(NULL, NULL, &pAdapterList);
> -            if (dwResult != ERROR_SUCCESS)
> -            {
> -                _ftprintf(stderr, TEXT("Enumerating adapters failed
> (error 0x%x).\n"), dwResult);
> -                iResult = 1; goto create_delete_adapter;
> -            }
> +            _ftprintf(stderr, TEXT("Enumerating adapters failed (error
> 0x%x).\n"), dwResult);
> +            iResult = 1;
> +            goto create_delete_adapter;
> +        }
>
> -            /* Check for duplicates. */
> -            for (struct tap_adapter_node *pAdapter = pAdapterList;
> pAdapter; pAdapter = pAdapter->pNext)
> +        LPTSTR adapter_name = szName ? _tcsdup(szName) :
> get_unique_adapter_name(szHwId, pAdapterList);
> +        if (adapter_name)
> +        {
> +            /* Check for duplicates when name was specified,
> +             * otherwise get_adapter_default_name() takes care of it */
> +            if (szName && !is_adapter_name_available(adapter_name,
> pAdapterList, TRUE))
>              {
> -                if (_tcsicmp(szName, pAdapter->szName) == 0)
> -                {
> -                    StringFromIID((REFIID)&pAdapter->guid, &szAdapterId);
> -                    _ftprintf(stderr, TEXT("Adapter \"%")
> TEXT(PRIsLPTSTR) TEXT("\" already exists (GUID %")
> -                              TEXT(PRIsLPOLESTR) TEXT(").\n"),
> pAdapter->szName, szAdapterId);
> -                    CoTaskMemFree(szAdapterId);
> -                    iResult = 1; goto create_cleanup_pAdapterList;
> -                }
> +                iResult = 1;
> +                goto create_cleanup_pAdapterList;
>              }
>
>              /* Rename the adapter. */
> -            dwResult = tap_set_adapter_name(&guidAdapter, szName, FALSE);
> +            dwResult = tap_set_adapter_name(&guidAdapter, adapter_name,
> FALSE);
>              if (dwResult != ERROR_SUCCESS)
>              {
>                  StringFromIID((REFIID)&guidAdapter, &szAdapterId);
>                  _ftprintf(stderr, TEXT("Renaming TUN/TAP adapter %")
> TEXT(PRIsLPOLESTR)
>                            TEXT(" to \"%") TEXT(PRIsLPTSTR) TEXT("\"
> failed (error 0x%x).\n"),
> -                          szAdapterId, szName, dwResult);
> +                          szAdapterId, adapter_name, dwResult);
>                  CoTaskMemFree(szAdapterId);
>                  iResult = 1; goto quit;
>              }
> +        }
>
> -            iResult = 0;
> +        iResult = 0;
>
>  create_cleanup_pAdapterList:
> -            tap_free_adapter_list(pAdapterList);
> -            if (iResult)
> -            {
> -                goto create_delete_adapter;
> -            }
> +        free(adapter_name);
> +
> +        tap_free_adapter_list(pAdapterList);
> +        if (iResult)
> +        {
> +            goto create_delete_adapter;
>          }
>
>          /* Output adapter GUID. */
> --
> 2.23.0.windows.1
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
Gert Doering May 21, 2023, 3:50 p.m. UTC | #2 
Your patch has been applied to the master and release/2.6 branch.

I have not tested it beyond "does it compile on MinGW and GHA" - I intended
to create a few interfaces across the landscape (Win7, Win10, Win11/ARM)
but things got in the way.  Will do later.

Staring at the code for a bit and things look all reasonable - memory
handling, strdup()/free(), etc.

commit 4323b8277814495f5a9a24d3225679c6e7a971a4 (master)
commit 868286fa756f7faf768bdd7e18a44e6a92e47066 (release/2.6)
Author: Lev Stipakov
Date:   Fri May 19 11:25:48 2023 +0300

     tapctl: generate driver-specific adapter names

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <20230519082548.1714-1-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26712.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/tapctl/main.c b/src/tapctl/main.c
index 1194036f..d76d553c 100644
--- a/src/tapctl/main.c
+++ b/src/tapctl/main.c
@@ -126,6 +126,85 @@  usage(void)
               title_string);
 }
 
+/**
+ * Checks if adapter with given name doesn't already exist
+ */
+static BOOL
+is_adapter_name_available(LPCTSTR name, struct tap_adapter_node *adapter_list, BOOL log)
+{
+    for (struct tap_adapter_node *a = adapter_list; a; a = a->pNext)
+    {
+        if (_tcsicmp(name, a->szName) == 0)
+        {
+            if (log)
+            {
+                LPOLESTR adapter_id = NULL;
+                StringFromIID((REFIID)&a->guid, &adapter_id);
+                _ftprintf(stderr, TEXT("Adapter \"%") TEXT(PRIsLPTSTR) TEXT("\" already exists (GUID %")
+                          TEXT(PRIsLPOLESTR) TEXT(").\n"), a->szName, adapter_id);
+                CoTaskMemFree(adapter_id);
+            }
+
+            return FALSE;
+        }
+    }
+
+    return TRUE;
+}
+
+/**
+ * Returns unique adapter name based on hwid or NULL if name cannot be generated.
+ * Caller is responsible for freeing it.
+ */
+static LPTSTR
+get_unique_adapter_name(LPCTSTR hwid, struct tap_adapter_node *adapter_list)
+{
+    if (hwid == NULL)
+    {
+        return NULL;
+    }
+
+    LPCTSTR base_name;
+    if (_tcsicmp(hwid, TEXT("ovpn-dco")) == 0)
+    {
+        base_name = TEXT("OpenVPN Data Channel Offload");
+    }
+    else if (_tcsicmp(hwid, TEXT("wintun")) == 0)
+    {
+        base_name = TEXT("OpenVPN Wintun");
+    }
+    else if (_tcsicmp(hwid, TEXT("root\\") TEXT(TAP_WIN_COMPONENT_ID)) == 0)
+    {
+        base_name = TEXT("OpenVPN TAP-Windows6");
+    }
+    else
+    {
+        return NULL;
+    }
+
+    if (is_adapter_name_available(base_name, adapter_list, FALSE))
+    {
+        return _tcsdup(base_name);
+    }
+
+    size_t name_len = _tcslen(base_name) + 10;
+    LPTSTR name = malloc(name_len * sizeof(TCHAR));
+    if (name == NULL)
+    {
+        return NULL;
+    }
+    for (int i = 1; i < 100; ++i)
+    {
+        _stprintf_s(name, name_len, TEXT("%ls #%d"), base_name, i);
+
+        if (is_adapter_name_available(name, adapter_list, FALSE))
+        {
+            return name;
+        }
+    }
+
+    return NULL;
+}
 
 /**
  * Program entry point
@@ -210,50 +289,49 @@  _tmain(int argc, LPCTSTR argv[])
             iResult = 1; goto quit;
         }
 
-        if (szName)
+        /* Get existing network adapters. */
+        struct tap_adapter_node *pAdapterList = NULL;
+        dwResult = tap_list_adapters(NULL, NULL, &pAdapterList);
+        if (dwResult != ERROR_SUCCESS)
         {
-            /* Get existing network adapters. */
-            struct tap_adapter_node *pAdapterList = NULL;
-            dwResult = tap_list_adapters(NULL, NULL, &pAdapterList);
-            if (dwResult != ERROR_SUCCESS)
-            {
-                _ftprintf(stderr, TEXT("Enumerating adapters failed (error 0x%x).\n"), dwResult);
-                iResult = 1; goto create_delete_adapter;
-            }
+            _ftprintf(stderr, TEXT("Enumerating adapters failed (error 0x%x).\n"), dwResult);
+            iResult = 1;
+            goto create_delete_adapter;
+        }
 
-            /* Check for duplicates. */
-            for (struct tap_adapter_node *pAdapter = pAdapterList; pAdapter; pAdapter = pAdapter->pNext)
+        LPTSTR adapter_name = szName ? _tcsdup(szName) : get_unique_adapter_name(szHwId, pAdapterList);
+        if (adapter_name)
+        {
+            /* Check for duplicates when name was specified,
+             * otherwise get_adapter_default_name() takes care of it */
+            if (szName && !is_adapter_name_available(adapter_name, pAdapterList, TRUE))
             {
-                if (_tcsicmp(szName, pAdapter->szName) == 0)
-                {
-                    StringFromIID((REFIID)&pAdapter->guid, &szAdapterId);
-                    _ftprintf(stderr, TEXT("Adapter \"%") TEXT(PRIsLPTSTR) TEXT("\" already exists (GUID %")
-                              TEXT(PRIsLPOLESTR) TEXT(").\n"), pAdapter->szName, szAdapterId);
-                    CoTaskMemFree(szAdapterId);
-                    iResult = 1; goto create_cleanup_pAdapterList;
-                }
+                iResult = 1;
+                goto create_cleanup_pAdapterList;
             }
 
             /* Rename the adapter. */
-            dwResult = tap_set_adapter_name(&guidAdapter, szName, FALSE);
+            dwResult = tap_set_adapter_name(&guidAdapter, adapter_name, FALSE);
             if (dwResult != ERROR_SUCCESS)
             {
                 StringFromIID((REFIID)&guidAdapter, &szAdapterId);
                 _ftprintf(stderr, TEXT("Renaming TUN/TAP adapter %") TEXT(PRIsLPOLESTR)
                           TEXT(" to \"%") TEXT(PRIsLPTSTR) TEXT("\" failed (error 0x%x).\n"),
-                          szAdapterId, szName, dwResult);
+                          szAdapterId, adapter_name, dwResult);
                 CoTaskMemFree(szAdapterId);
                 iResult = 1; goto quit;
             }
+        }
 
-            iResult = 0;
+        iResult = 0;
 
 create_cleanup_pAdapterList:
-            tap_free_adapter_list(pAdapterList);
-            if (iResult)
-            {
-                goto create_delete_adapter;
-            }
+        free(adapter_name);
+
+        tap_free_adapter_list(pAdapterList);
+        if (iResult)
+        {
+            goto create_delete_adapter;
         }
 
         /* Output adapter GUID. */