From: Yorgos Thessalonikefs Date: Fri, 31 May 2024 10:11:17 +0000 (+0200) Subject: - Fix memory leak on exit for unbound-dnstap-socket; creates false negatives X-Git-Tag: release-1.21.0rc1~62^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ac609fcbfc20b2396a6abcd4ca0c979eac544aa2;p=thirdparty%2Funbound.git - Fix memory leak on exit for unbound-dnstap-socket; creates false negatives during testing. --- diff --git a/dnstap/unbound-dnstap-socket.c b/dnstap/unbound-dnstap-socket.c index 04fda74b8..12dac40ee 100644 --- a/dnstap/unbound-dnstap-socket.c +++ b/dnstap/unbound-dnstap-socket.c @@ -75,17 +75,18 @@ static void usage(char* argv[]) { printf("usage: %s [options]\n", argv[0]); - printf(" Listen to dnstap messages\n"); + printf(" Listen to dnstap messages\n"); printf("stdout has dnstap log, stderr has verbose server log\n"); - printf("-u listen to unix socket with this file name\n"); - printf("-s listen for TCP on the IP and port\n"); - printf("-t listen for TLS on IP and port\n"); - printf("-x server key file for TLS service\n"); - printf("-y server cert file for TLS service\n"); - printf("-z cert file to verify client connections\n"); - printf("-l long format for DNS printout\n"); - printf("-v more verbose log output\n"); - printf("-h this help text\n"); + printf("-u listen to unix socket with this file name\n"); + printf("-s listen for TCP on the IP and port\n"); + printf("-t listen for TLS on IP and port\n"); + printf("-x server key file for TLS service\n"); + printf("-y server cert file for TLS service\n"); + printf("-z cert file to verify client connections\n"); + printf("-l long format for DNS printout\n"); + printf("-v more verbose log output\n"); + printf("-c internal unit test and exit\n"); + printf("-h this help text\n"); exit(1); } @@ -102,6 +103,14 @@ struct main_tap_data { struct tap_socket_list* acceptlist; }; +/* list of data */ +struct tap_data_list { + /** next in list */ + struct tap_data_list* next; + /** the data */ + struct tap_data* d; +}; + /** tap callback variables */ struct tap_data { /** the fd */ @@ -128,6 +137,10 @@ struct tap_data { uint8_t* frame; /** length of this frame */ size_t len; + /** back pointer to the tap_data_list entry; + * used to NULL the forward pointer to this data + * when this data is freed. */ + struct tap_data_list* data_list; }; /** list of sockets */ @@ -156,8 +169,82 @@ struct tap_socket { char* ip; /** for a TLS socket, the tls context */ SSL_CTX* sslctx; + /** dumb way to deal with memory leaks: + * tap_data was only freed on errors and not during exit leading to + * false positives when testing for memory leaks. */ + struct tap_data_list* data_list; }; +/** try to delete tail entries from the list if all of them have no data */ +static void tap_data_list_try_to_free_tail(struct tap_data_list* list) +{ + struct tap_data_list* current = list; + log_assert(!list->d); + if(!list->next) /* we are the last, we can't remove ourselves */ + return; + list = list->next; + while(list) { + if(list->d) /* a tail entry still has data; return */ + return; + list = list->next; + } + /* keep the next */ + list = current->next; + /* the tail will be removed; but not ourselves */ + current->next = NULL; + while(list) { + current = list; + list = list->next; + free(current); + } +} + +/** delete the tap structure */ +static void tap_data_free(struct tap_data* data) +{ + ub_event_del(data->ev); + ub_event_free(data->ev); +#ifdef HAVE_SSL + SSL_free(data->ssl); +#endif + close(data->fd); + free(data->id); + free(data->frame); + data->data_list->d = NULL; + tap_data_list_try_to_free_tail(data->data_list); + free(data); +} + +/** insert tap_data in the tap_data_list */ +static int tap_data_list_insert(struct tap_data_list** liststart, + struct tap_data* d) +{ + struct tap_data_list* entry = (struct tap_data_list*) + malloc(sizeof(*entry)); + if(!entry) + return 0; + entry->next = *liststart; + entry->d = d; + d->data_list = entry; + *liststart = entry; + return 1; +} + +/** delete the tap_data_list and free any remaining tap_data */ +static void tap_data_list_delete(struct tap_data_list* list) +{ + struct tap_data_list* e = list, *next; + while(e) { + next = e->next; + if(e->d) { + tap_data_free(e->d); + e->d = NULL; + } + free(e); + e = next; + } +} + /** del the tap event */ static void tap_socket_delev(struct tap_socket* s) { @@ -184,6 +271,7 @@ static void tap_socket_delete(struct tap_socket* s) #ifdef HAVE_SSL SSL_CTX_free(s->sslctx); #endif + tap_data_list_delete(s->data_list); ub_event_free(s->ev); free(s->socketpath); free(s->ip); @@ -728,20 +816,6 @@ static ssize_t tap_receive(struct tap_data* data, void* buf, size_t len) return receive_bytes(data, data->fd, buf, len); } -/** delete the tap structure */ -static void tap_data_free(struct tap_data* data) -{ - ub_event_del(data->ev); - ub_event_free(data->ev); -#ifdef HAVE_SSL - SSL_free(data->ssl); -#endif - close(data->fd); - free(data->id); - free(data->frame); - free(data); -} - /** reply with ACCEPT control frame to bidirectional client, * returns 0 on error */ static int reply_with_accept(struct tap_data* data) @@ -1046,7 +1120,6 @@ void dtio_tap_callback(int ATTR_UNUSED(fd), short ATTR_UNUSED(bits), void* arg) data->len = 0; data->len_done = 0; data->data_done = 0; - } /** callback for main listening file descriptor */ @@ -1129,6 +1202,8 @@ void dtio_mainfdcallback(int fd, short ATTR_UNUSED(bits), void* arg) &dtio_tap_callback, data); if(!data->ev) fatal_exit("could not ub_event_new"); if(ub_event_add(data->ev, NULL) != 0) fatal_exit("could not ub_event_add"); + if(!tap_data_list_insert(&tap_sock->data_list, data)) + fatal_exit("could not tap_data_list_insert"); } /** setup local accept sockets */ @@ -1243,6 +1318,79 @@ setup_and_run(struct config_strlist_head* local_list, free(maindata); } +/* internal unit tests */ +static int internal_unittest() +{ + /* unit test tap_data_list_try_to_free_tail() */ +#define unit_tap_datas_max 5 + struct tap_data* datas[unit_tap_datas_max]; + struct tap_data_list* list; + struct tap_socket* socket = calloc(1, sizeof(*socket)); + size_t i = 0; + log_assert(socket); + log_assert(unit_tap_datas_max>2); /* needed for the test */ + for(i=0; idata_list, datas[i])); + } + /* sanity base check */ + list = socket->data_list; + for(i=0; list; i++) list = list->next; + log_assert(i==unit_tap_datas_max); + + /* 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==unit_tap_datas_max); + + /* Free the third to last data, tail cannot be erased */ + list = socket->data_list; + for(i=0; inext; + 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==unit_tap_datas_max); + + /* Free the second to last data, try to remove tail from the third + * again, tail (last 2) should be removed */ + list = socket->data_list; + for(i=0; inext; + free(list->d); + list->d = NULL; + list = socket->data_list; + while(list->d) list = list->next; + tap_data_list_try_to_free_tail(list); + list = socket->data_list; + for(i=0; list; i++) list = list->next; + log_assert(i==unit_tap_datas_max-2); + + /* Free all the remaining data, try to remove tail from the start, + * only the start should remain */ + list = socket->data_list; + while(list) { + free(list->d); + list->d = NULL; + list = list->next; + } + tap_data_list_try_to_free_tail(socket->data_list); + list = socket->data_list; + for(i=0; list; i++) list = list->next; + log_assert(i==1); + + /* clean up */ + tap_data_list_delete(socket->data_list); + free(socket); + return 0; +} + /** getopt global, in case header files fail to declare it. */ extern int optind; /** getopt global, in case header files fail to declare it. */ @@ -1293,7 +1441,7 @@ int main(int argc, char** argv) #endif /* command line options */ - while( (c=getopt(argc, argv, "hls:t:u:vx:y:z:")) != -1) { + while( (c=getopt(argc, argv, "hcls:t:u:vx:y:z:")) != -1) { switch(c) { case 'u': if(!cfg_strlist_append(&local_list, @@ -1329,6 +1477,12 @@ int main(int argc, char** argv) case 'v': verbosity++; break; + case 'c': +#ifndef UNBOUND_DEBUG + fatal_exit("-c option needs compilation with " + "--enable-debug"); +#endif + return internal_unittest(); case 'h': case '?': default: diff --git a/testdata/02-unittest.tdir/02-unittest.test b/testdata/02-unittest.tdir/02-unittest.test index 7b1105b74..e5fb0c308 100644 --- a/testdata/02-unittest.tdir/02-unittest.test +++ b/testdata/02-unittest.tdir/02-unittest.test @@ -7,7 +7,7 @@ . ../common.sh PRE="../.." get_make -(cd $PRE ; $MAKE unittest; $MAKE lock-verify) +(cd $PRE ; $MAKE unittest; $MAKE lock-verify; $MAKE unbound-dnstap-socket) if test -f $PRE/unbound_do_valgrind_in_test; then do_valgrind=yes @@ -16,11 +16,16 @@ else fi VALGRIND_FLAGS="--leak-check=full --show-leak-kinds=all" +## START -- Loop over unit tests +## +for unit_cmd in "unittest" "unbound-dnstap-socket -c"; do + +echo "> testing $unit_cmd" if test $do_valgrind = "yes"; then echo "valgrind yes" echo tmpout=/tmp/tmpout.$$ - if (cd $PRE; valgrind $VALGRIND_FLAGS ./unittest >$tmpout 2>&1); then + if (cd $PRE; valgrind $VALGRIND_FLAGS ./$unit_cmd >$tmpout 2>&1); then echo "unit test worked." else echo "unit test failed." @@ -30,7 +35,7 @@ if test $do_valgrind = "yes"; then : # clean else cat $tmpout - echo "Memory leaked in unittest" + echo "Memory leaked in unit test" grep "in use at exit" $tmpout exit 1 fi @@ -38,14 +43,14 @@ if test $do_valgrind = "yes"; then : # clean else cat $tmpout - echo "Errors in unittest" + echo "Errors in unit test" grep "ERROR SUMMARY" $tmpout exit 1 fi rm -f $tmpout else # without valgrind - if (cd $PRE; ./unittest); then + if (cd $PRE; ./$unit_cmd); then echo "unit test worked." else echo "unit test failed." @@ -60,4 +65,9 @@ if test -f $PRE/ublocktrace.0; then exit 1 fi fi + +done +## +## END -- Loop over unit tests + exit 0