Probable fix for GitHub issue #63 - do not unite() if either path is TCP, since doing so can result in asymmetric failed NAT-t over UDP if one side has a firewall that permits outgoing UDP but not incoming.

6 files changed, 39 insertions, 19 deletions

diff --git a/main.cpp b/main.cpp
index 5011b562..6e9bf5db 100644
--- a/main.cpp
+++ b/main.cpp
@@ -462,6 +462,17 @@ int main(int argc,char **argv)
 	signal(SIGINT,&sighandlerQuit);
 	signal(SIGTERM,&sighandlerQuit);
 	signal(SIGQUIT,&sighandlerQuit);
+
+	/* Ensure that there are no inherited file descriptors open from a previous
+	 * incarnation. This is a hack to ensure that GitHub issue #61 or variants
+	 * of it do not return, and should not do anything otherwise bad. */
+	{
+		int mfd = STDIN_FILENO;
+		if (STDOUT_FILENO > mfd) mfd = STDOUT_FILENO;
+		if (STDERR_FILENO > mfd) mfd = STDERR_FILENO;
+		for(int f=mfd+1;f<1024;++f)
+			::close(f);
+	}
 #endif
 
 #ifdef __WINDOWS__
diff --git a/node/PacketDecoder.cpp b/node/PacketDecoder.cpp
index c7d3ffda..36f7ead7 100644
--- a/node/PacketDecoder.cpp
+++ b/node/PacketDecoder.cpp
@@ -81,8 +81,9 @@ bool PacketDecoder::tryDecode(const RuntimeEnvironment *_r)
 		//TRACE("<< %s from %s(%s)",Packet::verbString(verb()),source().toString().c_str(),_remoteAddress.toString().c_str());
 
 		switch(verb()) {
-			case Packet::VERB_NOP:
-				peer->receive(_r,_fromSock,_remoteAddress,hops(),packetId(),Packet::VERB_NOP,0,Packet::VERB_NOP,Utils::now());
+			//case Packet::VERB_NOP:
+			default: // ignore unknown verbs, but if they pass auth check they are still valid
+				peer->receive(_r,_fromSock,_remoteAddress,hops(),packetId(),verb(),0,Packet::VERB_NOP,Utils::now());
 				return true;
 			case Packet::VERB_HELLO:
 				return _doHELLO(_r); // legal, but why? :)
@@ -108,8 +109,6 @@ bool PacketDecoder::tryDecode(const RuntimeEnvironment *_r)
 				return _doNETWORK_CONFIG_REQUEST(_r,peer);
 			case Packet::VERB_NETWORK_CONFIG_REFRESH:
 				return _doNETWORK_CONFIG_REFRESH(_r,peer);
-			default: // ignore unknown verbs
-				return true;
 		}
 	} else {
 		_step = DECODE_WAITING_FOR_SENDER_LOOKUP; // should already be this...
diff --git a/node/Peer.cpp b/node/Peer.cpp
index 86a0da0f..5c87275f 100644
--- a/node/Peer.cpp
+++ b/node/Peer.cpp
@@ -117,7 +117,7 @@ void Peer::receive(
 		_lastMulticastFrame = now;
 }
 
-bool Peer::send(const RuntimeEnvironment *_r,const void *data,unsigned int len,uint64_t now)
+Path::Type Peer::send(const RuntimeEnvironment *_r,const void *data,unsigned int len,uint64_t now)
 {
 	Mutex::Lock _l(_lock);
 
@@ -164,9 +164,9 @@ bool Peer::send(const RuntimeEnvironment *_r,const void *data,unsigned int len,u
 
 	if ((bestPath)&&(_r->sm->send(bestPath->address(),bestPath->tcp(),bestPath->type() == Path::PATH_TYPE_TCP_OUT,data,len))) {
 		bestPath->sent(now);
-		return true;
+		return bestPath->type();
 	}
-	return false;
+	return Path::PATH_TYPE_NULL;
 }
 
 bool Peer::sendFirewallOpener(const RuntimeEnvironment *_r,uint64_t now)
diff --git a/node/Peer.hpp b/node/Peer.hpp
index 3a5316d4..39074ebc 100644
--- a/node/Peer.hpp
+++ b/node/Peer.hpp
@@ -136,9 +136,9 @@ public:
 	 * @param data Data to send
 	 * @param len Length of packet
 	 * @param now Current time
-	 * @return True if packet appears to have been sent, false if no path or other error
+	 * @return Type of path used or Path::PATH_TYPE_NULL on failure
 	 */
-	bool send(const RuntimeEnvironment *_r,const void *data,unsigned int len,uint64_t now);
+	Path::Type send(const RuntimeEnvironment *_r,const void *data,unsigned int len,uint64_t now);
 
 	/**
 	 * Send firewall opener to all UDP paths
diff --git a/node/Socket.hpp b/node/Socket.hpp
index 3cfedb1a..e91ce4de 100644
--- a/node/Socket.hpp
+++ b/node/Socket.hpp
@@ -81,6 +81,15 @@ public:
 	}
 
 	/**
+	 * @return True if this is a UDP socket
+	 */
+	inline bool udp() const
+		throw()
+	{
+		return ((_type == ZT_SOCKET_TYPE_UDP_V4)||(_type == ZT_SOCKET_TYPE_UDP_V6));
+	}
+
+	/**
 	 * @return True if this is a TCP socket
 	 */
 	inline bool tcp() const
diff --git a/node/Switch.cpp b/node/Switch.cpp
index e64a3383..1c794176 100644
--- a/node/Switch.cpp
+++ b/node/Switch.cpp
@@ -560,7 +560,7 @@ void Switch::_handleRemotePacketFragment(const SharedPtr<Socket> &fromSock,const
 			fragment.incrementHops();
 
 			SharedPtr<Peer> relayTo = _r->topology->getPeer(destination);
-			if ((!relayTo)||(!relayTo->send(_r,fragment.data(),fragment.size(),Utils::now()))) {
+			if ((!relayTo)||(relayTo->send(_r,fragment.data(),fragment.size(),Utils::now()) == Path::PATH_TYPE_NULL)) {
 				relayTo = _r->topology->getBestSupernode();
 				if (relayTo)
 					relayTo->send(_r,fragment.data(),fragment.size(),Utils::now());
@@ -633,10 +633,13 @@ void Switch::_handleRemotePacketHead(const SharedPtr<Socket> &fromSock,const Ine
 			packet->incrementHops();
 
 			SharedPtr<Peer> relayTo = _r->topology->getPeer(destination);
-			if ((relayTo)&&(relayTo->send(_r,packet->data(),packet->size(),Utils::now()))) {
-				// If we've relayed, this periodically tries to get them to
-				// talk directly to save our bandwidth.
-				unite(source,destination,false);
+			Path::Type relayedVia;
+			if ((relayTo)&&((relayedVia = relayTo->send(_r,packet->data(),packet->size(),Utils::now())) != Path::PATH_TYPE_NULL)) {
+				/* If both paths are UDP, attempt to invoke UDP NAT-t between peers
+				 * by sending VERB_RENDEZVOUS. Do not do this for TCP due to GitHub
+				 * issue #63. */
+				if ((fromSock->udp())&&(relayedVia == Path::PATH_TYPE_UDP))
+					unite(source,destination,false);
 			} else {
 				// If we've received a packet not for us and we don't have
 				// a direct path to its recipient, pass it to (another)
@@ -702,7 +705,7 @@ Address Switch::_sendWhoisRequest(const Address &addr,const Address *peersAlread
 		addr.appendTo(outp);
 		outp.armor(supernode->key(),true);
 		uint64_t now = Utils::now();
-		if (supernode->send(_r,outp.data(),outp.size(),now))
+		if (supernode->send(_r,outp.data(),outp.size(),now) != Path::PATH_TYPE_NULL)
 			return supernode->address();
 	}
 	return Address();
@@ -731,7 +734,7 @@ bool Switch::_trySend(const Packet &packet,bool encrypt)
 
 		tmp.armor(peer->key(),encrypt);
 
-		if (via->send(_r,tmp.data(),chunkSize,now)) {
+		if (via->send(_r,tmp.data(),chunkSize,now) != Path::PATH_TYPE_NULL) {
 			if (chunkSize < tmp.size()) {
 				// Too big for one bite, fragment the rest
 				unsigned int fragStart = chunkSize;
@@ -744,9 +747,7 @@ bool Switch::_trySend(const Packet &packet,bool encrypt)
 				for(unsigned int f=0;f<fragsRemaining;++f) {
 					chunkSize = std::min(remaining,(unsigned int)(ZT_UDP_DEFAULT_PAYLOAD_MTU - ZT_PROTO_MIN_FRAGMENT_LENGTH));
 					Packet::Fragment frag(tmp,fragStart,chunkSize,f + 1,totalFragments);
-					if (!via->send(_r,frag.data(),frag.size(),now)) {
-						TRACE("WARNING: packet send to %s failed on later fragment #%u (check IP layer buffer sizes?)",via->address().toString().c_str(),f + 1);
-					}
+					via->send(_r,frag.data(),frag.size(),now);
 					fragStart += chunkSize;
 					remaining -= chunkSize;
 				}