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