- Fix to stop possible loops in the tcp reuse code (write_wait list

and tcp_wait list). Based on analysis and patch from Prad Seniappan
  and Karthik Umashankar.

doc/Changelog

@@ -1,3 +1,8 @@
+7 October 2022: George
+	- Fix to stop possible loops in the tcp reuse code (write_wait list
+	  and tcp_wait list). Based on analysis and patch from Prad Seniappan
+	  and Karthik Umashankar.
+
 6 October 2022: Wouter
 	- Fix to stop responses with TC flag from resulting in partial
 	  responses. It retries to fetch the data elsewhere, or fails the

services/outside_network.c

@@ -86,10 +86,6 @@ static void serviced_tcp_initiate(struct serviced_query* sq, sldns_buffer* buff)
 static int randomize_and_send_udp(struct pending* pend, sldns_buffer* packet,
 	int timeout);
 
-/** remove waiting tcp from the outnet waiting list */
-static void waiting_list_remove(struct outside_network* outnet,
-	struct waiting_tcp* w);
-
 /** select a DNS ID for a TCP stream */
 static uint16_t tcp_select_id(struct outside_network* outnet,
 	struct reuse_tcp* reuse);
@@ -372,7 +368,8 @@ log_reuse_tcp(enum verbosity_value v, const char* msg, struct reuse_tcp* reuse)
 }
 
 /** pop the first element from the writewait list */
-static struct waiting_tcp* reuse_write_wait_pop(struct reuse_tcp* reuse)
+struct waiting_tcp*
+reuse_write_wait_pop(struct reuse_tcp* reuse)
 {
 	struct waiting_tcp* w = reuse->write_wait_first;
 	if(!w)
@@ -390,8 +387,8 @@ static struct waiting_tcp* reuse_write_wait_pop(struct reuse_tcp* reuse)
 }
 
 /** remove the element from the writewait list */
-static void reuse_write_wait_remove(struct reuse_tcp* reuse,
+void
-	struct waiting_tcp* w)
+reuse_write_wait_remove(struct reuse_tcp* reuse, struct waiting_tcp* w)
 {
 	log_assert(w);
 	log_assert(w->write_wait_queued);
@@ -415,8 +412,8 @@ static void reuse_write_wait_remove(struct reuse_tcp* reuse,
 }
 
 /** push the element after the last on the writewait list */
-static void reuse_write_wait_push_back(struct reuse_tcp* reuse,
+void
-	struct waiting_tcp* w)
+reuse_write_wait_push_back(struct reuse_tcp* reuse, struct waiting_tcp* w)
 {
 	if(!w) return;
 	log_assert(!w->write_wait_queued);
@@ -427,7 +424,9 @@ static void reuse_write_wait_push_back(struct reuse_tcp* reuse,
 		w->write_wait_prev = reuse->write_wait_last;
 	} else {
 		reuse->write_wait_first = w;
+		w->write_wait_prev = NULL;
 	}
+	w->write_wait_next = NULL;
 	reuse->write_wait_last = w;
 	w->write_wait_queued = 1;
 }
@@ -810,20 +809,50 @@ reuse_tcp_lru_snip(struct outside_network* outnet)
 	return reuse;
 }
 
-/** call callback on waiting_tcp, if not NULL */
+/** remove waiting tcp from the outnet waiting list */
-static void
+void
-waiting_tcp_callback(struct waiting_tcp* w, struct comm_point* c, int error,
+outnet_waiting_tcp_list_remove(struct outside_network* outnet, struct waiting_tcp* w)
-	struct comm_reply* reply_info)
 {
-	if(w && w->cb) {
+	struct waiting_tcp* p = outnet->tcp_wait_first, *prev = NULL;
-		fptr_ok(fptr_whitelist_pending_tcp(w->cb));
+	w->on_tcp_waiting_list = 0;
-		(void)(*w->cb)(c, w->cb_arg, error, reply_info);
+	while(p) {
+		if(p == w) {
+			/* remove w */
+			if(prev)
+				prev->next_waiting = w->next_waiting;
+			else	outnet->tcp_wait_first = w->next_waiting;
+			if(outnet->tcp_wait_last == w)
+				outnet->tcp_wait_last = prev;
+			w->next_waiting = NULL;
+			return;
 		}
+		prev = p;
+		p = p->next_waiting;
+	}
+	/* outnet_waiting_tcp_list_remove is currently called only with items
+	 * that are already in the waiting list. */
+	log_assert(0);
+}
+
+/** pop the first waiting tcp from the outnet waiting list */
+struct waiting_tcp*
+outnet_waiting_tcp_list_pop(struct outside_network* outnet)
+{
+	struct waiting_tcp* w = outnet->tcp_wait_first;
+	if(!outnet->tcp_wait_first) return NULL;
+	log_assert(w->on_tcp_waiting_list);
+	outnet->tcp_wait_first = w->next_waiting;
+	if(outnet->tcp_wait_last == w)
+		outnet->tcp_wait_last = NULL;
+	w->on_tcp_waiting_list = 0;
+	w->next_waiting = NULL;
+	return w;
 }
 
 /** add waiting_tcp element to the outnet tcp waiting list */
-static void
+void
-outnet_add_tcp_waiting(struct outside_network* outnet, struct waiting_tcp* w)
+outnet_waiting_tcp_list_add(struct outside_network* outnet,
+	struct waiting_tcp* w, int set_timer)
 {
 	struct timeval tv;
 	log_assert(!w->on_tcp_waiting_list);
@@ -835,16 +864,18 @@ outnet_add_tcp_waiting(struct outside_network* outnet, struct waiting_tcp* w)
 	else	outnet->tcp_wait_first = w;
 	outnet->tcp_wait_last = w;
 	w->on_tcp_waiting_list = 1;
+	if(set_timer) {
 #ifndef S_SPLINT_S
 		tv.tv_sec = w->timeout/1000;
 		tv.tv_usec = (w->timeout%1000)*1000;
 #endif
 		comm_timer_set(w->timer, &tv);
 	}
+}
 
 /** add waiting_tcp element as first to the outnet tcp waiting list */
-static void
+void
-outnet_add_tcp_waiting_first(struct outside_network* outnet,
+outnet_waiting_tcp_list_add_first(struct outside_network* outnet,
 	struct waiting_tcp* w, int reset_timer)
 {
 	struct timeval tv;
@@ -869,6 +900,17 @@ outnet_add_tcp_waiting_first(struct outside_network* outnet,
 		(outnet->tcp_reuse_first && outnet->tcp_reuse_last));
 }
 
+/** call callback on waiting_tcp, if not NULL */
+static void
+waiting_tcp_callback(struct waiting_tcp* w, struct comm_point* c, int error,
+	struct comm_reply* reply_info)
+{
+	if(w && w->cb) {
+		fptr_ok(fptr_whitelist_pending_tcp(w->cb));
+		(void)(*w->cb)(c, w->cb_arg, error, reply_info);
+	}
+}
+
 /** see if buffers can be used to service TCP queries */
 static void
 use_free_buffer(struct outside_network* outnet)
@@ -879,15 +921,10 @@ use_free_buffer(struct outside_network* outnet)
 		struct pending_tcp* pend_tcp = NULL;
 #endif
 		struct reuse_tcp* reuse = NULL;
-		w = outnet->tcp_wait_first;
+		w = outnet_waiting_tcp_list_pop(outnet);
-		log_assert(w->on_tcp_waiting_list);
-		outnet->tcp_wait_first = w->next_waiting;
-		if(outnet->tcp_wait_last == w)
-			outnet->tcp_wait_last = NULL;
 		log_assert(
 			(!outnet->tcp_reuse_first && !outnet->tcp_reuse_last) ||
 			(outnet->tcp_reuse_first && outnet->tcp_reuse_last));
-		w->on_tcp_waiting_list = 0;
 		reuse = reuse_tcp_find(outnet, &w->addr, w->addrlen,
 			w->ssl_upstream);
 		/* re-select an ID when moving to a new TCP buffer */
@@ -934,7 +971,7 @@ use_free_buffer(struct outside_network* outnet)
 #endif
 		} else {
 			/* no reuse and no free buffer, put back at the start */
-			outnet_add_tcp_waiting_first(outnet, w, 0);
+			outnet_waiting_tcp_list_add_first(outnet, w, 0);
 			break;
 		}
 #ifdef USE_DNSTAP
@@ -1008,7 +1045,7 @@ reuse_move_writewait_away(struct outside_network* outnet,
 		 * fail the query */
 		w->error_count ++;
 		reuse_tree_by_id_delete(&pend->reuse, w);
-		outnet_add_tcp_waiting(outnet, w);
+		outnet_waiting_tcp_list_add(outnet, w, 1);
 	}
 	while((w = reuse_write_wait_pop(&pend->reuse)) != NULL) {
 		if(verbosity >= VERB_CLIENT && w->pkt_len > 12+2+2 &&
@@ -1019,7 +1056,7 @@ reuse_move_writewait_away(struct outside_network* outnet,
 			verbose(VERB_CLIENT, "reuse_move_writewait_away item %s", buf);
 		}
 		reuse_tree_by_id_delete(&pend->reuse, w);
-		outnet_add_tcp_waiting(outnet, w);
+		outnet_waiting_tcp_list_add(outnet, w, 1);
 	}
 }
 
@@ -2237,7 +2274,7 @@ outnet_tcptimer(void* arg)
 	verbose(VERB_CLIENT, "outnet_tcptimer");
 	if(w->on_tcp_waiting_list) {
 		/* it is on the waiting list */
-		waiting_list_remove(outnet, w);
+		outnet_waiting_tcp_list_remove(outnet, w);
 		waiting_tcp_callback(w, NULL, NETEVENT_TIMEOUT, NULL);
 		waiting_tcp_delete(w);
 	} else {
@@ -2464,7 +2501,7 @@ pending_tcp_query(struct serviced_query* sq, sldns_buffer* packet,
 #ifdef USE_DNSTAP
 		w->sq = sq;
 #endif
-		outnet_add_tcp_waiting(sq->outnet, w);
+		outnet_waiting_tcp_list_add(sq->outnet, w, 1);
 	}
 	return w;
 }
@@ -2612,30 +2649,6 @@ serviced_create(struct outside_network* outnet, sldns_buffer* buff, int dnssec,
 	return sq;
 }
 
-/** remove waiting tcp from the outnet waiting list */
-static void
-waiting_list_remove(struct outside_network* outnet, struct waiting_tcp* w)
-{
-	struct waiting_tcp* p = outnet->tcp_wait_first, *prev = NULL;
-	w->on_tcp_waiting_list = 0;
-	while(p) {
-		if(p == w) {
-			/* remove w */
-			if(prev)
-				prev->next_waiting = w->next_waiting;
-			else	outnet->tcp_wait_first = w->next_waiting;
-			if(outnet->tcp_wait_last == w)
-				outnet->tcp_wait_last = prev;
-			return;
-		}
-		prev = p;
-		p = p->next_waiting;
-	}
-	/* waiting_list_remove is currently called only with items that are
-	 * already in the waiting list. */
-	log_assert(0);
-}
-
 /** reuse tcp stream, remove serviced query from stream,
  * return true if the stream is kept, false if it is to be closed */
 static int
@@ -2730,7 +2743,7 @@ serviced_delete(struct serviced_query* sq)
 				sq->pending = NULL;
 			} else {
 				verbose(VERB_CLIENT, "serviced_delete: tcpwait");
-				waiting_list_remove(sq->outnet, w);
+				outnet_waiting_tcp_list_remove(sq->outnet, w);
 				if(!w->in_cb_and_decommission)
 					waiting_tcp_delete(w);
 			}

services/outside_network.h

@@ -718,6 +718,30 @@ struct reuse_tcp* reuse_tcp_lru_snip(struct outside_network* outnet);
 /** delete readwait waiting_tcp elements, deletes the elements in the list */
 void reuse_del_readwait(rbtree_type* tree_by_id);
 
+/** remove waiting tcp from the outnet waiting list */
+void outnet_waiting_tcp_list_remove(struct outside_network* outnet,
+	struct waiting_tcp* w);
+
+/** pop the first waiting tcp from the outnet waiting list */
+struct waiting_tcp* outnet_waiting_tcp_list_pop(struct outside_network* outnet);
+
+/** add waiting_tcp element to the outnet tcp waiting list */
+void outnet_waiting_tcp_list_add(struct outside_network* outnet,
+	struct waiting_tcp* w, int set_timer);
+
+/** add waiting_tcp element as first to the outnet tcp waiting list */
+void outnet_waiting_tcp_list_add_first(struct outside_network* outnet,
+	struct waiting_tcp* w, int reset_timer);
+
+/** pop the first element from the writewait list */
+struct waiting_tcp* reuse_write_wait_pop(struct reuse_tcp* reuse);
+
+/** remove the element from the writewait list */
+void reuse_write_wait_remove(struct reuse_tcp* reuse, struct waiting_tcp* w);
+
+/** push the element after the last on the writewait list */
+void reuse_write_wait_push_back(struct reuse_tcp* reuse, struct waiting_tcp* w);
+
 /** get TCP file descriptor for address, returns -1 on failure,
  * tcp_mss is 0 or maxseg size to set for TCP packets. */
 int outnet_get_tcp_fd(struct sockaddr_storage* addr, socklen_t addrlen,

testcode/unittcpreuse.c

@@ -44,6 +44,8 @@
 #include "util/random.h"
 #include "services/outside_network.h"
 
+#define MAX_TCP_WAITING_NODES 5
+
 /** add number of new IDs to the reuse tree, randomly chosen */
 static void tcpid_addmore(struct reuse_tcp* reuse,
 	struct outside_network* outnet, unsigned int addnum)
@@ -228,9 +230,260 @@ static void tcp_reuse_tree_list_test(void)
 	free(outnet.tcp_conns);
 }
 
+static void check_waiting_tcp_list(struct outside_network* outnet,
+	struct waiting_tcp* first, struct waiting_tcp* last, size_t total)
+{
+	size_t i, j;
+	struct waiting_tcp* w = outnet->tcp_wait_first;
+	struct waiting_tcp* n = NULL;
+	if(first) unit_assert(outnet->tcp_wait_first == first);
+	if(last) unit_assert(outnet->tcp_wait_last == last && !last->next_waiting);
+	for(i=0; w; i++) {
+		unit_assert(i<total); /* otherwise we are looping */
+		unit_assert(w->on_tcp_waiting_list);
+		n = w->next_waiting;
+		for(j=0; n; j++) {
+			unit_assert(j<total-i-1); /* otherwise we are looping */
+			unit_assert(n != w);
+			n = n->next_waiting;
+		}
+		w = w->next_waiting;
+	}
+}
+
+/** clear the tcp waiting list */
+static void waiting_tcp_list_clear(struct outside_network* outnet)
+{
+	struct waiting_tcp* w = outnet->tcp_wait_first, *n = NULL;
+	if(!w) return;
+	unit_assert(outnet->tcp_wait_first);
+	unit_assert(outnet->tcp_wait_last);
+	while(w) {
+		n = w->next_waiting;
+		w->on_tcp_waiting_list = 0;
+		w->next_waiting = (struct waiting_tcp*)1; /* In purpose faux value */
+		w = n;
+	}
+	outnet->tcp_wait_first = NULL;
+	outnet->tcp_wait_last = NULL;
+}
+
+/** check removal of the waiting_tcp element on the given position of total
+ *  elements */
+static void check_waiting_tcp_removal(int is_pop,
+	struct outside_network* outnet, struct waiting_tcp* store,
+	size_t position, size_t total)
+{
+	size_t i;
+	struct waiting_tcp* w;
+	waiting_tcp_list_clear(outnet);
+	for(i=0; i<total; i++) {
+		outnet_waiting_tcp_list_add(outnet, &store[i], 0);
+	}
+	check_waiting_tcp_list(outnet, &store[0], &store[total-1], total);
+
+	if(is_pop) {
+		w = outnet_waiting_tcp_list_pop(outnet);
+		unit_assert(w); /* please clang-analyser */
+	} else {
+		w = outnet->tcp_wait_first;
+		for(i=0; i<position; i++) {
+			unit_assert(w); /* please clang-analyser */
+			w = w->next_waiting;
+		}
+		unit_assert(w); /* please clang-analyser */
+		outnet_waiting_tcp_list_remove(outnet, w);
+	}
+	unit_assert(!(w->on_tcp_waiting_list || w->next_waiting));
+
+	if(position == 0 && total == 1) {
+		/* the list should be empty */
+		check_waiting_tcp_list(outnet, NULL, NULL, total-1);
+	} else if(position == 0) {
+		/* first element should be gone */
+		check_waiting_tcp_list(outnet, &store[1], &store[total-1], total-1);
+	} else if(position == total - 1) {
+		/* last element should be gone */
+		check_waiting_tcp_list(outnet, &store[0], &store[total-2], total-1);
+	} else {
+		/* an element should be gone */
+		check_waiting_tcp_list(outnet, &store[0], &store[total-1], total-1);
+	}
+}
+
+static void waiting_tcp_list_test(void)
+{
+	size_t i = 0;
+	struct outside_network outnet;
+	struct waiting_tcp* w, *t = NULL;
+	struct waiting_tcp store[MAX_TCP_WAITING_NODES];
+	memset(&outnet, 0, sizeof(outnet));
+	memset(&store, 0, sizeof(store));
+
+	/* Check add first on empty list */
+	unit_show_func("services/outside_network.c", "outnet_waiting_tcp_list_add_first");
+	t = &store[i];
+	outnet_waiting_tcp_list_add_first(&outnet, t, 0);
+	check_waiting_tcp_list(&outnet, t, t, 1);
+
+	/* Check add */
+	unit_show_func("services/outside_network.c", "outnet_waiting_tcp_list_add");
+	for(i=1; i<MAX_TCP_WAITING_NODES-1; i++) {
+		w = &store[i];
+		outnet_waiting_tcp_list_add(&outnet, w, 0);
+	}
+	check_waiting_tcp_list(&outnet, t, w, MAX_TCP_WAITING_NODES-1);
+
+	/* Check add first on populated list */
+	unit_show_func("services/outside_network.c", "outnet_waiting_tcp_list_add_first");
+	w = &store[i];
+	t = outnet.tcp_wait_last;
+	outnet_waiting_tcp_list_add_first(&outnet, w, 0);
+	check_waiting_tcp_list(&outnet, w, t, MAX_TCP_WAITING_NODES);
+
+	/* Check removal */
+	unit_show_func("services/outside_network.c", "outnet_waiting_tcp_list_remove");
+	check_waiting_tcp_removal(0, &outnet, store, 2, 5);
+	check_waiting_tcp_removal(0, &outnet, store, 1, 3);
+	check_waiting_tcp_removal(0, &outnet, store, 0, 2);
+	check_waiting_tcp_removal(0, &outnet, store, 1, 2);
+	check_waiting_tcp_removal(0, &outnet, store, 0, 1);
+
+	/* Check pop */
+	unit_show_func("services/outside_network.c", "outnet_waiting_tcp_list_pop");
+	check_waiting_tcp_removal(1, &outnet, store, 0, 3);
+	check_waiting_tcp_removal(1, &outnet, store, 0, 2);
+	check_waiting_tcp_removal(1, &outnet, store, 0, 1);
+}
+
+static void check_reuse_write_wait(struct reuse_tcp* reuse,
+	struct waiting_tcp* first, struct waiting_tcp* last, size_t total)
+{
+	size_t i, j;
+	struct waiting_tcp* w = reuse->write_wait_first;
+	struct waiting_tcp* n = NULL;
+	if(first) unit_assert(reuse->write_wait_first == first && !first->write_wait_prev);
+	if(last) unit_assert(reuse->write_wait_last == last && !last->write_wait_next);
+	/* check one way */
+	for(i=0; w; i++) {
+		unit_assert(i<total); /* otherwise we are looping */
+		unit_assert(w->write_wait_queued);
+		n = w->write_wait_next;
+		for(j=0; n; j++) {
+			unit_assert(j<total-i-1); /* otherwise we are looping */
+			unit_assert(n != w);
+			n = n->write_wait_next;
+		}
+		w = w->write_wait_next;
+	}
+	/* check the other way */
+	w = reuse->write_wait_last;
+	for(i=0; w; i++) {
+		unit_assert(i<total); /* otherwise we are looping */
+		unit_assert(w->write_wait_queued);
+		n = w->write_wait_prev;
+		for(j=0; n; j++) {
+			unit_assert(j<total-i-1); /* otherwise we are looping */
+			unit_assert(n != w);
+			n = n->write_wait_prev;
+		}
+		w = w->write_wait_prev;
+	}
+}
+
+/** clear the tcp waiting list */
+static void reuse_write_wait_clear(struct reuse_tcp* reuse)
+{
+	struct waiting_tcp* w = reuse->write_wait_first, *n = NULL;
+	if(!w) return;
+	unit_assert(reuse->write_wait_first);
+	unit_assert(reuse->write_wait_last);
+	while(w) {
+		n = w->write_wait_next;
+		w->write_wait_queued = 0;
+		w->write_wait_next = (struct waiting_tcp*)1;  /* In purpose faux value */
+		w->write_wait_prev = (struct waiting_tcp*)1;  /* In purpose faux value */
+		w = n;
+	}
+	reuse->write_wait_first = NULL;
+	reuse->write_wait_last = NULL;
+}
+
+/** check removal of the reuse_write_wait element on the given position of total
+ *  elements */
+static void check_reuse_write_wait_removal(int is_pop,
+	struct reuse_tcp* reuse, struct waiting_tcp* store,
+	size_t position, size_t total)
+{
+	size_t i;
+	struct waiting_tcp* w;
+	reuse_write_wait_clear(reuse);
+	for(i=0; i<total; i++) {
+		reuse_write_wait_push_back(reuse, &store[i]);
+	}
+	check_reuse_write_wait(reuse, &store[0], &store[total-1], total);
+
+	if(is_pop) {
+		w = reuse_write_wait_pop(reuse);
+	} else {
+		w = reuse->write_wait_first;
+		for(i=0; i<position; i++) w = w->write_wait_next;
+		reuse_write_wait_remove(reuse, w);
+	}
+	unit_assert(!(w->write_wait_queued || w->write_wait_next || w->write_wait_prev));
+
+	if(position == 0 && total == 1) {
+		/* the list should be empty */
+		check_reuse_write_wait(reuse, NULL, NULL, total-1);
+	} else if(position == 0) {
+		/* first element should be gone */
+		check_reuse_write_wait(reuse, &store[1], &store[total-1], total-1);
+	} else if(position == total - 1) {
+		/* last element should be gone */
+		check_reuse_write_wait(reuse, &store[0], &store[total-2], total-1);
+	} else {
+		/* an element should be gone */
+		check_reuse_write_wait(reuse, &store[0], &store[total-1], total-1);
+	}
+}
+
+static void reuse_write_wait_test(void)
+{
+	size_t i;
+	struct reuse_tcp reuse;
+	struct waiting_tcp store[MAX_TCP_WAITING_NODES];
+	struct waiting_tcp* w;
+	memset(&reuse, 0, sizeof(reuse));
+	memset(&store, 0, sizeof(store));
+
+	/* Check adding */
+	unit_show_func("services/outside_network.c", "reuse_write_wait_push_back");
+	for(i=0; i<MAX_TCP_WAITING_NODES; i++) {
+		w = &store[i];
+		reuse_write_wait_push_back(&reuse, w);
+	}
+	check_reuse_write_wait(&reuse, &store[0], w, MAX_TCP_WAITING_NODES);
+
+	/* Check removal */
+	unit_show_func("services/outside_network.c", "reuse_write_wait_remove");
+	check_reuse_write_wait_removal(0, &reuse, store, 2, 5);
+	check_reuse_write_wait_removal(0, &reuse, store, 1, 3);
+	check_reuse_write_wait_removal(0, &reuse, store, 0, 2);
+	check_reuse_write_wait_removal(0, &reuse, store, 1, 2);
+	check_reuse_write_wait_removal(0, &reuse, store, 0, 1);
+
+	/* Check pop */
+	unit_show_func("services/outside_network.c", "reuse_write_wait_pop");
+	check_reuse_write_wait_removal(0, &reuse, store, 0, 3);
+	check_reuse_write_wait_removal(0, &reuse, store, 0, 2);
+	check_reuse_write_wait_removal(0, &reuse, store, 0, 1);
+}
+
 void tcpreuse_test(void)
 {
     unit_show_feature("tcp_reuse");
     tcpid_test();
     tcp_reuse_tree_list_test();
+    waiting_tcp_list_test();
+    reuse_write_wait_test();
 }