Fix for crazy Windows threading bug... repeatedly adding and removing a network now doesn't leave networks in limbo.

4 files changed, 63 insertions, 38 deletions

diff --git a/node/Network.cpp b/node/Network.cpp
index b38e9692..b3f065ce 100644
--- a/node/Network.cpp
+++ b/node/Network.cpp
@@ -66,10 +66,13 @@ Network::~Network()
 {
 	Thread::join(_setupThread);
 
-	if (_tap)
-		_r->tapFactory->close(_tap,_destroyOnDelete);
+	{
+		Mutex::Lock _l(_lock);
+		if (_tap)
+			_r->tapFactory->close(_tap,_destroyed);
+	}
 
-	if (_destroyOnDelete) {
+	if (_destroyed) {
 		Utils::rm(std::string(_r->homePath + ZT_PATH_SEPARATOR_S + "networks.d" + ZT_PATH_SEPARATOR_S + idString() + ".conf"));
 		Utils::rm(std::string(_r->homePath + ZT_PATH_SEPARATOR_S + "networks.d" + ZT_PATH_SEPARATOR_S + idString() + ".mcerts"));
 	} else {
@@ -80,12 +83,6 @@ Network::~Network()
 
 SharedPtr<Network> Network::newInstance(const RuntimeEnvironment *renv,NodeConfig *nc,uint64_t id)
 {
-	/* We construct Network via a static method to ensure that it is immediately
-	 * wrapped in a SharedPtr<>. Otherwise if there is traffic on the Ethernet
-	 * tap device, a SharedPtr<> wrap can occur in the Ethernet frame handler
-	 * that then causes the Network instance to be deleted before it is finished
-	 * being constructed. C++ edge cases, how I love thee. */
-
 	SharedPtr<Network> nw(new Network());
 	nw->_id = id;
 	nw->_nc = nc;
@@ -94,7 +91,7 @@ SharedPtr<Network> Network::newInstance(const RuntimeEnvironment *renv,NodeConfi
 	nw->_tap = (EthernetTap *)0;
 	nw->_enabled = true;
 	nw->_lastConfigUpdate = 0;
-	nw->_destroyOnDelete = false;
+	nw->_destroyed = false;
 	nw->_netconfFailure = NETCONF_FAILURE_NONE;
 
 	if (nw->controller() == renv->identity.address()) // TODO: fix Switch to allow packets to self
@@ -143,6 +140,9 @@ bool Network::setConfiguration(const Dictionary &conf,bool saveToDisk)
 {
 	Mutex::Lock _l(_lock);
 
+	if (_destroyed)
+		return false;
+
 	try {
 		SharedPtr<NetworkConfig> newConfig(new NetworkConfig(conf)); // throws if invalid
 		if ((newConfig->networkId() == _id)&&(newConfig->issuedTo() == _r->identity.address())) {
@@ -242,6 +242,10 @@ bool Network::isAllowed(const Address &peer) const
 void Network::clean()
 {
 	Mutex::Lock _l(_lock);
+
+	if (_destroyed)
+		return;
+
 	uint64_t now = Utils::now();
 
 	if ((_config)&&(_config->isPublic())) {
@@ -277,23 +281,17 @@ void Network::clean()
 Network::Status Network::status() const
 {
 	Mutex::Lock _l(_lock);
-	if (_tap) {
-		switch(_netconfFailure) {
-			case NETCONF_FAILURE_ACCESS_DENIED:
-				return NETWORK_ACCESS_DENIED;
-			case NETCONF_FAILURE_NOT_FOUND:
-				return NETWORK_NOT_FOUND;
-			case NETCONF_FAILURE_NONE:
-				if (_lastConfigUpdate > 0)
-					return NETWORK_OK;
-				else return NETWORK_WAITING_FOR_FIRST_AUTOCONF;
-			case NETCONF_FAILURE_INIT_FAILED:
-			default:
-				return NETWORK_INITIALIZATION_FAILED;
-		}
-	} else if (_netconfFailure == NETCONF_FAILURE_INIT_FAILED) {
-		return NETWORK_INITIALIZATION_FAILED;
-	} else return NETWORK_INITIALIZING;
+	switch(_netconfFailure) {
+		case NETCONF_FAILURE_ACCESS_DENIED:
+			return NETWORK_ACCESS_DENIED;
+		case NETCONF_FAILURE_NOT_FOUND:
+			return NETWORK_NOT_FOUND;
+		case NETCONF_FAILURE_NONE:
+			return ((_lastConfigUpdate > 0) ? ((_tap) ? NETWORK_OK : NETWORK_INITIALIZING) : NETWORK_WAITING_FOR_FIRST_AUTOCONF);
+		//case NETCONF_FAILURE_INIT_FAILED:
+		default:
+			return NETWORK_INITIALIZATION_FAILED;
+	}
 }
 
 void Network::_CBhandleTapData(void *arg,const MAC &from,const MAC &to,unsigned int etherType,const Buffer<4096> &data)
@@ -366,7 +364,7 @@ void Network::threadMain()
 	{
 		Mutex::Lock _l(_lock);
 		if (_tap) // the tap creation thread can technically be re-launched, though this isn't done right now
-			_r->tapFactory->close(_tap,_destroyOnDelete);
+			_r->tapFactory->close(_tap,false);
 		_tap = t;
 		if (t) {
 			if (_config)
@@ -409,6 +407,20 @@ void Network::setEnabled(bool enabled)
 		_tap->setEnabled(enabled);
 }
 
+void Network::destroy()
+{
+	Mutex::Lock _l(_lock);
+
+	_enabled = false;
+	_destroyed = true;
+
+	Thread::join(_setupThread);
+
+	if (_tap)
+		_r->tapFactory->close(_tap,true);
+	_tap = (EthernetTap *)0;
+}
+
 void Network::_restoreState()
 {
 	Buffer<ZT_NETWORK_CERT_WRITE_BUF_SIZE> buf;
diff --git a/node/Network.hpp b/node/Network.hpp
index 1d62bdf1..cab41411 100644
--- a/node/Network.hpp
+++ b/node/Network.hpp
@@ -99,11 +99,6 @@ private:
 	 */
 	static SharedPtr<Network> newInstance(const RuntimeEnvironment *renv,NodeConfig *nc,uint64_t id);
 
-	/**
-	 * Causes all persistent disk presence to be erased on delete, and this network won't be reloaded on next startup
-	 */
-	inline void destroyOnDelete() throw() { _destroyOnDelete = true; }
-
 public:
 	/**
 	 * Broadcast multicast group: ff:ff:ff:ff:ff:ff / 0
@@ -419,6 +414,15 @@ public:
 	 */
 	void setEnabled(bool enabled);
 
+	/**
+	 * Destroy this network
+	 *
+	 * This causes the network to disable itself, destroy its tap device, and on
+	 * delete to delete all trace of itself on disk and remove any persistent tap
+	 * device instances. Call this when a network is being removed from the system.
+	 */
+	void destroy();
+
 private:
 	static void _CBhandleTapData(void *arg,const MAC &from,const MAC &to,unsigned int etherType,const Buffer<4096> &data);
 
@@ -453,7 +457,7 @@ private:
 	SharedPtr<NetworkConfig> _config;
 	volatile uint64_t _lastConfigUpdate;
 
-	volatile bool _destroyOnDelete;
+	volatile bool _destroyed;
 
 	volatile enum {
 		NETCONF_FAILURE_NONE,
diff --git a/node/NodeConfig.cpp b/node/NodeConfig.cpp
index 3b5ce2f6..732d41a3 100644
--- a/node/NodeConfig.cpp
+++ b/node/NodeConfig.cpp
@@ -286,7 +286,7 @@ void NodeConfig::_doCommand(IpcConnection *ipcc,const char *commandLine)
 				if (nw == _networks.end()) {
 					ipcc->printf("404 leave %.16llx ERROR: not a member of that network"ZT_EOL_S,(unsigned long long)nwid);
 				} else {
-					nw->second->destroyOnDelete();
+					nw->second->destroy();
 					_networks.erase(nw);
 				}
 			} else {
diff --git a/node/Thread.hpp b/node/Thread.hpp
index a796af33..e02cdde6 100644
--- a/node/Thread.hpp
+++ b/node/Thread.hpp
@@ -31,12 +31,13 @@
 #include <stdexcept>

 

 #include "Constants.hpp"

-#include "AtomicCounter.hpp"

 

 #ifdef __WINDOWS__

 

+#include <WinSock2.h>

 #include <Windows.h>

 #include <string.h>

+#include "Mutex.hpp"

 

 namespace ZeroTier {

 

@@ -56,6 +57,7 @@ public:
 		throw()

 	{

 		_th = NULL;

+		_tid = 0;

 	}

 

 	template<typename C>

@@ -71,8 +73,15 @@ public:
 

 	static inline void join(const Thread &t)

 	{

-		if (t._th != NULL)

-			WaitForSingleObject(t._th,INFINITE);

+		if (t._th != NULL) {

+			for(;;) {

+				DWORD ec = STILL_ACTIVE;

+				GetExitCodeThread(t._th,&ec);

+				if (ec == STILL_ACTIVE)

+					WaitForSingleObject(t._th,1000);

+				else break;

+			}

+		}

 	}

 

 	static inline void sleep(unsigned long ms)