diff --git a/dnstap/unbound-dnstap-socket.c b/dnstap/unbound-dnstap-socket.c index a437b6296..f203aa7d7 100644 --- a/dnstap/unbound-dnstap-socket.c +++ b/dnstap/unbound-dnstap-socket.c @@ -200,18 +200,25 @@ static void tap_data_list_try_to_free_tail(struct tap_data_list* list) } /** delete the tap structure */ -static void tap_data_free(struct tap_data* data) +static void tap_data_free(struct tap_data* data, int free_tail) { - ub_event_del(data->ev); - ub_event_free(data->ev); + if(!data) + return; + if(data->ev) { + ub_event_del(data->ev); + ub_event_free(data->ev); + } #ifdef HAVE_SSL SSL_free(data->ssl); #endif - close(data->fd); + sock_close(data->fd); free(data->id); free(data->frame); - data->data_list->d = NULL; - tap_data_list_try_to_free_tail(data->data_list); + if(data->data_list) { + data->data_list->d = NULL; + if(free_tail) + tap_data_list_try_to_free_tail(data->data_list); + } free(data); } @@ -237,7 +244,7 @@ static void tap_data_list_delete(struct tap_data_list* list) while(e) { next = e->next; if(e->d) { - tap_data_free(e->d); + tap_data_free(e->d, 0); e->d = NULL; } free(e); @@ -260,7 +267,7 @@ static void tap_socket_close(struct tap_socket* s) { if(!s) return; if(s->fd == -1) return; - close(s->fd); + sock_close(s->fd); s->fd = -1; } @@ -992,7 +999,7 @@ static int tap_handshake(struct tap_data* data) return 0; } else if(r == 0) { /* closed */ - tap_data_free(data); + tap_data_free(data, 1); return 0; } else if(want == SSL_ERROR_SYSCALL) { /* SYSCALL and errno==0 means closed uncleanly */ @@ -1010,7 +1017,7 @@ static int tap_handshake(struct tap_data* data) if(!silent) log_err("SSL_handshake syscall: %s", strerror(errno)); - tap_data_free(data); + tap_data_free(data, 1); return 0; } else { unsigned long err = ERR_get_error(); @@ -1020,7 +1027,7 @@ static int tap_handshake(struct tap_data* data) verbose(VERB_OPS, "ssl handshake failed " "from %s", data->id); } - tap_data_free(data); + tap_data_free(data, 1); return 0; } } @@ -1028,7 +1035,7 @@ static int tap_handshake(struct tap_data* data) data->ssl_handshake_done = 1; if(!tap_check_peer(data)) { /* closed */ - tap_data_free(data); + tap_data_free(data, 1); return 0; } return 1; @@ -1054,7 +1061,7 @@ void dtio_tap_callback(int ATTR_UNUSED(fd), short ATTR_UNUSED(bits), void* arg) if(verbosity>=4) log_info("s recv %d", (int)ret); if(ret == 0) { /* closed or error */ - tap_data_free(data); + tap_data_free(data, 1); return; } else if(ret == -1) { /* continue later */ @@ -1076,7 +1083,7 @@ void dtio_tap_callback(int ATTR_UNUSED(fd), short ATTR_UNUSED(bits), void* arg) data->frame = calloc(1, data->len); if(!data->frame) { log_err("out of memory"); - tap_data_free(data); + tap_data_free(data, 1); return; } } @@ -1089,7 +1096,7 @@ void dtio_tap_callback(int ATTR_UNUSED(fd), short ATTR_UNUSED(bits), void* arg) if(verbosity>=4) log_info("f recv %d", (int)r); if(r == 0) { /* closed or error */ - tap_data_free(data); + tap_data_free(data, 1); return; } else if(r == -1) { /* continue later */ @@ -1114,13 +1121,13 @@ void dtio_tap_callback(int ATTR_UNUSED(fd), short ATTR_UNUSED(bits), void* arg) data->is_bidirectional = 1; if(verbosity) log_info("bidirectional stream"); if(!reply_with_accept(data)) { - tap_data_free(data); + tap_data_free(data, 1); return; } } else if(data->len >= 4 && sldns_read_uint32(data->frame) == FSTRM_CONTROL_FRAME_STOP && data->is_bidirectional) { if(!reply_with_finish(data)) { - tap_data_free(data); + tap_data_free(data, 1); return; } } @@ -1400,6 +1407,41 @@ static int internal_unittest() /* clean up */ tap_data_list_delete(socket->data_list); free(socket); + + /* Start again. Add two elements */ + socket = calloc(1, sizeof(*socket)); + log_assert(socket); + for(i=0; i<2; i++) { + datas[i] = calloc(1, sizeof(struct tap_data)); + log_assert(datas[i]); + log_assert(tap_data_list_insert(&socket->data_list, datas[i])); + } + /* sanity base check */ + list = socket->data_list; + for(i=0; list; i++) list = list->next; + log_assert(i==2); + + /* Free the last data, tail cannot be erased */ + list = socket->data_list; + while(list->next) list = list->next; + free(list->d); + list->d = NULL; + tap_data_list_try_to_free_tail(list); + list = socket->data_list; + for(i=0; list; i++) list = list->next; + log_assert(i==2); + + /* clean up */ + tap_data_list_delete(socket->data_list); + free(socket); + + if(log_get_lock()) { + lock_basic_destroy((lock_basic_type*)log_get_lock()); + } + checklock_stop(); +#ifdef USE_WINSOCK + WSACleanup(); +#endif return 0; } @@ -1531,6 +1573,9 @@ int main(int argc, char** argv) config_delstrlist(tcp_list.first); config_delstrlist(tls_list.first); + if(log_get_lock()) { + lock_basic_destroy((lock_basic_type*)log_get_lock()); + } checklock_stop(); #ifdef USE_WINSOCK WSACleanup(); diff --git a/doc/Changelog b/doc/Changelog index ec8024cc1..7e1b81eb4 100644 --- a/doc/Changelog +++ b/doc/Changelog @@ -1,3 +1,10 @@ +1 August 2024: Wouter + - Fix dnstap test program, cleans up to have clean memory on exit, + for tap_data_free, does not delete NULL items. Also it does not try + to free the tail, specifically in the free of the list since that + picked up the next item in the list for its loop causing invalid + free. Added internal unit test to unbound-dnstap-socket for that. + 31 July 2024: Wouter - Fix for #1114: Fix that cache fill for forward-host names is performed, so that with nonzero target-fetch-policy it fetches diff --git a/testdata/dnstap.tdir/dnstap.post b/testdata/dnstap.tdir/dnstap.post index 6d5e9d50d..8fefc7e84 100644 --- a/testdata/dnstap.tdir/dnstap.post +++ b/testdata/dnstap.tdir/dnstap.post @@ -12,4 +12,6 @@ kill_pid $FWD_PID kill $UNBOUND_PID kill $UNBOUND_PID >/dev/null 2>&1 cat unbound.log +cat tap.log +cat tap.errlog exit 0 diff --git a/testdata/dnstap.tdir/dnstap.test b/testdata/dnstap.tdir/dnstap.test index 3ec9c77bd..ebb180251 100644 --- a/testdata/dnstap.tdir/dnstap.test +++ b/testdata/dnstap.tdir/dnstap.test @@ -122,8 +122,6 @@ if test $num_responses -gt 2; then fi echo "> cat logfiles" -cat tap.log -cat tap.errlog cat fwd.log echo "> OK" exit 0