- Fix memory leak on exit for unbound-dnstap-socket; creates false negatives

during testing.

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 <socketpath> listen to unix socket with this file name\n");
+	printf("-u <socketpath>		listen to unix socket with this file name\n");
-	printf("-s <serverip[@port]> listen for TCP on the IP and port\n");
+	printf("-s <serverip[@port]>	listen for TCP on the IP and port\n");
-	printf("-t <serverip[@port]> listen for TLS on IP and port\n");
+	printf("-t <serverip[@port]>	listen for TLS on IP and port\n");
-	printf("-x <server.key> server key file for TLS service\n");
+	printf("-x <server.key>		server key file for TLS service\n");
-	printf("-y <server.pem> server cert file for TLS service\n");
+	printf("-y <server.pem>		server cert file for TLS service\n");
-	printf("-z <verify.pem> cert file to verify client connections\n");
+	printf("-z <verify.pem>		cert file to verify client connections\n");
-	printf("-l 		long format for DNS printout\n");
+	printf("-l			long format for DNS printout\n");
-	printf("-v 		more verbose log output\n");
+	printf("-v			more verbose log output\n");
-	printf("-h 		this help text\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; i<unit_tap_datas_max; 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==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; i<unit_tap_datas_max-3; i++) 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 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; i<unit_tap_datas_max-2; i++) list = list->next;
+	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:

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