Fix a bunch of Windows tap issues. Turns out NDIS6 allowed us to ditch some NDIS5 cruft, but I did have to add one hack specific to that one instead. Seems reliable now.

osdep/WindowsEthernetTap.cpp

@@ -53,7 +53,7 @@
 #include "WindowsEthernetTap.hpp"
 #include "OSUtils.hpp"
 
-#include "..\windows\TapDriver\tap-windows.h"
+#include "..\windows\TapDriver6\tap-windows.h"
 
 // ff:ff:ff:ff:ff:ff with no ADI
 //static const ZeroTier::MulticastGroup _blindWildcardMulticastGroup(ZeroTier::MAC(0xff),0);
@@ -89,11 +89,16 @@ public:
 };
 static const WindowsEthernetTapEnv WINENV;
 
-} // anonymous namespace
-
 // Only create or delete devices one at a time
 static Mutex _systemTapInitLock;
 
+// Set to true if existing taps should close and reopen due to new device creation
+// This is a hack to get around what seems to be a bug that causes existing
+// devices to go into a coma after new device creation.
+static volatile bool _needReset = false;
+
+} // anonymous namespace
+
 WindowsEthernetTap::WindowsEthernetTap(
 	const char *hp,
 	const MAC &mac,
@@ -124,6 +129,9 @@ WindowsEthernetTap::WindowsEthernetTap(
 
 	Mutex::Lock _l(_systemTapInitLock);
 
+	// Use NDIS5 if it's installed, since we don't want to switch out the driver on
+	// pre-existing installs (yet). We won't ship NDIS5 anymore so new installs will
+	// use NDIS6.
 	std::string tapDriverPath(_pathToHelpers + WINENV.tapDriverNdis5);
 	const char *tapDriverName = "zttap200";
 	if (::PathFileExistsA(tapDriverPath.c_str()) == FALSE) {
@@ -198,7 +206,7 @@ WindowsEthernetTap::WindowsEthernetTap(
 		if (devconLog != INVALID_HANDLE_VALUE)
 			SetFilePointer(devconLog,0,0,FILE_END);
 
-		// Execute devcon to install an instance of the Microsoft Loopback Adapter
+		// Execute devcon to create a new tap device
 		STARTUPINFOA startupInfo;
 		startupInfo.cb = sizeof(startupInfo);
 		if (devconLog != INVALID_HANDLE_VALUE) {
@@ -262,6 +270,9 @@ WindowsEthernetTap::WindowsEthernetTap(
 				}
 			} else break; // no more keys or error occurred
 		}
+
+		// Cause all existing taps to reset
+		_needReset = true;
 	}
 
 	if (_netCfgInstanceId.length() > 0) {
@@ -306,15 +317,15 @@ WindowsEthernetTap::WindowsEthernetTap(
 	if (ConvertInterfaceGuidToLuid(&_deviceGuid,&_deviceLuid) != NO_ERROR)
 		throw std::runtime_error("unable to convert device interface GUID to LUID");
 
+	// Certain functions can now work (e.g. ips())
+	_initialized = true;
+
 	if (friendlyName)
 		setFriendlyName(friendlyName);
 
 	// Start background thread that actually performs I/O
 	_injectSemaphore = CreateSemaphore(NULL,0,1,NULL);
 	_thread = Thread::start(this);
-
-	// Certain functions can now work (e.g. ips())
-	_initialized = true;
 }
 
 WindowsEthernetTap::~WindowsEthernetTap()
@@ -566,38 +577,25 @@ void WindowsEthernetTap::threadMain()
 	HANDLE wait4[3];
 	char *tapReadBuf = (char *)0;
 
-	// Shouldn't be needed, but Windows does not overcommit. This Windows
-	// tap code is defensive to schizoid paranoia degrees.
+	if (!_enableTapDevice()) {
+		_enabled = false;
+		return; // only happens if devcon is missing or totally fails
+	}
+
+	/* No idea why I did this. I did it a long time ago and there was only a
+	 * a snarky comment. But I'd never do crap like this without a reason, so
+	 * I am leaving it alone with a more descriptive snarky comment. */
 	while (!tapReadBuf) {
 		tapReadBuf = (char *)::malloc(ZT_IF_MTU + 32);
 		if (!tapReadBuf)
 			Sleep(1000);
 	}
 
-	// Tap is in this weird Windows global pseudo file space
 	Utils::snprintf(tapPath,sizeof(tapPath),"\\\\.\\Global\\%s.tap",_netCfgInstanceId.c_str());
-
-	/* More insanity: repetatively try to enable/disable tap device. The first
-	 * time we succeed, close it and do it again. This is to fix a driver init
-	 * bug that seems to be extremely non-deterministic and to only occur after
-	 * headless MSI upgrade. It cannot be reproduced in any other circumstance.
-	 *
-	 * Eventually when ZeroTier has actual money we will have someone create an
-	 * NDIS6 tap driver. Yes, we'll likely be cool and open source it. */
-	bool throwOneAway = true;
 	while (_run) {
-		_disableTapDevice();
-		Sleep(250);
-		if (!_enableTapDevice()) {
-			::free(tapReadBuf);
-			_enabled = false;
-			return; // only happens if devcon is missing or totally fails
-		}
-		Sleep(250);
-
 		_tap = CreateFileA(tapPath,GENERIC_READ|GENERIC_WRITE,0,NULL,OPEN_EXISTING,FILE_ATTRIBUTE_SYSTEM|FILE_FLAG_OVERLAPPED,NULL);
 		if (_tap == INVALID_HANDLE_VALUE) {
-			Sleep(500);
+			fprintf(stderr,"Error opening %s -- retrying.\r\n",tapPath);
 			continue;
 		}
 
@@ -605,8 +603,6 @@ void WindowsEthernetTap::threadMain()
 			uint32_t tmpi = 1;
 			DWORD bytesReturned = 0;
 			DeviceIoControl(_tap,TAP_WIN_IOCTL_SET_MEDIA_STATUS,&tmpi,sizeof(tmpi),&tmpi,sizeof(tmpi),&bytesReturned,NULL);
-			bytesReturned = 0;
-			DeviceIoControl(_tap,TAP_WIN_IOCTL_SET_MEDIA_STATUS,&tmpi,sizeof(tmpi),&tmpi,sizeof(tmpi),&bytesReturned,NULL);
 		}
 
 		{
@@ -688,74 +684,65 @@ void WindowsEthernetTap::threadMain()
 #endif
 		}
 
-		if (throwOneAway) {
-			throwOneAway = false;
-			CloseHandle(_tap);
-			_tap = INVALID_HANDLE_VALUE;
-			Sleep(1000);
-			continue;
-		} else break;
-	}
-
-	memset(&tapOvlRead,0,sizeof(tapOvlRead));
-	tapOvlRead.hEvent = CreateEvent(NULL,TRUE,FALSE,NULL);
-	memset(&tapOvlWrite,0,sizeof(tapOvlWrite));
-	tapOvlWrite.hEvent = CreateEvent(NULL,TRUE,FALSE,NULL);
-
-	wait4[0] = _injectSemaphore;
-	wait4[1] = tapOvlRead.hEvent;
-	wait4[2] = tapOvlWrite.hEvent; // only included if writeInProgress is true
-
-	// Start overlapped read, which is always active
-	ReadFile(_tap,tapReadBuf,sizeof(tapReadBuf),NULL,&tapOvlRead);
-	bool writeInProgress = false;
-
-	for(;;) {
-		if (!_run) break;
-		DWORD r = WaitForMultipleObjectsEx(writeInProgress ? 3 : 2,wait4,FALSE,5000,TRUE);
-		if (!_run) break;
-
-		if ((r == WAIT_TIMEOUT)||(r == WAIT_FAILED))
-			continue;
-
-		if (HasOverlappedIoCompleted(&tapOvlRead)) {
-			DWORD bytesRead = 0;
-			if (GetOverlappedResult(_tap,&tapOvlRead,&bytesRead,FALSE)) {
-				if ((bytesRead > 14)&&(_enabled)) {
-					MAC to(tapReadBuf,6);
-					MAC from(tapReadBuf + 6,6);
-					unsigned int etherType = ((((unsigned int)tapReadBuf[12]) & 0xff) << 8) | (((unsigned int)tapReadBuf[13]) & 0xff);
-					try {
-						// TODO: decode vlans
-						_handler(_arg,_nwid,from,to,etherType,0,tapReadBuf + 14,bytesRead - 14);
-					} catch ( ... ) {} // handlers should not throw
+		memset(&tapOvlRead,0,sizeof(tapOvlRead));
+		tapOvlRead.hEvent = CreateEvent(NULL,TRUE,FALSE,NULL);
+		memset(&tapOvlWrite,0,sizeof(tapOvlWrite));
+		tapOvlWrite.hEvent = CreateEvent(NULL,TRUE,FALSE,NULL);
+
+		wait4[0] = _injectSemaphore;
+		wait4[1] = tapOvlRead.hEvent;
+		wait4[2] = tapOvlWrite.hEvent; // only included if writeInProgress is true
+
+		ReadFile(_tap,tapReadBuf,sizeof(tapReadBuf),NULL,&tapOvlRead);
+		bool writeInProgress = false;
+		while ((_run)&&(!_needReset)) {
+			DWORD r = WaitForMultipleObjectsEx(writeInProgress ? 3 : 2,wait4,FALSE,2500,TRUE);
+			if (!_run) break; // will also break outer while(_run)
+
+			if ((r == WAIT_TIMEOUT)||(r == WAIT_FAILED))
+				continue;
+
+			if (HasOverlappedIoCompleted(&tapOvlRead)) {
+				DWORD bytesRead = 0;
+				if (GetOverlappedResult(_tap,&tapOvlRead,&bytesRead,FALSE)) {
+					if ((bytesRead > 14)&&(_enabled)) {
+						MAC to(tapReadBuf,6);
+						MAC from(tapReadBuf + 6,6);
+						unsigned int etherType = ((((unsigned int)tapReadBuf[12]) & 0xff) << 8) | (((unsigned int)tapReadBuf[13]) & 0xff);
+						try {
+							// TODO: decode vlans
+							_handler(_arg,_nwid,from,to,etherType,0,tapReadBuf + 14,bytesRead - 14);
+						} catch ( ... ) {} // handlers should not throw
+					}
 				}
+				ReadFile(_tap,tapReadBuf,ZT_IF_MTU + 32,NULL,&tapOvlRead);
 			}
-			ReadFile(_tap,tapReadBuf,ZT_IF_MTU + 32,NULL,&tapOvlRead);
-		}
 
-		if (writeInProgress) {
-			if (HasOverlappedIoCompleted(&tapOvlWrite)) {
-				writeInProgress = false;
-				_injectPending_m.lock();
-				_injectPending.pop();
-			} else continue; // still writing, so skip code below and wait
-		} else _injectPending_m.lock();
-
-		if (!_injectPending.empty()) {
-			WriteFile(_tap,_injectPending.front().first.data,_injectPending.front().second,NULL,&tapOvlWrite);
-			writeInProgress = true;
+			if (writeInProgress) {
+				if (HasOverlappedIoCompleted(&tapOvlWrite)) {
+					writeInProgress = false;
+					_injectPending_m.lock();
+					_injectPending.pop();
+				} else continue; // still writing, so skip code below and wait
+			} else _injectPending_m.lock();
+
+			if (!_injectPending.empty()) {
+				WriteFile(_tap,_injectPending.front().first.data,_injectPending.front().second,NULL,&tapOvlWrite);
+				writeInProgress = true;
+			}
+
+			_injectPending_m.unlock();
 		}
 
-		_injectPending_m.unlock();
-	}
+		CancelIo(_tap);
 
-	CancelIo(_tap);
+		CloseHandle(tapOvlRead.hEvent);
+		CloseHandle(tapOvlWrite.hEvent);
+		CloseHandle(_tap);
+		_tap = INVALID_HANDLE_VALUE;
 
-	CloseHandle(tapOvlRead.hEvent);
-	CloseHandle(tapOvlWrite.hEvent);
-	CloseHandle(_tap);
-	_tap = INVALID_HANDLE_VALUE;
+		// We will restart and re-open the tap unless _run == false
+	}
 
 	::free(tapReadBuf);
 }