From: George Thessalonikefs Date: Fri, 7 Oct 2022 09:25:36 +0000 (+0200) Subject: - Fix to stop possible loops in the tcp reuse code (write_wait list X-Git-Tag: release-1.17.0rc1~1 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=2569b12b9c8b703944d82a62781a7f42626ac8f8;p=thirdparty%2Funbound.git - 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. --- diff --git a/doc/Changelog b/doc/Changelog index b73b8bdd1..c956c58bc 100644 --- a/doc/Changelog +++ b/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 diff --git a/services/outside_network.c b/services/outside_network.c index 165a50755..a4529ade5 100644 --- a/services/outside_network.c +++ b/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, - struct waiting_tcp* w) +void +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, - struct waiting_tcp* w) +void +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 */ -static void -waiting_tcp_callback(struct waiting_tcp* w, struct comm_point* c, int error, - struct comm_reply* reply_info) +/** remove waiting tcp from the outnet waiting list */ +void +outnet_waiting_tcp_list_remove(struct outside_network* outnet, struct waiting_tcp* w) { - if(w && w->cb) { - fptr_ok(fptr_whitelist_pending_tcp(w->cb)); - (void)(*w->cb)(c, w->cb_arg, error, reply_info); + 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; + 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 -outnet_add_tcp_waiting(struct outside_network* outnet, struct waiting_tcp* w) +void +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; + tv.tv_sec = w->timeout/1000; + tv.tv_usec = (w->timeout%1000)*1000; #endif - comm_timer_set(w->timer, &tv); + comm_timer_set(w->timer, &tv); + } } /** add waiting_tcp element as first to the outnet tcp waiting list */ -static void -outnet_add_tcp_waiting_first(struct outside_network* outnet, +void +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; - 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 = outnet_waiting_tcp_list_pop(outnet); 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); } diff --git a/services/outside_network.h b/services/outside_network.h index c383b8f09..467c81f60 100644 --- a/services/outside_network.h +++ b/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, diff --git a/testcode/unittcpreuse.c b/testcode/unittcpreuse.c index 087c6c1b9..b32262dac 100644 --- a/testcode/unittcpreuse.c +++ b/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(ion_tcp_waiting_list); + n = w->next_waiting; + for(j=0; n; j++) { + unit_assert(jnext_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; itcp_wait_first; + for(i=0; inext_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; iwrite_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(iwrite_wait_queued); + n = w->write_wait_next; + for(j=0; n; j++) { + unit_assert(jwrite_wait_next; + } + w = w->write_wait_next; + } + /* check the other way */ + w = reuse->write_wait_last; + for(i=0; w; i++) { + unit_assert(iwrite_wait_queued); + n = w->write_wait_prev; + for(j=0; n; j++) { + unit_assert(jwrite_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; iwrite_wait_first; + for(i=0; iwrite_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