View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0007645 | GNUnet | cadet service | public | 2023-01-29 23:39 | 2024-03-07 21:38 |
| Reporter | ulfvonbelow | Assigned To | |||
| Priority | normal | Severity | trivial | Reproducibility | always |
| Status | assigned | Resolution | open | ||
| Product Version | Git master | ||||
| Target Version | 0.20.0 | ||||
| Summary | 0007645: Memory leaks in test_cadet.c | ||||
| Description | t_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 Information | Patch 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. | ||||
| Tags | memory-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
| ||||
|
|
I think this needs some investigation, returning void* seems odd. |
|
|
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). |
| 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 |
| 2024-03-07 21:38 | schanzen | Assigned To | schanzen => |