View Issue Details

IDProjectCategoryView StatusLast Update
0007645GNUnetcadet servicepublic2023-06-01 20:25
Reporterulfvonbelow Assigned Toschanzen  
PrioritynormalSeveritytrivialReproducibilityalways
Status assignedResolutionopen 
Product VersionGit master 
Target Version0.20.0 
Summary0007645: Memory leaks in test_cadet.c
Descriptiont_op's elements are overwritten without any chance of freeing them. Many channel wrappers are leaked.
Steps To Reproduce./configure --enable-sanitizer
make
make install
make check
Additional InformationPatch attached. Note that it modifies the cadet API slightly, so that GNUNET_CADET_channel_destroy returns the closure it was created with. If this is undesirable, I ask that someone more familiar with the workings of this many-tests-in-one program fix its memory leaks.
Tagsmemory-leak, patch
Attached Files
0001-CADET-return-closure-from-GNUNET_CADET_channel_destr.patch (7,422 bytes)   
From 8b9c9ee72fded119b4ee3dc8fb47cd454836bd7c Mon Sep 17 00:00:00 2001
From: ulfvonbelow <strilen@tilde.club>
Date: Sun, 29 Jan 2023 06:53:10 -0600
Subject: [PATCH] CADET: return closure from GNUNET_CADET_channel_destroy.

We already have similar behavior for other cancellation/destruction-type
procedures that take a handle that was given a closure at creation-time.

Also uses this new functionality to fix memory leaks in test_cadet.c. If this
API change is believed to be unreasonable, please fix the test some other
way.
---
 src/cadet/cadet_api.c              |  5 ++-
 src/cadet/test_cadet.c             | 57 +++++++++++++++++-------------
 src/include/gnunet_cadet_service.h |  3 +-
 3 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/src/cadet/cadet_api.c b/src/cadet/cadet_api.c
index 0bfb01868..d8fc7f747 100644
--- a/src/cadet/cadet_api.c
+++ b/src/cadet/cadet_api.c
@@ -826,12 +826,13 @@ GNUNET_CADET_close_port (struct GNUNET_CADET_Port *p)
  *
  * @param channel Channel handle, becomes invalid after this call.
  */
-void
+void *
 GNUNET_CADET_channel_destroy (struct GNUNET_CADET_Channel *channel)
 {
   struct GNUNET_CADET_Handle *h = channel->cadet;
   struct GNUNET_CADET_LocalChannelDestroyMessage *msg;
   struct GNUNET_MQ_Envelope *env;
+  void *ctx;
 
   if (NULL != h->mq)
   {
@@ -842,7 +843,9 @@ GNUNET_CADET_channel_destroy (struct GNUNET_CADET_Channel *channel)
   GNUNET_log (GNUNET_ERROR_TYPE_INFO,
               "Destroying channel due to GNUNET_CADET_channel_destroy()\n");
   channel->disconnects = NULL;
+  ctx = channel->ctx;
   destroy_channel (channel);
+  return ctx;
 }
 
 
diff --git a/src/cadet/test_cadet.c b/src/cadet/test_cadet.c
index 61c09f389..4f7638957 100644
--- a/src/cadet/test_cadet.c
+++ b/src/cadet/test_cadet.c
@@ -130,7 +130,12 @@ static size_t size_payload = sizeof(uint32_t);
 /**
  * Operation to get peer ids.
  */
-static struct GNUNET_TESTBED_Operation *t_op[2];
+static struct GNUNET_TESTBED_Operation *t_op_i[2];
+
+/**
+ * Operation to get peer configurations.
+ */
+static struct GNUNET_TESTBED_Operation *t_op_c[2];
 
 /**
  * Peer ids.
@@ -338,16 +343,17 @@ disconnect_cadet_peers (void *cls)
               line);
   for (unsigned int i = 0; i < 2; i++)
   {
-    GNUNET_TESTBED_operation_done (t_op[i]);
+    GNUNET_TESTBED_operation_done (t_op_i[i]);
+    GNUNET_TESTBED_operation_done (t_op_c[i]);
   }
   if (NULL != outgoing_ch)
   {
-    GNUNET_CADET_channel_destroy (outgoing_ch);
+    GNUNET_free_nz (GNUNET_CADET_channel_destroy (outgoing_ch));
     outgoing_ch = NULL;
   }
   if (NULL != incoming_ch)
   {
-    GNUNET_CADET_channel_destroy (incoming_ch);
+    GNUNET_free_nz (GNUNET_CADET_channel_destroy (incoming_ch));
     incoming_ch = NULL;
   }
   GNUNET_CADET_TEST_cleanup (test_ctx);
@@ -477,7 +483,7 @@ gather_stats_and_exit (void *cls)
               l);
   if (NULL != outgoing_ch)
   {
-    GNUNET_CADET_channel_destroy (outgoing_ch);
+    GNUNET_free_nz (GNUNET_CADET_channel_destroy (outgoing_ch));
     outgoing_ch = NULL;
   }
   stats_op = GNUNET_TESTBED_get_statistics (peers_running,
@@ -610,7 +616,7 @@ reconnect_op (void *cls)
               l);
   if (NULL != outgoing_ch)
   {
-    GNUNET_CADET_channel_destroy (outgoing_ch);
+    GNUNET_free_nz (GNUNET_CADET_channel_destroy (outgoing_ch));
     outgoing_ch = NULL;
   }
   ch = GNUNET_new (struct CadetTestChannelWrapper);
@@ -771,6 +777,7 @@ disconnect_handler (void *cls,
       get_peers_task = GNUNET_SCHEDULER_add_now (&get_peers,
                                                  NULL);
     }
+    GNUNET_free (ch_w);
     return;
   }
 
@@ -1069,7 +1076,7 @@ handle_data (void *cls,
         GNUNET_log (GNUNET_ERROR_TYPE_INFO,
                     "Destroying channel %p...\n",
                     outgoing_ch);
-        GNUNET_CADET_channel_destroy (outgoing_ch);
+        GNUNET_free_nz (GNUNET_CADET_channel_destroy (outgoing_ch));
         outgoing_ch = NULL;
       }
     }
@@ -1131,12 +1138,12 @@ handle_data (void *cls,
     }
     if (test == P2P_SIGNAL)
     {
-      GNUNET_CADET_channel_destroy (incoming_ch);
+      GNUNET_free_nz (GNUNET_CADET_channel_destroy (incoming_ch));
       incoming_ch = NULL;
     }
     else
     {
-      GNUNET_CADET_channel_destroy (outgoing_ch);
+      GNUNET_free_nz (GNUNET_CADET_channel_destroy (outgoing_ch));
       outgoing_ch = NULL;
     }
   }
@@ -1397,22 +1404,22 @@ tmain (void *cls,
                                                   (void *) __LINE__);
   GNUNET_SCHEDULER_add_shutdown (&shutdown_task,
                                  NULL);
-  t_op[0] = GNUNET_TESTBED_peer_get_information (peers[0],
-                                                 GNUNET_TESTBED_PIT_IDENTITY,
-                                                 &pi_cb,
-                                                 (void *) 0L);
-  t_op[1] = GNUNET_TESTBED_peer_get_information (peers[num_peers - 1],
-                                                 GNUNET_TESTBED_PIT_IDENTITY,
-                                                 &pi_cb,
-                                                 (void *) 1L);
-  t_op[0] = GNUNET_TESTBED_peer_get_information (peers[0],
-                                                 GNUNET_TESTBED_PIT_CONFIGURATION,
-                                                 &pi_cb,
-                                                 (void *) 0L);
-  t_op[1] = GNUNET_TESTBED_peer_get_information (peers[num_peers - 1],
-                                                 GNUNET_TESTBED_PIT_CONFIGURATION,
-                                                 &pi_cb,
-                                                 (void *) 1L);
+  t_op_i[0] = GNUNET_TESTBED_peer_get_information (peers[0],
+                                                   GNUNET_TESTBED_PIT_IDENTITY,
+                                                   &pi_cb,
+                                                   (void *) 0L);
+  t_op_i[1] = GNUNET_TESTBED_peer_get_information (peers[num_peers - 1],
+                                                   GNUNET_TESTBED_PIT_IDENTITY,
+                                                   &pi_cb,
+                                                   (void *) 1L);
+  t_op_c[0] = GNUNET_TESTBED_peer_get_information (peers[0],
+                                                   GNUNET_TESTBED_PIT_CONFIGURATION,
+                                                   &pi_cb,
+                                                   (void *) 0L);
+  t_op_c[1] = GNUNET_TESTBED_peer_get_information (peers[num_peers - 1],
+                                                   GNUNET_TESTBED_PIT_CONFIGURATION,
+                                                   &pi_cb,
+                                                   (void *) 1L);
   GNUNET_log (GNUNET_ERROR_TYPE_DEBUG, "requested peer ids\n");
 }
 
diff --git a/src/include/gnunet_cadet_service.h b/src/include/gnunet_cadet_service.h
index acc7bb330..89ae1d0c1 100644
--- a/src/include/gnunet_cadet_service.h
+++ b/src/include/gnunet_cadet_service.h
@@ -244,8 +244,9 @@ GNUNET_CADET_channel_create (struct GNUNET_CADET_Handle *h,
  * accepted and no data callbacks will be called.
  *
  * @param channel Channel handle, becomes invalid after this call.
+ * @return Closure that was given to #GNUNET_CADET_channel_create().
  */
-void
+void *
 GNUNET_CADET_channel_destroy (struct GNUNET_CADET_Channel *channel);
 
 
-- 
2.38.1

Activities

schanzen

2023-02-06 05:58

administrator   ~0019763

I think this needs some investigation, returning void* seems odd.

schanzen

2023-02-15 10:20

administrator   ~0019845

It boils down to this: The "ctx" that is passed to GNUNET_CADET_channel_create() as a second argument is the responsibility of the caller, not the API.
The caller must have a way of cleaning this up. Either when the API is "done" and eventually passes ctx again (disconnect handler) or when the caller itself does a cleanup after, e.g. deciding to abort.
The tests needs to track the CadetTestChannelWrapper for the respective channel. Mostly they do. But there may be a race condition timeout.
I don't think it is really worth fixing. But the way to do it would probably to introduce global variables for the wrapper struct(s).

Issue History

Date Modified Username Field Change
2023-01-29 23:39 ulfvonbelow New Issue
2023-01-29 23:39 ulfvonbelow Tag Attached: memory-leak
2023-01-29 23:39 ulfvonbelow Tag Attached: patch
2023-01-29 23:39 ulfvonbelow File Added: 0001-CADET-return-closure-from-GNUNET_CADET_channel_destr.patch
2023-02-06 05:58 schanzen Note Added: 0019763
2023-02-06 05:58 schanzen Assigned To => schanzen
2023-02-06 05:58 schanzen Status new => assigned
2023-02-06 06:13 schanzen Assigned To schanzen => t3sserakt
2023-02-06 06:13 schanzen Target Version => 0.19.4
2023-02-13 09:17 schanzen Assigned To t3sserakt => schanzen
2023-02-15 10:20 schanzen Note Added: 0019845
2023-06-01 20:25 schanzen Target Version 0.19.4 => 0.20.0