Added standardization target for code style, fixed LWIP_DEBUG redefinition warning, fixed rare tcp_close() and cb_poll() bugs

2017-09-13 22:34:25 -07:00
parent 0dc390ddcf
commit 3bec79314e
8 changed files with 188 additions and 151 deletions
--- a/include/Debug.hpp
+++ b/include/Debug.hpp
@@ -28,13 +28,13 @@
 #include <sys/syscall.h>
 #include <sys/types.h>

-#define ZT_DEBUG_LEVEL     6 // Set this to adjust what you'd like to see in the debug traces
+#define ZT_DEBUG_LEVEL     5 // Set this to adjust what you'd like to see in the debug traces

 #define ZT_MSG_TEST        1 // For use in selftest
 #define ZT_MSG_ERROR       2 // Errors
 #define ZT_MSG_INFO        3 // Information which is generally useful to any developer
-#define ZT_MSG_EXTRA       4 // If nothing in your world makes sense
-#define ZT_MSG_TRANSFER    5 // RX/TX specific statements
+#define ZT_MSG_TRANSFER    4 // RX/TX specific statements
+#define ZT_MSG_EXTRA       5 // If nothing in your world makes sense
 #define ZT_MSG_FLOW        6 // High-level flow messages

 #define ZT_COLOR           true
--- a/include/lwipopts.h
+++ b/include/lwipopts.h
@@ -73,8 +73,6 @@
 */
 #define LWIP_DBG_HALT          0x08U

-#define LWIP_DEBUG      0
-
 //#define LWIP_DBG_TYPES_ON LWIP_DBG_TRACE | LWIP_DBG_STATE | LWIP_DBG_FRESH

 #define LWIP_DBG_MIN_LEVEL              LWIP_DBG_LEVEL_ALL
--- a/make-linux.mk
+++ b/make-linux.mk
@@ -282,6 +282,11 @@ nativetest:
 ## Misc                                                                     ##
 ##############################################################################

+standardize:
+	vera++ --transform trim_right src/*.cpp
+	vera++ --transform trim_right src/*.hpp
+	vera++ --transform trim_right include/*.cpp
+	
 clean:
 	-rm -rf $(BUILD)/*
 	-find . -type f \( -name '*.a' -o -name '*.o' -o -name '*.so' -o -name '*.o.d' -o -name '*.out' -o -name '*.log' -o -name '*.dSYM' \) -delete
--- a/make-mac.mk
+++ b/make-mac.mk
@@ -294,6 +294,11 @@ nativetest:
 ## Misc                                                                     ##
 ##############################################################################

+standardize:
+	vera++ --transform trim_right src/*.cpp
+	vera++ --transform trim_right src/*.hpp
+	vera++ --transform trim_right include/*.cpp
+
 clean:
 	-rm -rf $(BUILD)/*
 	-find . -type f \( -name '*.a' -o -name '*.o' -o -name '*.so' -o -name '*.o.d' -o -name '*.out' -o -name '*.log' -o -name '*.dSYM' \) -delete
--- a/src/libzt.cpp
+++ b/src/libzt.cpp
@@ -1086,6 +1086,7 @@ int zts_close(ZT_CLOSE_SIG)
 	ZeroTier::VirtualSocket *vs = get_virtual_socket(fd);
 	if(!vs) {
 		DEBUG_ERROR("no vs found for fd=%d", fd);
+		handle_general_failure();
 		errno = EBADF;
 		return -1;
 	}
@@ -2157,6 +2158,9 @@ ZeroTier::VirtualSocket *get_virtual_socket(int fd)
 		if(p) {
 			vs = p->first;
 		}
+		else {
+			DEBUG_ERROR("unable to locate virtual socket");
+		}
 	}
 	ZeroTier::_multiplexer_lock.unlock();
 	return vs;
@@ -2378,7 +2382,7 @@ void *zts_start_service(void *thread_id) {
 void handle_general_failure() {
 #ifdef ZT_EXIT_ON_GENERAL_FAIL
 	DEBUG_ERROR("exiting (ZT_EXIT_ON_GENERAL_FAIL==1)");
-	//exit(-1);
+	exit(-1);
 #endif
 }

--- a/src/lwIP.cpp
+++ b/src/lwIP.cpp
@@ -83,8 +83,10 @@ err_t lwip_eth_tx(struct netif *netif, struct pbuf *p)
 	src_mac.setTo(ethhdr->src.addr, 6);
 	dest_mac.setTo(ethhdr->dest.addr, 6);

-	tap->_handler(tap->_arg,NULL,tap->_nwid,src_mac,dest_mac,
-		ZeroTier::Utils::ntoh((uint16_t)ethhdr->type),0,buf + sizeof(struct eth_hdr),totalLength - sizeof(struct eth_hdr));
+	char *data = buf + sizeof(struct eth_hdr);
+	int len = totalLength - sizeof(struct eth_hdr);
+	int proto = ZeroTier::Utils::ntoh((uint16_t)ethhdr->type);
+	tap->_handler(tap->_arg, NULL, tap->_nwid, src_mac, dest_mac, proto, 0, data, len);

 	if(ZT_DEBUG_LEVEL >= ZT_MSG_TRANSFER) {
 		char flagbuf[32];
@@ -127,7 +129,11 @@ namespace ZeroTier
 				tap->lwipdev.linkoutput = lwip_eth_tx;
 				tap->lwipdev.hwaddr_len = 6;
 				tap->_mac.copyTo(tap->lwipdev.hwaddr, tap->lwipdev.hwaddr_len);
-				tap->lwipdev.flags = NETIF_FLAG_BROADCAST | NETIF_FLAG_ETHARP | NETIF_FLAG_IGMP | NETIF_FLAG_LINK_UP | NETIF_FLAG_UP;
+				tap->lwipdev.flags = NETIF_FLAG_BROADCAST
+					| NETIF_FLAG_ETHARP
+					| NETIF_FLAG_IGMP
+					| NETIF_FLAG_LINK_UP
+					| NETIF_FLAG_UP;
 				netif_set_default(&(tap->lwipdev));
 				netif_set_link_up(&(tap->lwipdev));
 				netif_set_up(&(tap->lwipdev));
@@ -172,7 +178,8 @@ namespace ZeroTier

 				// state and flags
 				tap->lwipdev6.state = tap;
-				tap->lwipdev6.flags = NETIF_FLAG_LINK_UP | NETIF_FLAG_UP;
+				tap->lwipdev6.flags = NETIF_FLAG_LINK_UP
+					| NETIF_FLAG_UP;

 				netif_set_up(&(tap->lwipdev6));

@@ -241,6 +248,11 @@ namespace ZeroTier
 		return 0;
 	}

+	int lwIP::lwip_num_total_pcbs()
+	{
+		return lwip_num_current_raw_pcbs() + lwip_num_current_udp_pcbs() + lwip_num_current_tcp_pcbs();
+	}
+
 	int lwIP::lwip_add_dns_nameserver(struct sockaddr *addr)
 	{
 		return -1;
@@ -253,7 +265,6 @@ namespace ZeroTier

 	void lwIP::lwip_loop(VirtualTap *tap)
 	{
-		// DEBUG_INFO();
 		uint64_t prev_tcp_time = 0, prev_discovery_time = 0;
 		while(tap->_run)
 		{
@@ -292,7 +303,8 @@ namespace ZeroTier
 		}
 	}

-	void lwIP::lwip_eth_rx(VirtualTap *tap, const MAC &from,const MAC &to,unsigned int etherType,const void *data,unsigned int len)
+	void lwIP::lwip_eth_rx(VirtualTap *tap, const MAC &from, const MAC &to, unsigned int etherType,
+		const void *data, unsigned int len)
 	{
 		struct pbuf *p,*q;
 		if (!tap->_enabled){
@@ -353,7 +365,7 @@ namespace ZeroTier
 	int lwIP::lwip_Socket(void **pcb, int socket_family, int socket_type, int protocol)
 	{
 		if(!can_provision_new_socket(socket_type)) {
-			DEBUG_ERROR("unable to create new socket due to limitation of network stack");
+			DEBUG_ERROR("unable to create socket due to limitation of network stack, PCBs=%d", lwip_num_total_pcbs());
 			errno = ENOMEM;
 			return -1;
 		}
@@ -374,11 +386,9 @@ namespace ZeroTier

 	int lwIP::lwip_Connect(VirtualSocket *vs, const struct sockaddr *addr, socklen_t addrlen)
 	{
-		//DEBUG_INFO();
 		ip_addr_t ba;
 		char addrstr[INET6_ADDRSTRLEN];
 		int port = 0, err = 0;
-
 #if defined(LIBZT_IPV4)
 			struct sockaddr_in *in4 = (struct sockaddr_in *)addr;
 			if(addr->sa_family == AF_INET && vs->socket_type == SOCK_STREAM) {
@@ -397,7 +407,7 @@ namespace ZeroTier
 			}
 #endif
 		if(vs->socket_type == SOCK_DGRAM) {
-			// Generates no network traffic
+			// generates no network traffic
 			if((err = udp_connect((struct udp_pcb*)vs->pcb,(ip_addr_t *)&ba,port)) < 0) {
 				DEBUG_ERROR("error while connecting to with UDP");
 			}
@@ -428,7 +438,6 @@ namespace ZeroTier
 	int lwIP::lwip_Bind(VirtualTap *tap, VirtualSocket *vs, const struct sockaddr *addr, socklen_t addrlen)
 	{
 		// TODO: Check case for IP_ADDR_ANY
-		//DEBUG_EXTRA("vs=%p", vs);
 		ip_addr_t ba;
 		char addrstr[INET6_ADDRSTRLEN];
 		memset(addrstr, 0, INET6_ADDRSTRLEN);
@@ -498,7 +507,6 @@ namespace ZeroTier

 	VirtualSocket* lwIP::lwip_Accept(VirtualSocket *vs)
 	{
-		//DEBUG_INFO();
 		if(!vs) {
 			DEBUG_ERROR("invalid virtual socket");
 			handle_general_failure();
@@ -527,25 +535,17 @@ namespace ZeroTier
 			vs->tap->_tcpconns_m.lock();
 			vs->_rx_m.lock();
 		}
-		if(vs->RXbuf->count()) {
-			int max = vs->socket_type == SOCK_STREAM ? ZT_STACK_TCP_SOCKET_RX_SZ : ZT_STACK_TCP_SOCKET_RX_SZ;
-			int wr = std::min((ssize_t)max, (ssize_t)vs->RXbuf->count());
+		if(vs->socket_type == SOCK_STREAM && vs->RXbuf->count()) {
+			handle_general_failure();
+			int wr = std::min((ssize_t)ZT_STACK_TCP_SOCKET_RX_SZ, (ssize_t)vs->RXbuf->count());
 			int n = vs->tap->_phy.streamSend(vs->sock, vs->RXbuf->get_buf(), wr);
-			char str[22];
-			memcpy(str, vs->RXbuf->get_buf(), 22);
+			if(n > 0) {
 				vs->RXbuf->consume(n);
-			
-			if(vs->socket_type == SOCK_DGRAM)
-			{
-				// TODO
-			}
-			if(vs->socket_type == SOCK_STREAM) { // Only acknolwedge receipt of TCP packets
 				tcp_recved((struct tcp_pcb*)vs->pcb, n);
 				DEBUG_TRANS("TCP RX %d bytes", n);
 			}
 		}
 		if(vs->RXbuf->count() == 0) {
-			DEBUG_INFO("wrote everything");
 			vs->tap->_phy.setNotifyWritable(vs->sock, false); // nothing else to send to the app
 		}
 		if(!lwip_invoked) {
@@ -562,7 +562,8 @@ namespace ZeroTier
 			DEBUG_ERROR("no virtual socket");
 			return -1;
 		}
-		DEBUG_EXTRA("fd=%d, vs=%p, len=%d", vs->app_fd, vs, len);
+		DEBUG_EXTRA("fd=%d, vs=%p, pcb=%p, pcb->state=%d, len=%d",
+			vs->app_fd, vs, (struct tcp_pcb*)(vs->pcb), ((struct tcp_pcb*)(vs->pcb))->state, len);
 		if(vs->socket_type == SOCK_DGRAM) {
 			// TODO: Packet re-assembly hasn't yet been tested with lwIP so UDP packets are limited to MTU-sized chunks
 			int udp_trans_len = std::min(len, (ssize_t)ZT_MAX_MTU);
@@ -589,7 +590,6 @@ namespace ZeroTier
 		if(vs->socket_type == SOCK_STREAM) {
 			// How much we are currently allowed to write to the VirtualSocket
 			ssize_t sndbuf = ((struct tcp_pcb*)vs->pcb)->snd_buf;
-			int r;
 			if(!sndbuf) {
 				// PCB send buffer is full, turn off readability notifications for the
 				// corresponding PhySocket until lwip_cb_sent() is called and confirms that there is
@@ -609,7 +609,7 @@ namespace ZeroTier
 				err = -1; // nothing to write
 			}
 			if(!err) {
-				r = std::min((ssize_t)vs->TXbuf->count(), sndbuf);
+				int r = std::min((ssize_t)vs->TXbuf->count(), sndbuf);
 				// Writes data pulled from the client's socket buffer to LWIP. This merely sends the
 				// data to LWIP to be enqueued and eventually sent to the network.
 				if(r > 0) {
@@ -655,30 +655,40 @@ namespace ZeroTier
 			udp_remove((struct udp_pcb*)vs->pcb);
 		}
 		if(vs->socket_type == SOCK_STREAM) {
-			// according to documentation, tcp_pcb is deallocated by the stack's own tcp code. do nothing
-		}
-		// TODO: check if already closed? vs->TCP_pcb->state != CLOSED
 			if(vs->pcb) {
-			if(((struct tcp_pcb*)vs->pcb)->state == SYN_SENT /*|| vs->TCP_pcb->state == CLOSE_WAIT*/) {
-				// DEBUG_EXTRA("ignoring close request. invalid PCB state for this operation. sock=%p", vs->sock);
-				// TODO: errno = ?;
-				return -1;
-			}
 				struct tcp_pcb* tpcb = (struct tcp_pcb*)vs->pcb;
-			if(tcp_close(tpcb) == ERR_OK) {
+				if(tpcb->state == CLOSED) {
+					DEBUG_ERROR("pcb is in CLOSED state");
+					// calling tcp_close() here would be redundant
+					return 0;
+				}
+				if(tpcb->state == CLOSE_WAIT) {
+					DEBUG_ERROR("pcb is in CLOSE_WAIT state");
+					// calling tcp_close() here would be redundant
+				}
+				if(tpcb->state > TIME_WAIT) {
+					DEBUG_ERROR("warning, pcb=%p is in an invalid state=%d", vs->pcb, tpcb->state);
+					handle_general_failure();
+					err = -1;
+				}
 				// unregister callbacks for this PCB
 				tcp_arg(tpcb, NULL);
-				tcp_recv(tpcb, NULL);
-				tcp_err(tpcb,  NULL);
-				tcp_sent(tpcb, NULL);
-				tcp_poll(tpcb, NULL, 1);
+				if(tpcb->state == LISTEN) {
+					tcp_accept(tpcb, NULL);
 				}
 				else {
+					tcp_recv(tpcb, NULL);
+					tcp_sent(tpcb, NULL);
+					tcp_poll(tpcb, NULL, 0);
+					tcp_err(tpcb,  NULL);
+				}
+				if((err = tcp_close(tpcb)) < 0) {
 					DEBUG_ERROR("error while calling tcp_close, fd=%d, vs=%p, pcb=%p", vs->app_fd, vs, vs->pcb);
 					errno = lwip_err_to_errno(err);
 					err = -1;
 				}
 			}
+		}
 		return err;
 	}

@@ -724,16 +734,13 @@ namespace ZeroTier
 		int tot = 0;
 		if(!vs) {
 			DEBUG_ERROR("no virtual socket");
-			return ERR_OK; // FIXME: Determine if this is correct behaviour expected by the stack 
+			handle_general_failure();
+			return ERR_OK;
 		}
 		struct pbuf* q = p;
 		if(p == NULL) {
-			/*
-			if(((struct tcp_pcb*)vs->pcb)->state == CLOSE_WAIT) {
-				// FIXME: Implement?
-			}
-			*/
-			return ERR_ABRT; // close connection
+			DEBUG_INFO("p=0x0 for pcb=%p, vs->pcb=%p, this indicates a closure. No need to call tcp_close()", PCB, vs->pcb);
+			return ERR_ABRT;
 		}
 		vs->tap->_tcpconns_m.lock();
 		vs->_rx_m.lock();
@@ -764,7 +771,7 @@ namespace ZeroTier
 				vs->RXbuf->consume(w);
 				if(w < write_attempt_sz) {
 					DEBUG_TRANS("len=%5d rx_buf_len=%10d [VSRXBF        -->     APPFDS]", w, vs->RXbuf->count());
-					DEBUG_ERROR("intended to write len=%d, only wrote len=%d", write_attempt_sz, w);
+					DEBUG_EXTRA("intended to write len=%d, only wrote len=%d", write_attempt_sz, w);
 				}
 				else {
 					DEBUG_TRANS("len=%5d rx_buf_len=%10d [VSRXBF        -->     APPFDS]", w, vs->RXbuf->count());
@@ -783,7 +790,6 @@ namespace ZeroTier
 	// callback from stack to notify driver of the successful acceptance of a connection
 	err_t lwIP::lwip_cb_accept(void *arg, struct tcp_pcb *newPCB, err_t err)
 	{
-		//DEBUG_INFO();
 		VirtualSocket *vs = (VirtualSocket*)arg;
 		struct sockaddr_storage ss;
 #if defined(LIBZT_IPV4)
@@ -935,13 +941,24 @@ namespace ZeroTier
 	err_t lwIP::lwip_cb_poll(void* arg, struct tcp_pcb *PCB)
 	{
 		VirtualSocket *vs = (VirtualSocket *)arg;
-		DEBUG_EXTRA("fd=%d, vs=%p, PCB=%p", vs->app_fd, vs, PCB);
 		if(!vs) {
 			DEBUG_ERROR("invalid vs");
 			handle_general_failure();
 			return ERR_OK; // TODO: determine appropriate error value, if any
 		}
-		
+		if(vs->socket_type == SOCK_DGRAM) {
+			DEBUG_INFO("fd=%d, vs=%p, pcb=%p", vs->app_fd, vs, PCB, vs->pcb);
+		}
+		if(vs->socket_type == SOCK_STREAM) {
+			DEBUG_INFO("fd=%d, vs=%p, PCB=%p, vs->pcb=%p, vs->pcb->state=%d", vs->app_fd, vs, PCB, (struct tcp_pcb*)(vs->pcb), ((struct tcp_pcb*)(vs->pcb))->state);
+			if(((struct tcp_pcb*)(vs->pcb))->state == CLOSE_WAIT) {
+				DEBUG_EXTRA("pcb->state=CLOSE_WAIT. do nothing");
+				return ERR_OK;
+			}
+			if(((struct tcp_pcb*)(vs->pcb))->state == CLOSED) {
+				DEBUG_EXTRA("pcb->state=CLOSED. do nothing");
+				return ERR_OK;
+			}
 			// --- Check buffers to see if we need to finish reading/writing anything ---

 			// TODO: Make a more generic form of each of these RX/TX blocks that can be shared
@@ -960,7 +977,7 @@ namespace ZeroTier
 					vs->RXbuf->consume(w);
 					if(w < write_attempt_sz) {
 						DEBUG_TRANS("len=%5d rx_buf_len=%10d [VSRXBF        -->     APPFDS]", w, vs->RXbuf->count());
-					DEBUG_ERROR("intended to write len=%d, only wrote len=%d", write_attempt_sz, w);
+						DEBUG_EXTRA("intended to write len=%d, only wrote len=%d", write_attempt_sz, w);
 					}
 					else {
 						DEBUG_TRANS("len=%5d rx_buf_len=%10d [VSRXBF        -->     APPFDS]", w, vs->RXbuf->count());
@@ -968,7 +985,6 @@ namespace ZeroTier
 				}
 			}
 			vs->_rx_m.unlock();
-
 			// No need to lock the TX buffer since lwip_Write() will lock it for us
 			// TX
 			if(vs->TXbuf->count()) {
@@ -976,6 +992,7 @@ namespace ZeroTier
 				// is where we retry
 				lwipstack->lwip_Write(vs, vs->TXbuf->get_buf(), vs->TXbuf->count());
 			}
+		}
 		return ERR_OK;
 	}

@@ -986,8 +1003,12 @@ namespace ZeroTier
 			DEBUG_ERROR("err=%d, invalid virtual socket", err);
 			errno = -1;
 		}
+		if(vs->socket_type == SOCK_STREAM) {
+			DEBUG_ERROR("vs=%p, pcb=%p, pcb->state=%d, fd=%d, err=%d", vs, vs->pcb, ((struct tcp_pcb*)(vs->pcb))->state, vs->app_fd, err);
+		}
+		if(vs->socket_type == SOCK_DGRAM) {
 			DEBUG_ERROR("vs=%p, pcb=%p, fd=%d, err=%d", vs, vs->pcb, vs->app_fd, err);
-		vs->tap->Close(vs);
+		}
 		switch(err)
 		{
 			case ERR_MEM: // -1
--- a/src/lwIP.hpp
+++ b/src/lwIP.hpp
@@ -216,6 +216,11 @@ namespace ZeroTier {
 		 */
 		static int lwip_num_current_raw_pcbs();

+		/*
+		 * Returns the total number of PCBs of any time or state
+ 		 */
+		int lwip_num_total_pcbs();
+
 		/*
 		 * Registers a DNS nameserver with the network stack
 		 */
--- a/test/selftest.cpp
+++ b/test/selftest.cpp
@@ -1150,7 +1150,6 @@ void udp_client_sustained_4(UDP_UNIT_TEST_SIG_4)

 	int num_to_send = 10;
 	for(int i=0; i<num_to_send; i++) {
-		sleep(1);
 		// tx
 		if((w = SENDTO(fd, msg.c_str(), strlen(msg.c_str()), 0, (struct sockaddr *)remote_addr, sizeof(*remote_addr))) < 0) {
 			DEBUG_ERROR("error sending packet, err=%d", errno);