[Openvpn-devel] [PATCH] In tap.c use DiInstallDevice to install the driver on a new adapter
From: Selva Nair
As reported in Trac 1321, additional adapter instalaltion
by tapctl.exe fails to fully setup the device node (some registry
keys missing, error in setapi.dev.log etc.).
Although the exact cause of this failure is unclear,
letting the Plug and Play subsystem handle the instalaltion
by calling DiInsatllDevice() avoids it.
We let the system automatically choose the best driver
by passing NULL for driverinfo to DiInstallDevice().
This also eliminates the need for enumerating all drivers
in the Net class and selecting a matching one.
Somehow mingw-w64 fails to find DiInstallDriver() in
newdev.lib although the header does define it. Use LoadLibrary()
to locate it at run time (available in Vista and above).
Built using mingw and tested both the msi installer (code shared
with libopenvpnmscia.dll) and tapctl.exe on Windows 10 64 bit.
Fixes: Trac #1321
Signed-off-by: Selva Nair
---
config-msvc.h| 1 +
src/tapctl/tap.c | 231 +++
2 files changed, 78 insertions(+), 154 deletions(-)
diff --git a/config-msvc.h b/config-msvc.h
index 8ef4897..f199bb2 100644
--- a/config-msvc.h
+++ b/config-msvc.h
@@ -112,6 +112,7 @@
#define HAVE_EC_GROUP_ORDER_BITS 1
#define OPENSSL_NO_EC 1
#define HAVE_EVP_CIPHER_CTX_RESET 1
+#define HAVE_DIINSTALLDEVICE 1
#define PATH_SEPARATOR '\\'
#define PATH_SEPARATOR_STR "\\"
diff --git a/src/tapctl/tap.c b/src/tapctl/tap.c
index 7cb3ded..e36d132 100644
--- a/src/tapctl/tap.c
+++ b/src/tapctl/tap.c
@@ -33,18 +33,69 @@
#include
#include
#include
+#include
#ifdef _MSC_VER
#pragma comment(lib, "advapi32.lib")
#pragma comment(lib, "ole32.lib")
#pragma comment(lib, "setupapi.lib")
+#pragma comment(lib, "newdev.lib")
#endif
+
const static GUID GUID_DEVCLASS_NET = { 0x4d36e972L, 0xe325, 0x11ce, { 0xbf,
0xc1, 0x08, 0x00, 0x2b, 0xe1, 0x03, 0x18 } };
const static TCHAR szAdapterRegKeyPathTemplate[] =
TEXT("SYSTEM\\CurrentControlSet\\Control\\Network\\%") TEXT(PRIsLPOLESTR)
TEXT("\\%") TEXT(PRIsLPOLESTR) TEXT("\\Connection");
#define ADAPTER_REGKEY_PATH_MAX
(_countof(TEXT("SYSTEM\\CurrentControlSet\\Control\\Network\\")) - 1 + 38 +
_countof(TEXT("\\")) - 1 + 38 + _countof(TEXT("\\Connection")))
+/**
+ * Dynamically load a library and find a function in it
+ *
+ * @param libname Name of the library to load
+ * @param funcnameName of the function to find
+ * @param m Pointer to a module. On return this is set to the
+ *the handle to the loaded library. The caller must
+ *free it by calling FreeLibrary() if not NULL.
+ *
+ * @returnPointer to the function
+ *NULL on error -- use GetLastError() to find the error
code.
+ *
+ **/
+static void *
+find_function(const WCHAR *libname, const char *funcname, HMODULE *m)
+{
+WCHAR libpath[MAX_PATH];
+void *fptr = NULL;
+
+/* Make sure the dll is loaded from the system32 folder */
+if (!GetSystemDirectoryW(libpath, _countof(libpath)))
+{
+ return NULL;
+}
+
+size_t len = _countof(libpath) - wcslen(libpath) - 1;
+if (len < wcslen(libname) + 1)
+{
+ SetLastError(ERROR_INSUFFICIENT_BUFFER);
+ return NULL;
+}
+wcsncat(libpath, L"\\", len);
+wcsncat(libpath, libname, len-1);
+
+*m = LoadLibraryW(libpath);
+if (*m == NULL)
+{
+ return NULL;
+}
+fptr = GetProcAddress(*m, funcname);
+if (!fptr)
+{
+ FreeLibrary(*m);
+ *m = NULL;
+ return NULL;
+}
+return fptr;
+}
/**
* Returns length of string of strings
@@ -678,6 +729,7 @@ tap_create_adapter(
_Out_ LPGUID pguidAdapter)
{
DWORD dwResult;
+HMODULE libnewdev = NULL;
if (szHwId == NULL
|| pbRebootRequired == NULL
@@ -746,129 +798,7 @@ tap_create_adapter(
goto cleanup_hDevInfoList;
}
-/* Search for the driver. */
-if (!SetupDiBuildDriverInfoList(
-hDevInfoList,
-&devinfo_data,
-SPDIT_CLASSDRIVER))
-{
-dwResult = GetLastError();
-msg(M_NONFATAL, "%s: SetupDiBuildDriverInfoList failed", __FUNCTION__);
-goto cleanup_hDevInfoList;
-}
-DWORDLONG dwlDriverVersion = 0;
-DWORD drvinfo_detail_data_size = sizeof(SP_DRVINFO_DETAIL_DATA) + 0x100;
-SP_DRVINFO_DETAIL_DATA *drvinfo_detail_data = (SP_DRVINFO_DETAIL_DATA
*)malloc(drvinfo_detail_data_size);
-if (drvinfo_detail_data == NULL)
-{
-msg(M_FATAL, "%s: malloc(%u) failed", __FUNCTION__,
drvinfo_detail_data_size);
-dwResult = ERROR_OUTOFMEMORY; goto cleanup_DriverInfoList;
-}
-
-for (DWORD dwIndex = 0;; dwIndex++)
-{
-/* Get a driver from the list. */
-SP_DRVINFO_DATA drvinfo_data = { .cbSize = sizeof(SP_DRVINFO_DATA) };
-if (!SetupDiEnumDriverInfo(
-hDevInfoList,
-&devinfo_data,
-SPDIT_CLASSDRIVER,
-
Re: [Openvpn-devel] [PATCH v2] openvpnmsica: make adapter renaming non-fatal
Hi,
> -Original Message-
> From: Lev Stipakov
> Sent: Wednesday, September 2, 2020 11:37 PM
> To: openvpn-devel@lists.sourceforge.net
> Cc: Lev Stipakov
> Subject: [Openvpn-devel] [PATCH v2] openvpnmsica: make adapter renaming
> non-fatal
>
> From: Lev Stipakov
>
> For some users renaming adapter
>
> Local Area Connection > OpenVPN TAP-Windows6
>
> mysteriously fails, see https://github.com/OpenVPN/openvpn-
> build/issues/187
>
> Since renaming is just a "nice to have", make it non-fatal
> and, in case of error, only log message and don't display messagebox.
>
> Signed-off-by: Lev Stipakov
> ---
>
> v2: only log error, don't display messagebox
>
> src/openvpnmsica/dllmain.c | 2 +-
> src/openvpnmsica/openvpnmsica.c | 9 +++--
> src/tapctl/main.c | 2 +-
> src/tapctl/tap.c| 11 +++
> src/tapctl/tap.h| 6 +-
> 5 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/src/openvpnmsica/dllmain.c b/src/openvpnmsica/dllmain.c
> index 201fd9af..34946ed8 100644
> --- a/src/openvpnmsica/dllmain.c
> +++ b/src/openvpnmsica/dllmain.c
> @@ -193,6 +193,6 @@ x_msg_va(const unsigned int flags, const char
> *format, va_list arglist)
> }
> }
>
> -MsiProcessMessage(s->hInstall, INSTALLMESSAGE_ERROR, hRecordProg);
> +MsiProcessMessage(s->hInstall, (flags & M_WARN) ?
> INSTALLMESSAGE_INFO : INSTALLMESSAGE_ERROR, hRecordProg);
> MsiCloseHandle(hRecordProg);
> }
> diff --git a/src/openvpnmsica/openvpnmsica.c
> b/src/openvpnmsica/openvpnmsica.c
> index 31e90bd2..f203f736 100644
> --- a/src/openvpnmsica/openvpnmsica.c
> +++ b/src/openvpnmsica/openvpnmsica.c
> @@ -1096,12 +1096,9 @@ ProcessDeferredAction(_In_ MSIHANDLE hInstall)
> dwResult = tap_create_adapter(NULL, NULL, szHardwareId,
> &bRebootRequired, &guidAdapter);
> if (dwResult == ERROR_SUCCESS)
> {
> -/* Set adapter name. */
> -dwResult = tap_set_adapter_name(&guidAdapter, szName);
> -if (dwResult != ERROR_SUCCESS)
> -{
> -tap_delete_adapter(NULL, &guidAdapter,
> &bRebootRequired);
> -}
> +/* Set adapter name. May fail on some machines, but
> that is not critical - use silent
> + flag to mute messagebox and print error only to log
> */
> +tap_set_adapter_name(&guidAdapter, szName, TRUE);
> }
> }
> else if (wcsncmp(szArg[i], L"deleteN=", 8) == 0)
> diff --git a/src/tapctl/main.c b/src/tapctl/main.c
> index 31bb2ec7..d5bc7290 100644
> --- a/src/tapctl/main.c
> +++ b/src/tapctl/main.c
> @@ -237,7 +237,7 @@ _tmain(int argc, LPCTSTR argv[])
> }
>
> /* Rename the adapter. */
> -dwResult = tap_set_adapter_name(&guidAdapter, szName);
> +dwResult = tap_set_adapter_name(&guidAdapter, szName,
> FALSE);
> if (dwResult != ERROR_SUCCESS)
> {
> StringFromIID((REFIID)&guidAdapter, &szAdapterId);
> diff --git a/src/tapctl/tap.c b/src/tapctl/tap.c
> index 7cb3dedc..0dfe239f 100644
> --- a/src/tapctl/tap.c
> +++ b/src/tapctl/tap.c
> @@ -1140,9 +1140,12 @@ ExecCommand(const WCHAR* cmdline)
> DWORD
> tap_set_adapter_name(
> _In_ LPCGUID pguidAdapter,
> -_In_ LPCTSTR szName)
> +_In_ LPCTSTR szName,
> +_In_ BOOL bSilent)
> {
> DWORD dwResult;
> +int msg_flag = bSilent ? M_WARN : M_NONFATAL;
> +msg_flag |= M_ERRNO;
>
> if (pguidAdapter == NULL || szName == NULL)
> {
> @@ -1176,7 +1179,7 @@ tap_set_adapter_name(
> if (dwResult != ERROR_SUCCESS)
> {
> SetLastError(dwResult); /* MSDN does not mention RegOpenKeyEx()
> to set GetLastError(). But we do have an error code. Set last error
> manually. */
> -msg(M_NONFATAL | M_ERRNO, "%s: RegOpenKeyEx(HKLM, \"%"
> PRIsLPTSTR "\") failed", __FUNCTION__, szRegKey);
> +msg(msg_flag, "%s: RegOpenKeyEx(HKLM, \"%" PRIsLPTSTR "\")
> failed", __FUNCTION__, szRegKey);
> goto cleanup_szAdapterId;
> }
>
> @@ -1185,7 +1188,7 @@ tap_set_adapter_name(
> if (dwResult != ERROR_SUCCESS)
> {
> SetLastError(dwResult);
> -msg(M_NONFATAL | M_ERRNO, "%s: Error reading adapter name",
> __FUNCTION__);
> +msg(msg_flag, "%s: Error reading adapter name", __FUNCTION__);
> goto cleanup_hKey;
> }
>
> @@ -1203,7 +1206,7 @@ tap_set_adapter_name(
> if (dwResult != ERROR_SUCCESS)
> {
> SetLastError(dwResult);
> -msg(M_NONFATAL | M_ERRNO, "%s: Error renaming adapter",
> __FUNCTION__);
> +msg(msg_flag, "%s: Error renaming adapter", __FUNCTION__);
> goto cleanup_hKey;
> }
>
> diff --git a/src/tapctl/tap.h b/src/tapctl/tap.h
> index 102de32d..1f531cf2 100644
> --- a/src/tapctl/tap.h
> +++ b/src/tapctl/tap.h
> @@ -117,13 +117,17 @@ tap_enable_adapter(
> * @par
Re: [Openvpn-devel] On tap-windows6 adapter installation by tapctl.exe
Hi Lev,
Thanks for confirming. What you tested is exactly what I have in mind.
I suppose you tested it using MSVC. I recall when I worked on creating tap
adapters on the fly (patch abandoned for lack of time) some functions in
newdev.dll did not resolve with mingw and I always had to load them on
runtime. I'll check those hard corners again and submit a patch soonish
(hopefully today).
Selva
On Thu, Sep 3, 2020 at 8:11 AM Lev Stipakov wrote:
> Hi,
>
> >
> > As per setupapi.dev.log, it appears that step 4 (d) is failing with some
> access error to the driver store unless elevated to SYSTEM (see Trac 1321).
> This leaves the adapter not fully configured. Hard to say exactly what
> fails as none of the function calls return any error.
>
> I can confirm that. This call
>
> /* Install the device. */
> if (!SetupDiCallClassInstaller(
> DIF_INSTALLDEVICE,
> hDevInfoList,
> &devinfo_data))
> {
> dwResult = GetLastError();
> msg(M_NONFATAL | M_ERRNO, "%s:
> SetupDiCallClassInstaller(DIF_INSTALLDEVICE) failed", __FUNCTION__);
> goto cleanup_remove_device;
> }
>
> produces this error in setupapi.dev.log
>
> >>> [Configure Driver Package -
> c:\windows\system32\driverstore\filerepository\oemvista.inf_amd64_8a00bc07868b5df3\oemvista.inf]
> >>> Section start 2020/09/03 12:08:49.525
> cmd: "C:\Users\lev\Projects\openvpn\x64-Output\Debug\tapctl.exe"
> create
> sto: Source Filter = tap0901.ndi
> sto: Target Filter = ROOT\NET\0004
> inf: Class GUID = {4d36e972-e325-11ce-bfc1-08002be10318}
> inf: Class Options = Configurable
> !!! idb: Failed to open driver package object
> 'oemvista.inf_amd64_8a00bc07868b5df3'. Error = 0x0005
> <<< Section end 2020/09/03 12:08:49.558
> <<< [Exit status: FAILURE(0x0005)]
>
>
> >>> [Restart Device - ROOT\NET\0004]
> >>> Section start 2020/09/03 12:08:49.565
> cmd: "C:\Users\lev\Projects\openvpn\x64-Output\Debug\tapctl.exe"
> create
> dvi: Device Status: 0x01802501 [0x0e - 0x]
> !dvi: Device restart was skipped (NEEDREBOOT/NEEDRESTART).
> <<< Section end 2020/09/03 12:08:49.588
> <<< [Exit status: SUCCESS]
>
> > I propose to replace 4(d) with a call to DiInstallDevice() instead. My
> tests show this completes without error.
>
> I can confirm that too. This patch
>
> diff --git a/src/tapctl/tap.c b/src/tapctl/tap.c
> index 7cb3dedc..8d071320 100644
> --- a/src/tapctl/tap.c
> +++ b/src/tapctl/tap.c
> @@ -29,6 +29,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -38,6 +39,7 @@
> #pragma comment(lib, "advapi32.lib")
> #pragma comment(lib, "ole32.lib")
> #pragma comment(lib, "setupapi.lib")
> +#pragma comment(lib, "newdev.lib")
> #endif
>
> const static GUID GUID_DEVCLASS_NET = { 0x4d36e972L, 0xe325, 0x11ce,
> { 0xbf, 0xc1, 0x08, 0x00, 0x2b, 0xe1, 0x03, 0x18 } };
> @@ -900,19 +902,13 @@ tap_create_adapter(
> }
>
> /* Install the device. */
> -if (!SetupDiCallClassInstaller(
> -DIF_INSTALLDEVICE,
> -hDevInfoList,
> -&devinfo_data))
> +if (!DiInstallDevice(NULL, hDevInfoList, &devinfo_data, NULL, 0,
> pbRebootRequired))
> {
> dwResult = GetLastError();
> -msg(M_NONFATAL | M_ERRNO, "%s:
> SetupDiCallClassInstaller(DIF_INSTALLDEVICE) failed", __FUNCTION__);
> +msg(M_NONFATAL | M_ERRNO, "%s: DiInstallDevice failed",
> __FUNCTION__);
> goto cleanup_remove_device;
> }
>
> -/* Check if a system reboot is required. (Ignore errors) */
> -check_reboot(hDevInfoList, &devinfo_data, pbRebootRequired);
> -
> /* Get network adapter ID from registry. Retry for max 30sec. */
> dwResult = get_net_adapter_guid(hDevInfoList, &devinfo_data, 30,
> pguidAdapter);
>
> works for both tapctl and MSI installer - created adapters are usable
> by non-admin.
>
> >
> > This also has an advantage that we could call it with driver_info = NULL
> which will force the system to use the latest matching driver. That would
> also eliminate step 3 which is right now very inefficient, though not
> required to fix the problem at hand.
>
> Yeah, I also works for me without
>
> for (DWORD dwIndex = 0;; dwIndex++)
> {
>
> loop.
>
> >
> > If this sounds sane, I'll submit a patch.
>
> Please do!
>
> --
> -Lev
>
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] On tap-windows6 adapter installation by tapctl.exe
Hi,
>
> As per setupapi.dev.log, it appears that step 4 (d) is failing with some
> access error to the driver store unless elevated to SYSTEM (see Trac 1321).
> This leaves the adapter not fully configured. Hard to say exactly what fails
> as none of the function calls return any error.
I can confirm that. This call
/* Install the device. */
if (!SetupDiCallClassInstaller(
DIF_INSTALLDEVICE,
hDevInfoList,
&devinfo_data))
{
dwResult = GetLastError();
msg(M_NONFATAL | M_ERRNO, "%s:
SetupDiCallClassInstaller(DIF_INSTALLDEVICE) failed", __FUNCTION__);
goto cleanup_remove_device;
}
produces this error in setupapi.dev.log
>>> [Configure Driver Package -
>>> c:\windows\system32\driverstore\filerepository\oemvista.inf_amd64_8a00bc07868b5df3\oemvista.inf]
>>> Section start 2020/09/03 12:08:49.525
cmd: "C:\Users\lev\Projects\openvpn\x64-Output\Debug\tapctl.exe" create
sto: Source Filter = tap0901.ndi
sto: Target Filter = ROOT\NET\0004
inf: Class GUID = {4d36e972-e325-11ce-bfc1-08002be10318}
inf: Class Options = Configurable
!!! idb: Failed to open driver package object
'oemvista.inf_amd64_8a00bc07868b5df3'. Error = 0x0005
<<< Section end 2020/09/03 12:08:49.558
<<< [Exit status: FAILURE(0x0005)]
>>> [Restart Device - ROOT\NET\0004]
>>> Section start 2020/09/03 12:08:49.565
cmd: "C:\Users\lev\Projects\openvpn\x64-Output\Debug\tapctl.exe" create
dvi: Device Status: 0x01802501 [0x0e - 0x]
!dvi: Device restart was skipped (NEEDREBOOT/NEEDRESTART).
<<< Section end 2020/09/03 12:08:49.588
<<< [Exit status: SUCCESS]
> I propose to replace 4(d) with a call to DiInstallDevice() instead. My tests
> show this completes without error.
I can confirm that too. This patch
diff --git a/src/tapctl/tap.c b/src/tapctl/tap.c
index 7cb3dedc..8d071320 100644
--- a/src/tapctl/tap.c
+++ b/src/tapctl/tap.c
@@ -29,6 +29,7 @@
#include
#include
+#include
#include
#include
#include
@@ -38,6 +39,7 @@
#pragma comment(lib, "advapi32.lib")
#pragma comment(lib, "ole32.lib")
#pragma comment(lib, "setupapi.lib")
+#pragma comment(lib, "newdev.lib")
#endif
const static GUID GUID_DEVCLASS_NET = { 0x4d36e972L, 0xe325, 0x11ce,
{ 0xbf, 0xc1, 0x08, 0x00, 0x2b, 0xe1, 0x03, 0x18 } };
@@ -900,19 +902,13 @@ tap_create_adapter(
}
/* Install the device. */
-if (!SetupDiCallClassInstaller(
-DIF_INSTALLDEVICE,
-hDevInfoList,
-&devinfo_data))
+if (!DiInstallDevice(NULL, hDevInfoList, &devinfo_data, NULL, 0,
pbRebootRequired))
{
dwResult = GetLastError();
-msg(M_NONFATAL | M_ERRNO, "%s:
SetupDiCallClassInstaller(DIF_INSTALLDEVICE) failed", __FUNCTION__);
+msg(M_NONFATAL | M_ERRNO, "%s: DiInstallDevice failed", __FUNCTION__);
goto cleanup_remove_device;
}
-/* Check if a system reboot is required. (Ignore errors) */
-check_reboot(hDevInfoList, &devinfo_data, pbRebootRequired);
-
/* Get network adapter ID from registry. Retry for max 30sec. */
dwResult = get_net_adapter_guid(hDevInfoList, &devinfo_data, 30,
pguidAdapter);
works for both tapctl and MSI installer - created adapters are usable
by non-admin.
>
> This also has an advantage that we could call it with driver_info = NULL
> which will force the system to use the latest matching driver. That would
> also eliminate step 3 which is right now very inefficient, though not
> required to fix the problem at hand.
Yeah, I also works for me without
for (DWORD dwIndex = 0;; dwIndex++)
{
loop.
>
> If this sounds sane, I'll submit a patch.
Please do!
--
-Lev
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Fix --remote protocol can't be set without port argument
According client-options.rst additional argumets ``port`` and ``proto``
are both optional and it's allowed to have port absent and protocol set:
--remote args
Examples:
remote server.example.net tcp
But when protocol is set without preceeding port argument, it is being
misparsed as a port with subsequent error:
RESOLVE: Cannot resolve host address: server.example.net:tcp
(Servname not supported for ai_socktype)
Since protocol names are predefined and don't match service names, fix
this behavior by checking second argument for valid protocol first.
Signed-off-by: Vladislav Grishenko
---
src/openvpn/options.c | 24 +---
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 8bf82c57..02ac08d8 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -5682,16 +5682,26 @@ add_option(struct options *options,
re.remote = p[1];
if (p[2])
{
-re.remote_port = p[2];
-if (p[3])
+/* Since port is optional, second parameter can be a protocol */
+int proto = ascii2proto(p[2]);
+sa_family_t af = ascii2af(p[2]);
+if (proto < 0)
{
-const int proto = ascii2proto(p[3]);
-const sa_family_t af = ascii2af(p[3]);
-if (proto < 0)
+/* Second is not proto, port then. Protocol should be third */
+re.remote_port = p[2];
+if (p[3])
{
-msg(msglevel, "remote: bad protocol associated with host
%s: '%s'", p[1], p[3]);
-goto err;
+proto = ascii2proto(p[3]);
+af = ascii2af(p[3]);
+if (proto < 0)
+{
+msg(msglevel, "remote: bad protocol associated with
host %s: '%s'", p[1], p[3]);
+goto err;
+}
}
+}
+if (proto >= 0)
+{
re.proto = proto;
re.af = af;
}
--
2.17.1
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
