Fix for GitHub issue #122 and other fixes

6 files changed, 95 insertions, 33 deletions

diff --git a/control/IpcConnection.cpp b/control/IpcConnection.cpp
index 5924f545..45156e95 100644
--- a/control/IpcConnection.cpp
+++ b/control/IpcConnection.cpp
@@ -36,16 +36,20 @@
 #include "IpcConnection.hpp"
 
 #ifndef __WINDOWS__
+#include <unistd.h>
+#include <sys/ioctl.h>
 #include <sys/socket.h>
 #include <sys/un.h>
-#include <unistd.h>
+#include <sys/socket.h>
+#include <sys/select.h>
 #endif
 
 namespace ZeroTier {
 
-IpcConnection::IpcConnection(const char *endpoint,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg) :
+IpcConnection::IpcConnection(const char *endpoint,unsigned int timeout,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg) :
 	_handler(commandHandler),
 	_arg(arg),
+	_timeout(timeout),
 #ifdef __WINDOWS__
 	_sock(INVALID_HANDLE_VALUE),
 	_incoming(false),
@@ -81,12 +85,13 @@ IpcConnection::IpcConnection(const char *endpoint,void (*commandHandler)(void *,
 }
 
 #ifdef __WINDOWS__
-IpcConnection::IpcConnection(HANDLE s,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg) :
+IpcConnection::IpcConnection(HANDLE s,unsigned int timeout,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg) :
 #else
-IpcConnection::IpcConnection(int s,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg) :
+IpcConnection::IpcConnection(int s,unsigned int timeout,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg) :
 #endif
 	_handler(commandHandler),
 	_arg(arg),
+	_timeout(timeout),
 	_sock(s),
 #ifdef __WINDOWS__
 	_incoming(true),
@@ -104,18 +109,23 @@ IpcConnection::~IpcConnection()
 	_writeLock.unlock();
 
 #ifdef __WINDOWS__
+
 	while (_running) {
-		Thread::cancelIO(_thread);
+		Thread::cancelIO(_thread); // cause Windows to break from blocking read and detect shutdown
 		Sleep(100);
 	}
-#else
+
+#else // !__WINDOWS__
+
 	int s = _sock;
 	_sock = 0;
 	if (s > 0) {
 		::shutdown(s,SHUT_RDWR);
 		::close(s);
 	}
-#endif
+	Thread::join(_thread);
+
+#endif // __WINDOWS__ / !__WINDOWS__
 }
 
 void IpcConnection::printf(const char *format,...)
@@ -134,7 +144,7 @@ void IpcConnection::printf(const char *format,...)
 
 #ifdef __WINDOWS__
 	_writeBuf.append(tmp,n);
-	Thread::cancelIO(_thread);
+	Thread::cancelIO(_thread); // cause Windows to break from blocking read and service write buffer
 #else
 	if (_sock > 0)
 		::write(_sock,tmp,n);
@@ -144,20 +154,39 @@ void IpcConnection::printf(const char *format,...)
 void IpcConnection::threadMain()
 	throw()
 {
-	char tmp[65536];
-	char linebuf[65536];
+	char tmp[16384];
+	char linebuf[16384];
 	unsigned int lineptr = 0;
 	char c;
 
 #ifdef __WINDOWS__
+
 	DWORD n,i;
 	std::string wbuf;
-#else
+
+#else // !__WINDOWS__
+
 	int s,n,i;
-#endif
+	fd_set readfds,writefds,errorfds;
+	struct timeval tout;
+
+#ifdef SO_NOSIGPIPE
+	if (_sock > 0) {
+		i = 1;
+		::setsockopt(_sock,SOL_SOCKET,SO_NOSIGPIPE,(char *)&i,sizeof(i));
+	}
+#endif // SO_NOSIGPIPE
+
+#endif // __WINDOWS__ / !__WINDOWS__
 
 	while (_run) {
+
 #ifdef __WINDOWS__
+
+		/* Note that we do not use fucking timeouts in Windows, since it does seem
+		 * to properly detect named pipe endpoint close. But we do use a write buffer
+		 * because Windows won't let you divorce reading and writing threads without
+		 * all that OVERLAPPED cruft. */
 		{
 			Mutex::Lock _l(_writeLock);
 			if (!_run)
@@ -187,16 +216,42 @@ void IpcConnection::threadMain()
 		}
 		if (!_run)
 			break;
-#else
+
+#else // !__WINDOWS__
+
+		/* So today I learned that there is no reliable way to detect a half-closed
+		 * Unix domain socket. So to make sure we don't leave orphaned sockets around
+		 * we just use fucking timeouts. If a socket fucking times out, we break from
+		 * the I/O loop and terminate the thread. But this IpcConnection code is ugly
+		 * so maybe the OS is simply offended by it and refuses to reveal its mysteries
+		 * to me. Oh well... this IPC code will probably get canned when we go to
+		 * local HTTP RESTful interfaces or soemthing like that. */
 		if ((s = _sock) <= 0)
 			break;
-		n = (int)::read(s,tmp,sizeof(tmp));
-		if ((n <= 0)||(_sock <= 0))
-			break;
-#endif
+		FD_ZERO(&readfds);
+		FD_ZERO(&writefds);
+		FD_ZERO(&errorfds);
+		FD_SET(s,&readfds);
+		FD_SET(s,&errorfds);
+		tout.tv_sec = _timeout; // use a fucking timeout
+		tout.tv_usec = 0;
+		if (select(s+1,&readfds,&writefds,&errorfds,&tout) <= 0) {
+			break; // socket has fucking timed out
+		} else {
+			if (FD_ISSET(s,&errorfds))
+				break; // socket has an exception... sometimes works
+			else {
+				n = (int)::read(s,tmp,sizeof(tmp));
+				if ((n <= 0)||(_sock <= 0))
+					break; // read returned error... sometimes works
+			}
+		}
+
+#endif // __WINDOWS__ / !__WINDOWS__
+
 		for(i=0;i<n;++i) {
 			c = (linebuf[lineptr] = tmp[i]);
-			if ((c == '\r')||(c == '\n')||(lineptr == (sizeof(linebuf) - 1))) {
+			if ((c == '\r')||(c == '\n')||(c == (char)0)||(lineptr == (sizeof(linebuf) - 1))) {
 				if (lineptr) {
 					linebuf[lineptr] = (char)0;
 					_handler(_arg,this,IPC_EVENT_COMMAND,linebuf);
@@ -211,11 +266,13 @@ void IpcConnection::threadMain()
 	_writeLock.unlock();
 
 #ifdef __WINDOWS__
+
 	if (_incoming)
 		DisconnectNamedPipe(_sock);
 	CloseHandle(_sock);
 	_running = false;
-#endif
+
+#endif // __WINDOWS__
 
 	if (r)
 		_handler(_arg,this,IPC_EVENT_CONNECTION_CLOSED,(const char *)0);
diff --git a/control/IpcConnection.hpp b/control/IpcConnection.hpp
index 20c5dcf0..5bbc683e 100644
--- a/control/IpcConnection.hpp
+++ b/control/IpcConnection.hpp
@@ -61,11 +61,12 @@ public:
 	 * Connect to an IPC endpoint
 	 *
 	 * @param endpoint Endpoint path
+	 * @param timeout Inactivity timeout in seconds
 	 * @param commandHandler Command handler function
 	 * @param arg First argument to command handler
 	 * @throws std::runtime_error Unable to connect
 	 */
-	IpcConnection(const char *endpoint,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg);
+	IpcConnection(const char *endpoint,unsigned int timeout,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg);
 	~IpcConnection();
 
 	/**
@@ -80,13 +81,14 @@ public:
 private:
 	// Used by IpcListener to construct incoming connections
 #ifdef __WINDOWS__
-	IpcConnection(HANDLE s,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg);
+	IpcConnection(HANDLE s,unsigned int timeout,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg);
 #else
-	IpcConnection(int s,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg);
+	IpcConnection(int s,unsigned int timeout,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg);
 #endif
 
 	void (*_handler)(void *,IpcConnection *,IpcConnection::EventType,const char *);
 	void *_arg;
+	unsigned int _timeout;
 #ifdef __WINDOWS__
 	HANDLE _sock;
 	std::string _writeBuf;
diff --git a/control/IpcListener.cpp b/control/IpcListener.cpp
index 681328f8..9ad8ad06 100644
--- a/control/IpcListener.cpp
+++ b/control/IpcListener.cpp
@@ -42,10 +42,11 @@
 
 namespace ZeroTier {
 
-IpcListener::IpcListener(const char *ep,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg) :
+IpcListener::IpcListener(const char *ep,unsigned int timeout,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg) :
 	_endpoint(ep),
 	_handler(commandHandler),
 	_arg(arg),
+	_timeout(timeout),
 #ifdef __WINDOWS__
 	_run(true),
 	_running(true)
@@ -130,7 +131,7 @@ void IpcListener::threadMain()
 					break;
 				}
 				try {
-					_handler(_arg,new IpcConnection(s,_handler,_arg),IpcConnection::IPC_EVENT_NEW_CONNECTION,(const char *)0);
+					_handler(_arg,new IpcConnection(s,_timeout,_handler,_arg),IpcConnection::IPC_EVENT_NEW_CONNECTION,(const char *)0);
 				} catch ( ... ) {} // handlers should not throw
 			} else {
 				CloseHandle(s);
@@ -155,7 +156,7 @@ void IpcListener::threadMain()
 			break;
 		}
 		try {
-			_handler(_arg,new IpcConnection(s,_handler,_arg),IpcConnection::IPC_EVENT_NEW_CONNECTION,(const char *)0);
+			_handler(_arg,new IpcConnection(s,_timeout,_handler,_arg),IpcConnection::IPC_EVENT_NEW_CONNECTION,(const char *)0);
 		} catch ( ... ) {} // handlers should not throw
 	}
 #endif
diff --git a/control/IpcListener.hpp b/control/IpcListener.hpp
index e7346766..b417d6c2 100644
--- a/control/IpcListener.hpp
+++ b/control/IpcListener.hpp
@@ -50,7 +50,7 @@ public:
 	 * The supplied handler is passed on to incoming instances of IpcConnection. When
 	 * a connection is first opened, it is called with IPC_EVENT_NEW_CONNECTION. The
 	 * receiver must take ownership of the connection object. When a connection is
-	 * closed, IPC_EVENT_CONNECTION_CLOSING is generated. At this point (or after) the
+	 * closed, IPC_EVENT_CONNECTION_CLOSED is generated. At this point (or after) the
 	 * receiver must delete the object. IPC_EVENT_COMMAND is generated when lines of
 	 * text are read, and in this cases the last argument is not NULL. No closed event
 	 * is generated in the event of manual delete if the connection is still open.
@@ -60,11 +60,12 @@ public:
 	 * use cases are simple enough that it's not too bad.
 	 *
 	 * @param IPC endpoint name (OS-specific)
+	 * @param timeout Endpoint inactivity timeout in seconds
 	 * @param commandHandler Function to call for each command
 	 * @param arg First argument to pass to handler
 	 * @throws std::runtime_error Unable to bind to endpoint
 	 */
-	IpcListener(const char *ep,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg);
+	IpcListener(const char *ep,unsigned int timeout,void (*commandHandler)(void *,IpcConnection *,IpcConnection::EventType,const char *),void *arg);
 
 	~IpcListener();
 
@@ -75,6 +76,7 @@ private:
 	std::string _endpoint;
 	void (*_handler)(void *,IpcConnection *,IpcConnection::EventType,const char *);
 	void *_arg;
+	unsigned int _timeout;
 #ifdef __WINDOWS__
 	volatile bool _run;
 	volatile bool _running;
diff --git a/control/NodeControlClient.cpp b/control/NodeControlClient.cpp
index df1a62b6..11fdc9c8 100644
--- a/control/NodeControlClient.cpp
+++ b/control/NodeControlClient.cpp
@@ -59,7 +59,7 @@ NodeControlClient::NodeControlClient(const char *ep,const char *authToken,void (
 	impl->resultHandler = resultHandler;
 	impl->arg = arg;
 	try {
-		impl->ipcc = new IpcConnection(ep,&_CBipcResultHandler,_impl);
+		impl->ipcc = new IpcConnection(ep,ZT_IPC_TIMEOUT,&_CBipcResultHandler,_impl);
 		impl->ipcc->printf("auth %s"ZT_EOL_S,authToken);
 	} catch ( ... ) {
 		impl->ipcc = (IpcConnection *)0;
diff --git a/control/NodeControlService.cpp b/control/NodeControlService.cpp
index 5d4d08d6..285eae58 100644
--- a/control/NodeControlService.cpp
+++ b/control/NodeControlService.cpp
@@ -70,7 +70,7 @@ void NodeControlService::threadMain()
 					break;
 			} else if ((_node->initialized())&&(_node->address())) {
 				Utils::snprintf(tmp,sizeof(tmp),"%s%.10llx",ZT_IPC_ENDPOINT_BASE,(unsigned long long)_node->address());
-				_listener = new IpcListener(tmp,&_CBcommandHandler,this);
+				_listener = new IpcListener(tmp,ZT_IPC_TIMEOUT,&_CBcommandHandler,this);
 				break;
 			}
 			Thread::sleep(100); // wait for Node to start
@@ -83,18 +83,18 @@ void NodeControlService::threadMain()
 
 void NodeControlService::_CBcommandHandler(void *arg,IpcConnection *ipcc,IpcConnection::EventType event,const char *commandLine)
 {
-	if (!((NodeControlService *)arg)->_running)
-		return;
-	if ((!commandLine)||(!commandLine[0]))
-		return;
 	switch(event) {
 		case IpcConnection::IPC_EVENT_COMMAND: {
+			if ((!((NodeControlService *)arg)->_running)||(!commandLine)||(!commandLine[0]))
+				return;
 			((NodeControlService *)arg)->_doCommand(ipcc,commandLine);
 		}	break;
+
 		case IpcConnection::IPC_EVENT_NEW_CONNECTION: {
 			Mutex::Lock _l(((NodeControlService *)arg)->_connections_m);
 			((NodeControlService *)arg)->_connections[ipcc] = false; // not yet authenticated
 		}	break;
+
 		case IpcConnection::IPC_EVENT_CONNECTION_CLOSED: {
 			Mutex::Lock _l(((NodeControlService *)arg)->_connections_m);
 			((NodeControlService *)arg)->_connections.erase(ipcc);