From: W.C.A. Wijngaards Date: Tue, 5 Aug 2025 13:46:54 +0000 (+0200) Subject: - Fix #1309: incorrectly reclaimed tcp handler can cause data X-Git-Tag: release-1.24.0rc1~43 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=da6b735ed9f2d5d7259ae5130f26a49bc526c632;p=thirdparty%2Funbound.git - Fix #1309: incorrectly reclaimed tcp handler can cause data corruption and segfault. --- diff --git a/doc/Changelog b/doc/Changelog index 883920b77..e1d3e5b45 100644 --- a/doc/Changelog +++ b/doc/Changelog @@ -1,3 +1,7 @@ +5 August 2025: Wouter + - Fix #1309: incorrectly reclaimed tcp handler can cause data + corruption and segfault. + 1 August 2025: Wouter - Fix testbound test program to accurately output packets from hex. diff --git a/util/netevent.c b/util/netevent.c index 8d6445abf..952efc111 100644 --- a/util/netevent.c +++ b/util/netevent.c @@ -3218,6 +3218,10 @@ comm_point_tcp_accept_callback(int fd, short event, void* arg) } /* accept incoming connection. */ c_hdl = c->tcp_free; + if(!c_hdl->is_in_tcp_free) { + /* Should not happen */ + fatal_exit("inconsistent tcp_free state in accept_callback"); + } /* clear leftover flags from previous use, and then set the * correct event base for the event structure for libevent */ ub_event_free(c_hdl->ev->ev); @@ -3292,10 +3296,16 @@ comm_point_tcp_accept_callback(int fd, short event, void* arg) #endif } + /* Paranoia: Check that the state has not changed from above: */ + if(c_hdl != c->tcp_free || !c_hdl->is_in_tcp_free) { + /* Should not happen */ + fatal_exit("tcp_free state changed within accept_callback!"); + } /* grab the tcp handler buffers */ c->cur_tcp_count++; c->tcp_free = c_hdl->tcp_free; c_hdl->tcp_free = NULL; + c_hdl->is_in_tcp_free = 0; if(!c->tcp_free) { /* stop accepting incoming queries for now. */ comm_point_stop_listening(c); @@ -3316,12 +3326,15 @@ reclaim_tcp_handler(struct comm_point* c) #endif } comm_point_close(c); - if(c->tcp_parent) { - if(c != c->tcp_parent->tcp_free) { - c->tcp_parent->cur_tcp_count--; - c->tcp_free = c->tcp_parent->tcp_free; - c->tcp_parent->tcp_free = c; - } + if(c->tcp_parent && !c->is_in_tcp_free) { + if(c->tcp_free || c->tcp_parent->cur_tcp_count <= 0) { + /* Should not happen */ + fatal_exit("bad tcp_free state in reclaim_tcp"); + } + c->tcp_parent->cur_tcp_count--; + c->tcp_free = c->tcp_parent->tcp_free; + c->tcp_parent->tcp_free = c; + c->is_in_tcp_free = 1; if(!c->tcp_free) { /* re-enable listening on accept socket */ comm_point_start_listening(c->tcp_parent, -1, -1); @@ -4707,12 +4720,15 @@ reclaim_http_handler(struct comm_point* c) #endif } comm_point_close(c); - if(c->tcp_parent) { - if(c != c->tcp_parent->tcp_free) { - c->tcp_parent->cur_tcp_count--; - c->tcp_free = c->tcp_parent->tcp_free; - c->tcp_parent->tcp_free = c; - } + if(c->tcp_parent && !c->is_in_tcp_free) { + if(c->tcp_free || c->tcp_parent->cur_tcp_count <= 0) { + /* Should not happen */ + fatal_exit("bad tcp_free state in reclaim_http"); + } + c->tcp_parent->cur_tcp_count--; + c->tcp_free = c->tcp_parent->tcp_free; + c->tcp_parent->tcp_free = c; + c->is_in_tcp_free = 1; if(!c->tcp_free) { /* re-enable listening on accept socket */ comm_point_start_listening(c->tcp_parent, -1, -1); @@ -5748,6 +5764,7 @@ comm_point_create_udp(struct comm_base *base, int fd, sldns_buffer* buffer, c->cur_tcp_count = 0; c->tcp_handlers = NULL; c->tcp_free = NULL; + c->is_in_tcp_free = 0; c->type = comm_udp; c->tcp_do_close = 0; c->do_not_close = 0; @@ -5812,6 +5829,7 @@ comm_point_create_udp_ancil(struct comm_base *base, int fd, c->cur_tcp_count = 0; c->tcp_handlers = NULL; c->tcp_free = NULL; + c->is_in_tcp_free = 0; c->type = comm_udp; c->tcp_do_close = 0; c->do_not_close = 0; @@ -5879,6 +5897,7 @@ comm_point_create_doq(struct comm_base *base, int fd, sldns_buffer* buffer, c->cur_tcp_count = 0; c->tcp_handlers = NULL; c->tcp_free = NULL; + c->is_in_tcp_free = 0; c->type = comm_doq; c->tcp_do_close = 0; c->do_not_close = 0; @@ -5979,6 +5998,7 @@ comm_point_create_tcp_handler(struct comm_base *base, c->cur_tcp_count = 0; c->tcp_handlers = NULL; c->tcp_free = NULL; + c->is_in_tcp_free = 0; c->type = comm_tcp; c->tcp_do_close = 0; c->do_not_close = 0; @@ -6016,6 +6036,7 @@ comm_point_create_tcp_handler(struct comm_base *base, /* add to parent free list */ c->tcp_free = parent->tcp_free; parent->tcp_free = c; + c->is_in_tcp_free = 1; /* ub_event stuff */ evbits = UB_EV_PERSIST | UB_EV_READ | UB_EV_TIMEOUT; c->ev->ev = ub_event_new(base->eb->base, c->fd, evbits, @@ -6078,6 +6099,7 @@ comm_point_create_http_handler(struct comm_base *base, c->cur_tcp_count = 0; c->tcp_handlers = NULL; c->tcp_free = NULL; + c->is_in_tcp_free = 0; c->type = comm_http; c->tcp_do_close = 1; c->do_not_close = 0; @@ -6136,6 +6158,7 @@ comm_point_create_http_handler(struct comm_base *base, /* add to parent free list */ c->tcp_free = parent->tcp_free; parent->tcp_free = c; + c->is_in_tcp_free = 1; /* ub_event stuff */ evbits = UB_EV_PERSIST | UB_EV_READ | UB_EV_TIMEOUT; c->ev->ev = ub_event_new(base->eb->base, c->fd, evbits, @@ -6197,6 +6220,7 @@ comm_point_create_tcp(struct comm_base *base, int fd, int num, return NULL; } c->tcp_free = NULL; + c->is_in_tcp_free = 0; c->type = comm_tcp_accept; c->tcp_do_close = 0; c->do_not_close = 0; @@ -6291,6 +6315,7 @@ comm_point_create_tcp_out(struct comm_base *base, size_t bufsize, c->cur_tcp_count = 0; c->tcp_handlers = NULL; c->tcp_free = NULL; + c->is_in_tcp_free = 0; c->type = comm_tcp; c->tcp_do_close = 0; c->do_not_close = 0; @@ -6355,6 +6380,7 @@ comm_point_create_http_out(struct comm_base *base, size_t bufsize, c->cur_tcp_count = 0; c->tcp_handlers = NULL; c->tcp_free = NULL; + c->is_in_tcp_free = 0; c->type = comm_http; c->tcp_do_close = 0; c->do_not_close = 0; @@ -6425,6 +6451,7 @@ comm_point_create_local(struct comm_base *base, int fd, size_t bufsize, c->cur_tcp_count = 0; c->tcp_handlers = NULL; c->tcp_free = NULL; + c->is_in_tcp_free = 0; c->type = comm_local; c->tcp_do_close = 0; c->do_not_close = 1; @@ -6488,6 +6515,7 @@ comm_point_create_raw(struct comm_base* base, int fd, int writing, c->cur_tcp_count = 0; c->tcp_handlers = NULL; c->tcp_free = NULL; + c->is_in_tcp_free = 0; c->type = comm_raw; c->tcp_do_close = 0; c->do_not_close = 1; diff --git a/util/netevent.h b/util/netevent.h index 96de0032c..f0f336e43 100644 --- a/util/netevent.h +++ b/util/netevent.h @@ -238,6 +238,8 @@ struct comm_point { /** linked list of free tcp_handlers to use for new queries. For tcp_accept the first entry, for tcp_handlers the next one. */ struct comm_point* tcp_free; + /** Whether this struct is in its parent's tcp_free list */ + int is_in_tcp_free; /* -------- SSL TCP DNS ------- */ /** the SSL object with rw bio (owned) or for commaccept ctx ref */