Don't rely on stdout to rename newly created adapter

2 files changed, 74 insertions, 129 deletions

diff --git a/dist-assets/windows/installer.nsh b/dist-assets/windows/installer.nsh
index d44b0a7aae..d67de6963c 100644
--- a/dist-assets/windows/installer.nsh
+++ b/dist-assets/windows/installer.nsh
@@ -95,66 +95,6 @@
 !define ExtractWintun '!insertmacro "ExtractWintun"'
 
 #
-# ForceRenameAdapter
-#
-# For when there's a broken TAP adapter present, such that the adapter name
-# we're trying to use is already taken.
-#
-# This function will iteratively find a name that is not taken.
-# Names tried follow the pattern "NewAdapterBaseName-1", "NewAdapterBaseName-2", etc.
-#
-# Returns: 0 in $R0 on success, otherwise an error message in $R0
-#
-!macro ForceRenameAdapter CurrentAdapterName NewAdapterBaseName
-
-	Var /GLOBAL ForceRenameAdapter_Counter
-
-	log::Log "ForceRenameAdapter()"
-
-	Push $0
-	Push $1
-
-	Push 0
-	Pop $ForceRenameAdapter_Counter
-
-	ForceRenameAdapter_retry:
-
-	${If} $ForceRenameAdapter_Counter == 10
-		StrCpy $R0 "Exhausted namespace when forcing adapter rename"
-		log::Log $R0
-		Goto ForceRenameAdapter_return
-	${EndIf}
-
-	IntOp $ForceRenameAdapter_Counter $ForceRenameAdapter_Counter + 1
-	StrCpy $0 "${NewAdapterBaseName}-$ForceRenameAdapter_Counter"
-	log::Log "Renaming adapter to $\"$0$\""
-
-	nsExec::ExecToStack '"$SYSDIR\netsh.exe" interface set interface name = "${CurrentAdapterName}" newname = "$0"'
-
-	Pop $0
-	Pop $1
-
-	${If} $0 != 0
-		StrCpy $R0 "Failed to rename virtual adapter: error $0"
-		log::LogWithDetails $R0 $1
-		Goto ForceRenameAdapter_retry
-	${EndIf}
-
-	log::Log "ForceRenameAdapter() completed successfully"
-
-	Push 0
-	Pop $R0
-
-	ForceRenameAdapter_return:
-
-	Pop $1
-	Pop $0
-
-!macroend
-
-!define ForceRenameAdapter '!insertmacro "ForceRenameAdapter"'
-
-#
 # RemoveBrandedTap
 #
 # Try to remove the Mullvad TAP adapter driver
@@ -221,8 +161,6 @@
 #
 !macro InstallTapDriver
 
-	Var /GLOBAL InstallTapDriver_TapName
-
 	log::Log "InstallTapDriver()"
 	
 	Push $0
@@ -261,39 +199,6 @@
 		Goto InstallTapDriver_return
 	${EndIf}
 
-	log::Log "Identifying recently added adapter"
-	nsExec::ExecToStack '"$TEMP\tap-driver\driverlogic.exe" find-tap'
-
-	Pop $0
-	Pop $1
-
-	${If} $0 != ${DL_GENERAL_SUCCESS}
-		StrCpy $R0 "Failed to identify new adapter: $1"
-		log::Log $R0
-		Goto InstallTapDriver_return
-	${EndIf}
-
-	StrCpy $InstallTapDriver_TapName $1
-	
-	log::Log "New virtual adapter is named $\"$1$\""
-	log::Log "Renaming adapter to $\"Mullvad$\""
-
-	nsExec::ExecToStack '"$SYSDIR\netsh.exe" interface set interface name = "$1" newname = "Mullvad"'
-
-	Pop $0
-	Pop $1
-
-	${If} $0 != 0
-		StrCpy $R0 "Failed to rename virtual adapter: error $0"
-		log::LogWithDetails $R0 $1
-
-		${ForceRenameAdapter} $InstallTapDriver_TapName "Mullvad"
-
-		${If} $R0 != 0
-			Goto InstallTapDriver_return
-		${EndIf}
-	${EndIf}
-
 	log::Log "InstallTapDriver() completed successfully"
 	
 	Push 0
diff --git a/windows/driverlogic/src/driverlogic.cpp b/windows/driverlogic/src/driverlogic.cpp
index e426cc2243..f969ef24b1 100644
--- a/windows/driverlogic/src/driverlogic.cpp
+++ b/windows/driverlogic/src/driverlogic.cpp
@@ -288,7 +288,7 @@ bool DeleteDevice(HDEVINFO devInfo, const SP_DEVINFO_DATA &devInfoData)
 	return true;
 }
 
-void ForEachDevice(const std::wstring &tapHardwareId, std::function<void(HDEVINFO, const SP_DEVINFO_DATA &)> func)
+void ForEachNetworkDevice(const std::optional<std::wstring> hwId, std::function<void(HDEVINFO, const SP_DEVINFO_DATA &)> func)
 {
 	HDEVINFO devInfo = SetupDiGetClassDevsW(
 		&GUID_DEVCLASS_NET,
@@ -325,37 +325,40 @@ void ForEachDevice(const std::wstring &tapHardwareId, std::function<void(HDEVINF
 			THROW_WINDOWS_ERROR(lastError, "Enumerating network adapters");
 		}
 
-		try
+		if (hwId.has_value())
 		{
-			const auto hardwareId = GetDeviceRegistryStringProperty(devInfo, devInfoData, SPDRP_HARDWAREID);
+			try
+			{
+				const auto hardwareId = GetDeviceRegistryStringProperty(devInfo, devInfoData, SPDRP_HARDWAREID);
 
-			if (!hardwareId.has_value() ||
-				0 != tapHardwareId.compare(hardwareId.value()))
+				if (!hardwareId.has_value() ||
+					0 != hwId->compare(hardwareId.value()))
+				{
+					continue;
+				}
+			}
+			catch (const std::exception & e)
 			{
+				//
+				// Skip this adapter
+				//
+
+				std::wcerr << L"Skipping TAP adapter due to exception caught while iterating: "
+					<< common::string::ToWide(e.what()) << std::endl;
 				continue;
 			}
 		}
-		catch (const std::exception & e)
-		{
-			//
-			// Skip this adapter
-			//
-
-			std::cerr << "Skipping TAP adapter due to exception caught while iterating: "
-				<< e.what() << std::endl;
-			continue;
-		}
 
 		func(devInfo, devInfoData);
 	}
 }
 
-std::set<NetworkAdapter> GetTapAdapters(const std::wstring &tapHardwareId)
+std::set<NetworkAdapter> GetNetworkAdapters(const std::optional<std::wstring> hardwareId)
 {
 	std::set<NetworkAdapter> adapters;
 	common::network::Nci nci;
 
-	ForEachDevice(tapHardwareId, [&](HDEVINFO devInfo, const SP_DEVINFO_DATA &devInfoData) {
+	ForEachNetworkDevice(hardwareId, [&](HDEVINFO devInfo, const SP_DEVINFO_DATA &devInfoData) {
 		try
 		{
 			//
@@ -378,8 +381,8 @@ std::set<NetworkAdapter> GetTapAdapters(const std::wstring &tapHardwareId)
 			// Skip this adapter
 			//
 
-			std::cerr << "Skipping TAP adapter due to exception caught while iterating: "
-				<< e.what() << std::endl;
+			std::wcerr << L"Skipping adapter due to exception caught while iterating: "
+				<< common::string::ToWide(e.what()) << std::endl;
 		}
 	});
 
@@ -491,6 +494,42 @@ ATTEMPT_UPDATE:
 		<< rebootRequired;
 }
 
+//
+// Broken adapters may use our "Mullvad" name, so find one that is not in use.
+// NOTE: Enumerating adapters first and picking the next free name is not sufficient,
+//       because the broken TAP may not be included.
+//
+void RenameAdapterToMullvad(const NetworkAdapter &adapter)
+{
+	common::network::Nci nci;
+
+	try
+	{
+		nci.setConnectionName(common::Guid::FromString(adapter.guid), TAP_BASE_ALIAS);
+		return;
+	}
+	catch (...)
+	{
+	}
+
+	for (int i = 1; i < 10; i++)
+	{
+		std::wstringstream ss;
+		ss << TAP_BASE_ALIAS << L"-" << i;
+
+		try
+		{
+			nci.setConnectionName(common::Guid::FromString(adapter.guid), ss.str().c_str());
+			return;
+		}
+		catch (...)
+		{
+		}
+	}
+
+	THROW_ERROR("Exhausted TAP adapter namespace");
+}
+
 std::optional<NetworkAdapter> FindMullvadAdapter(const std::set<NetworkAdapter> &tapAdapters)
 {
 	if (tapAdapters.empty())
@@ -523,7 +562,7 @@ std::optional<NetworkAdapter> FindMullvadAdapter(const std::set<NetworkAdapter>
 	// Look for TAP adapter with alias "Mullvad-1", "Mullvad-2", etc.
 	//
 
-	for (auto i = 0; i < 10; ++i)
+	for (auto i = 1; i < 10; ++i)
 	{
 		std::wstringstream ss;
 
@@ -544,7 +583,7 @@ std::optional<NetworkAdapter> FindMullvadAdapter(const std::set<NetworkAdapter>
 
 NetworkAdapter FindBrandedTap()
 {
-	std::set<NetworkAdapter> added = GetTapAdapters(TAP_HARDWARE_ID);
+	std::set<NetworkAdapter> added = GetNetworkAdapters(TAP_HARDWARE_ID);
 
 	if (added.empty())
 	{
@@ -562,7 +601,7 @@ NetworkAdapter FindBrandedTap()
 
 void RemoveTapDriver(const std::wstring &tapHardwareId)
 {
-	ForEachDevice(tapHardwareId, [](HDEVINFO devInfo, const SP_DEVINFO_DATA &devInfoData) {
+	ForEachNetworkDevice(tapHardwareId, [](HDEVINFO devInfo, const SP_DEVINFO_DATA &devInfoData) {
 		try
 		{
 			DeleteDevice(devInfo, devInfoData);
@@ -573,15 +612,15 @@ void RemoveTapDriver(const std::wstring &tapHardwareId)
 			// Skip this adapter
 			//
 
-			std::cerr << "Skipping TAP adapter due to exception caught while iterating: "
-				<< e.what() << std::endl;
+			std::wcerr << L"Skipping TAP adapter due to exception caught while iterating: "
+				<< common::string::ToWide(e.what()) << std::endl;
 		}
 	});
 }
 
 void DeleteVanillaMullvadAdapter()
 {
-	auto tapAdapters = GetTapAdapters(DEPRECATED_TAP_HARDWARE_ID);
+	auto tapAdapters = GetNetworkAdapters(DEPRECATED_TAP_HARDWARE_ID);
 	std::optional<NetworkAdapter> mullvadAdapter = FindMullvadAdapter(tapAdapters);
 
 	if (!mullvadAdapter.has_value())
@@ -592,7 +631,12 @@ void DeleteVanillaMullvadAdapter()
 	const auto mullvadGuid = mullvadAdapter.value().guid;
 	bool deletedAdapter = false;
 
-	ForEachDevice(DEPRECATED_TAP_HARDWARE_ID, [&](HDEVINFO devInfo, const SP_DEVINFO_DATA &devInfoData) {
+	//
+	// Enumerate over all network devices with the hardware ID DEPRECATED_TAP_HARDWARE_ID,
+	// and delete any adapter whose GUID matches that of the "Mullvad" adapter.
+	//
+
+	ForEachNetworkDevice(DEPRECATED_TAP_HARDWARE_ID, [&](HDEVINFO devInfo, const SP_DEVINFO_DATA &devInfoData) {
 		try
 		{
 			if (0 == GetNetCfgInstanceId(devInfo, devInfoData).compare(mullvadGuid))
@@ -606,8 +650,8 @@ void DeleteVanillaMullvadAdapter()
 			// Skip this adapter
 			//
 
-			std::cerr << "Skipping TAP adapter due to exception caught while iterating: "
-				<< e.what() << std::endl;
+			std::wcerr << L"Skipping TAP adapter due to exception caught while iterating: "
+				<< common::string::ToWide(e.what()) << std::endl;
 			return;
 		}
 	});
@@ -638,6 +682,7 @@ int wmain(int argc, const wchar_t * argv[], const wchar_t * [])
 
 			CreateTapDevice();
 			UpdateTapDriver(argv[2]);
+			RenameAdapterToMullvad(FindBrandedTap());
 		}
 		else if (0 == _wcsicmp(argv[1], L"update"))
 		{
@@ -661,11 +706,6 @@ int wmain(int argc, const wchar_t * argv[], const wchar_t * [])
 		{
 			DeleteVanillaMullvadAdapter();
 		}
-		else if (0 == _wcsicmp(argv[1], L"find-tap"))
-		{
-			const auto tap = FindBrandedTap();
-			std::wcout << tap.alias;
-		}
 		else
 		{
 			goto INVALID_ARGUMENTS;
@@ -673,7 +713,7 @@ int wmain(int argc, const wchar_t * argv[], const wchar_t * [])
 	}
 	catch (const std::exception &e)
 	{
-		std::cerr << e.what();
+		std::wcerr << common::string::ToWide(e.what());
 		return GENERAL_ERROR;
 	}
 	catch (...)