- 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.

diff --git a/dnstap/unbound-dnstap-socket.c b/dnstap/unbound-dnstap-socket.c
index a437b62967858979e6dd8b6dcb94e44ecd1379c7..f203aa7d73a16a1ad9fb08c760e67a56992583b0 100644 (file)
--- 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 ec8024cc13f80ffcbc0069387da5169e56562683..7e1b81eb4ddf99a9edf0971122b266538c15e96e 100644 (file)
--- 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 6d5e9d50d044390961a25e7d1eb0a5526121acaf..8fefc7e844b2a6eb40f0b4bc01706e720a2521d5 100644 (file)
--- 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 3ec9c77bd0c85927e39dcd69720be1464c214739..ebb1802513be99631e84b103ce87dab487de3f8d 100644 (file)
--- 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