- Merge #1080: AddressSanitizer detection in tdir tests and memory leak

gthess · gthess · commit ad12109191ed · 2024-06-04T17:34:58.000+02:00
fixes.
diff --git a/daemon/daemon.c b/daemon/daemon.c
@@ -503,7 +503,10 @@ daemon_clear_allocs(struct daemon* daemon)
 {
 	int i;
 
-	for(i=0; i<daemon->num; i++) {
+	/* daemon->num may be different during reloads (after configuration
+	 * read). Use old_num which has the correct value used to setup the
+	 * worker_allocs */
+	for(i=0; i<daemon->old_num; i++) {
 		alloc_clear(daemon->worker_allocs[i]);
 		free(daemon->worker_allocs[i]);
 	}
diff --git 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 <socketpath> listen to unix socket with this file name\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("-x <server.key> server key 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("-l 		long format for DNS printout\n");
-	printf("-v 		more verbose log output\n");
-	printf("-h 		this help text\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("-t <serverip[@port]>	listen for TLS on IP and port\n");
+	printf("-x <server.key>		server key 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("-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; 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:
diff --git a/doc/Changelog b/doc/Changelog
@@ -1,3 +1,11 @@
+4 June 2024: Yorgos
+	- Merge #1080: AddressSanitizer detection in tdir tests and memory leak
+	  fixes.
+	- Fix memory leak when reload_keep_cache is used and num-threads
+	  changes.
+	- Fix memory leak on exit for unbound-dnstap-socket; creates false
+	  negatives during testing.
+
 3 June 2024: Wouter
 	- Fix to squelch connection reset by peer errors from log. And fix
 	  that the tcp read errors are labeled as initial for the first calls.
diff --git a/testcode/mini_tdir.sh b/testcode/mini_tdir.sh
@@ -127,6 +127,7 @@ dir=$name.$$
 result=result.$name
 done=.done-$name
 skip=.skip-$name
+asan_text="SUMMARY: AddressSanitizer"
 success="no"
 if test -x "`which bash`"; then
 	shell="bash"
@@ -200,6 +201,16 @@ if test -f $name.post -a ! -f ../$skip; then
 		echo "Warning: $name.post did not exit successfully"
 	fi
 fi
+# Check if there were any AddressSanitizer errors
+# if compiled with -fsanitize=address
+if grep "$asan_text" $result >/dev/null 2>&1; then
+	if test -f ../$done; then
+		rm ../$done
+	fi
+	echo "$name: FAILED (AddressSanitizer)" >> $result
+	echo "$name: FAILED (AddressSanitizer)"
+	success="no"
+fi
 echo "DateRunEnd: "`date "+%s" 2>/dev/null` >> $result
 
 mv $result ..
diff --git a/testdata/02-unittest.tdir/02-unittest.test b/testdata/02-unittest.tdir/02-unittest.test

Original file line number	Diff line number	Diff line change
`@@ -503,7 +503,10 @@ daemon_clear_allocs(struct daemon* daemon)`
`503`	`503`	`{`
`504`	`504`	`int i;`
`505`	`505`
`506`		`- for(i=0; i<daemon->num; i++) {`
	`506`	`+ /* daemon->num may be different during reloads (after configuration`
	`507`	`+ * read). Use old_num which has the correct value used to setup the`
	`508`	`+ * worker_allocs */`
	`509`	`+ for(i=0; i<daemon->old_num; i++) {`
`507`	`510`	`alloc_clear(daemon->worker_allocs[i]);`
`508`	`511`	`free(daemon->worker_allocs[i]);`
`509`	`512`	`}`