From 3e5348f84f7e054e5ff63658f125a2b5954fb62c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Ga=C5=82at?= Date: Thu, 27 Jun 2024 08:41:17 +0000 Subject: [PATCH] sample: fix issue with memory leak on events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some events have context allocated on heap, but it was not freed in all states. Signed-off-by: Robert Gałat --- samples/sid_end_device/include/sidewalk.h | 4 +- samples/sid_end_device/src/cli/app.c | 4 +- samples/sid_end_device/src/cli/app_shell.c | 46 ++++++++++++++----- samples/sid_end_device/src/hello/app.c | 39 ++++++++++++---- .../sid_end_device/src/sbdt/file_transfer.c | 4 +- .../src/sensor_monitoring/app.c | 6 +-- .../src/sensor_monitoring/app_tx.c | 17 +++++-- samples/sid_end_device/src/sidewalk.c | 37 +++++++++++---- .../sid_dut_shell/mock/sidewalk_mock.h | 2 +- 9 files changed, 118 insertions(+), 41 deletions(-) diff --git a/samples/sid_end_device/include/sidewalk.h b/samples/sid_end_device/include/sidewalk.h index e66e37e367..c2d2f0d53d 100644 --- a/samples/sid_end_device/include/sidewalk.h +++ b/samples/sid_end_device/include/sidewalk.h @@ -24,9 +24,11 @@ typedef enum { SID_EVENT_LAST, } sidewalk_event_t; +typedef void (*ctx_free)(void *ctx); typedef struct { sidewalk_event_t id; void *ctx; + ctx_free ctx_free; } sidewalk_ctx_event_t; typedef struct { @@ -56,7 +58,7 @@ typedef struct { void sidewalk_start(sidewalk_ctx_t *context); -int sidewalk_event_send(sidewalk_event_t event, void *ctx); +int sidewalk_event_send(sidewalk_event_t event, void *ctx, ctx_free free); sidewalk_msg_t *get_message_buffer(uint16_t message_id); diff --git a/samples/sid_end_device/src/cli/app.c b/samples/sid_end_device/src/cli/app.c index 97eef5e4f6..68bf5e2732 100644 --- a/samples/sid_end_device/src/cli/app.c +++ b/samples/sid_end_device/src/cli/app.c @@ -20,7 +20,7 @@ static uint32_t persistent_link_mask; static void on_sidewalk_event(bool in_isr, void *context) { - int err = sidewalk_event_send(SID_EVENT_SIDEWALK, NULL); + int err = sidewalk_event_send(SID_EVENT_SIDEWALK, NULL, NULL); if (err) { LOG_ERR("Send event err %d", err); }; @@ -81,7 +81,7 @@ static void on_sidewalk_status_changed(const struct sid_status *status, void *co } else { memcpy(new_status, status, sizeof(struct sid_status)); } - sidewalk_event_send(SID_EVENT_NEW_STATUS, new_status); + sidewalk_event_send(SID_EVENT_NEW_STATUS, new_status, sid_hal_free); switch (status->state) { case SID_STATE_READY: diff --git a/samples/sid_end_device/src/cli/app_shell.c b/samples/sid_end_device/src/cli/app_shell.c index 728dd319ec..4961f103bf 100644 --- a/samples/sid_end_device/src/cli/app_shell.c +++ b/samples/sid_end_device/src/cli/app_shell.c @@ -208,6 +208,18 @@ static int cmd_sid_option_handle_set_link3_profile(const char *value, return 0; } +static void free_sid_option_event_ctx(void *ctx) +{ + sidewalk_option_t *p_opt = (sidewalk_option_t *)ctx; + if (p_opt == NULL) { + return; + } + if (p_opt->data) { + sid_hal_free(p_opt->data); + } + sid_hal_free(p_opt); +} + static int cmd_sid_option(cli_event_t event, enum sid_option option, void *data, size_t len) { sidewalk_option_t *p_opt = sid_hal_malloc(sizeof(sidewalk_option_t)); @@ -227,12 +239,9 @@ static int cmd_sid_option(cli_event_t event, enum sid_option option, void *data, p_opt->data = NULL; } - int err = sidewalk_event_send((sidewalk_event_t)event, p_opt); + int err = sidewalk_event_send((sidewalk_event_t)event, p_opt, free_sid_option_event_ctx); if (err) { - if (p_opt->data) { - sid_hal_free(p_opt->data); - } - sid_hal_free(p_opt); + free_sid_option_event_ctx(p_opt); err = -ENOMSG; } @@ -262,7 +271,7 @@ static int cmd_sid_simple_param(cli_event_t event, uint32_t *data) } memcpy(event_ctx, data, sizeof(uint32_t)); - int err = sidewalk_event_send((sidewalk_event_t)event, event_ctx); + int err = sidewalk_event_send((sidewalk_event_t)event, event_ctx, sid_hal_free); if (err) { sid_hal_free(event_ctx); return -ENOMSG; @@ -299,7 +308,7 @@ int cmd_sid_deinit(const struct shell *shell, int32_t argc, const char **argv) { CHECK_ARGUMENT_COUNT(argc, CMD_SID_DEINIT_ARG_REQUIRED, CMD_SID_DEINIT_ARG_OPTIONAL); - return sidewalk_event_send((sidewalk_event_t)DUT_EVENT_DEINIT, NULL); + return sidewalk_event_send((sidewalk_event_t)DUT_EVENT_DEINIT, NULL, NULL); } int cmd_sid_start(const struct shell *shell, int32_t argc, const char **argv) @@ -336,6 +345,18 @@ int cmd_sid_stop(const struct shell *shell, int32_t argc, const char **argv) return cmd_sid_simple_param(DUT_EVENT_STOP, &link_type); } +static void free_sid_send_event_ctx(void *ctx) +{ + sidewalk_msg_t *send = (sidewalk_msg_t *)ctx; + if (send == NULL) { + return; + } + if (send->msg.data) { + sid_hal_free(send->msg.data); + } + sid_hal_free(send); +} + int cmd_sid_send(const struct shell *shell, int32_t argc, const char **argv) { CHECK_ARGUMENT_COUNT(argc, CMD_SID_SEND_ARG_REQUIRED, CMD_SID_SEND_ARG_OPTIONAL); @@ -496,16 +517,17 @@ int cmd_sid_send(const struct shell *shell, int32_t argc, const char **argv) sidewalk_msg_t *send = sid_hal_malloc(sizeof(sidewalk_msg_t)); if (!send) { + sid_hal_free(msg.data); return -ENOMEM; } memset(send, 0x0, sizeof(*send)); memcpy(&send->msg, &msg, sizeof(struct sid_msg)); memcpy(&send->desc, &desc, sizeof(struct sid_msg_desc)); - int err = sidewalk_event_send((sidewalk_event_t)SID_EVENT_SEND_MSG, send); + int err = sidewalk_event_send((sidewalk_event_t)SID_EVENT_SEND_MSG, send, + free_sid_send_event_ctx); if (err) { - sid_hal_free(send->msg.data); - sid_hal_free(send); + free_sid_send_event_ctx(send); return -ENOMSG; } @@ -517,7 +539,7 @@ int cmd_sid_factory_reset(const struct shell *shell, int32_t argc, const char ** CHECK_ARGUMENT_COUNT(argc, CMD_SID_FACTORY_RESET_ARG_REQUIRED, CMD_SID_FACTORY_RESET_ARG_OPTIONAL); - int err = sidewalk_event_send((sidewalk_event_t)SID_EVENT_FACTORY_RESET, NULL); + int err = sidewalk_event_send((sidewalk_event_t)SID_EVENT_FACTORY_RESET, NULL, NULL); if (err) { shell_error(shell, "event err %d", err); } @@ -911,7 +933,7 @@ int cmd_sid_last_status(const struct shell *shell, int32_t argc, const char **ar CHECK_ARGUMENT_COUNT(argc, CMD_SID_LAST_STATUS_ARG_REQUIRED, CMD_SID_LAST_STATUS_ARG_OPTIONAL); - if (0 != sidewalk_event_send((sidewalk_event_t)DUT_EVENT_GET_STATUS, NULL)) { + if (0 != sidewalk_event_send((sidewalk_event_t)DUT_EVENT_GET_STATUS, NULL, NULL)) { shell_error(shell, "Failed to send Event"); } return 0; diff --git a/samples/sid_end_device/src/hello/app.c b/samples/sid_end_device/src/hello/app.c index 9f92a9fd24..a1f40aeac7 100644 --- a/samples/sid_end_device/src/hello/app.c +++ b/samples/sid_end_device/src/hello/app.c @@ -29,12 +29,24 @@ static uint32_t persistent_link_mask; static void on_sidewalk_event(bool in_isr, void *context) { - int err = sidewalk_event_send(SID_EVENT_SIDEWALK, NULL); + int err = sidewalk_event_send(SID_EVENT_SIDEWALK, NULL, NULL); if (err) { LOG_ERR("Send event err %d", err); }; } +static void free_sid_echo_event_ctx(void *ctx) +{ + sidewalk_msg_t *echo = (sidewalk_msg_t *)ctx; + if (echo == NULL) { + return; + } + if (echo->msg.data) { + sid_hal_free(echo->msg.data); + } + sid_hal_free(echo); +} + static void on_sidewalk_msg_received(const struct sid_msg_desc *msg_desc, const struct sid_msg *msg, void *context) { @@ -70,10 +82,9 @@ static void on_sidewalk_msg_received(const struct sid_msg_desc *msg_desc, const echo->desc.link_type = SID_LINK_TYPE_ANY; echo->desc.link_mode = SID_LINK_MODE_CLOUD; - int err = sidewalk_event_send(SID_EVENT_SEND_MSG, echo); + int err = sidewalk_event_send(SID_EVENT_SEND_MSG, echo, free_sid_echo_event_ctx); if (err) { - sid_hal_free(echo->msg.data); - sid_hal_free(echo); + free_sid_echo_event_ctx(echo); LOG_ERR("Send event err %d", err); } else { application_state_sending(&global_state_notifier, true); @@ -141,7 +152,7 @@ static void on_sidewalk_status_changed(const struct sid_status *status, void *co } else { memcpy(new_status, status, sizeof(struct sid_status)); } - err = sidewalk_event_send(SID_EVENT_NEW_STATUS, new_status); + err = sidewalk_event_send(SID_EVENT_NEW_STATUS, new_status, sid_hal_free); switch (status->state) { case SID_STATE_READY: @@ -190,6 +201,17 @@ static void on_sidewalk_status_changed(const struct sid_status *status, void *co } } } +static void free_sid_hello_event_ctx(void *ctx) +{ + sidewalk_msg_t *hello = (sidewalk_msg_t *)ctx; + if (hello == NULL) { + return; + } + if (hello->msg.data) { + sid_hal_free(hello->msg.data); + } + sid_hal_free(hello->msg.data); +} static void app_button_handler(uint32_t event) { @@ -216,10 +238,9 @@ static void app_button_handler(uint32_t event) hello->desc.link_type = SID_LINK_TYPE_ANY; hello->desc.link_mode = SID_LINK_MODE_CLOUD; - int err = sidewalk_event_send(SID_EVENT_SEND_MSG, hello); + int err = sidewalk_event_send(SID_EVENT_SEND_MSG, hello, free_sid_hello_event_ctx); if (err) { - sid_hal_free(hello->msg.data); - sid_hal_free(hello); + free_sid_hello_event_ctx(hello); LOG_ERR("Send event err %d", err); } else { application_state_sending(&global_state_notifier, true); @@ -227,7 +248,7 @@ static void app_button_handler(uint32_t event) return; } - sidewalk_event_send((sidewalk_event_t)event, NULL); + sidewalk_event_send((sidewalk_event_t)event, NULL, NULL); } static int app_buttons_init(void) diff --git a/samples/sid_end_device/src/sbdt/file_transfer.c b/samples/sid_end_device/src/sbdt/file_transfer.c index db3d42ebad..a4a4692aae 100644 --- a/samples/sid_end_device/src/sbdt/file_transfer.c +++ b/samples/sid_end_device/src/sbdt/file_transfer.c @@ -76,7 +76,7 @@ static void on_data_received(const struct sid_bulk_data_transfer_desc *const des transfer->data = buffer->data; transfer->data_size = buffer->size; - int err = sidewalk_event_send(SID_EVENT_FILE_TRANSFER, transfer); + int err = sidewalk_event_send(SID_EVENT_FILE_TRANSFER, transfer, sid_hal_free); if (err) { LOG_ERR("Event transfer err %d", err); LOG_INF("Cancelig file transfer"); @@ -117,7 +117,7 @@ static void on_finalize_request(uint32_t file_id, void *context) LOG_ERR("dfu image finalize fail %d", err); } - err = sidewalk_event_send(SID_EVENT_REBOOT, NULL); + err = sidewalk_event_send(SID_EVENT_REBOOT, NULL, NULL); if (err) { LOG_ERR("reboot event send ret %d", err); } diff --git a/samples/sid_end_device/src/sensor_monitoring/app.c b/samples/sid_end_device/src/sensor_monitoring/app.c index 6a43a1d721..ef1e5b7fa0 100644 --- a/samples/sid_end_device/src/sensor_monitoring/app.c +++ b/samples/sid_end_device/src/sensor_monitoring/app.c @@ -42,7 +42,7 @@ static sidewalk_ctx_t sid_ctx; static void on_sidewalk_event(bool in_isr, void *context) { - int err = sidewalk_event_send(SID_EVENT_SIDEWALK, NULL); + int err = sidewalk_event_send(SID_EVENT_SIDEWALK, NULL, NULL); if (err) { LOG_ERR("Send event err %d", err); }; @@ -112,7 +112,7 @@ static void on_sidewalk_status_changed(const struct sid_status *status, void *co } else { memcpy(new_status, status, sizeof(struct sid_status)); } - sidewalk_event_send(SID_EVENT_NEW_STATUS, new_status); + sidewalk_event_send(SID_EVENT_NEW_STATUS, new_status, sid_hal_free); int err = 0; switch (status->state) { @@ -165,7 +165,7 @@ static void on_sidewalk_status_changed(const struct sid_status *status, void *co static void sidewalk_btn_handler(uint32_t event) { - int err = sidewalk_event_send((sidewalk_event_t)event, NULL); + int err = sidewalk_event_send((sidewalk_event_t)event, NULL, NULL); if (err) { LOG_ERR("Send event err %d", err); return; diff --git a/samples/sid_end_device/src/sensor_monitoring/app_tx.c b/samples/sid_end_device/src/sensor_monitoring/app_tx.c index 6773184c97..cac8928825 100644 --- a/samples/sid_end_device/src/sensor_monitoring/app_tx.c +++ b/samples/sid_end_device/src/sensor_monitoring/app_tx.c @@ -81,6 +81,18 @@ static uint32_t last_link_mask_get(void) return last_link_mask; } +static void free_sid_msg_event_ctx(void *ctx) +{ + sidewalk_msg_t *sid_msg = (sidewalk_msg_t *)ctx; + if (sid_msg == NULL) { + return; + } + if (sid_msg->msg.data) { + sid_hal_free(sid_msg->msg.data); + } + sid_hal_free(sid_msg); +} + static int app_tx_demo_msg_send(struct sid_parse_state *state, uint8_t *buffer, struct sid_demo_msg_desc *demo_desc, struct sid_msg_desc *sid_desc) { @@ -111,10 +123,9 @@ static int app_tx_demo_msg_send(struct sid_parse_state *state, uint8_t *buffer, memcpy(sid_msg->msg.data, msg_buffer, sid_msg->msg.size); memcpy(&sid_msg->desc, sid_desc, sizeof(struct sid_msg_desc)); - int err = sidewalk_event_send(SID_EVENT_SEND_MSG, sid_msg); + int err = sidewalk_event_send(SID_EVENT_SEND_MSG, sid_msg, free_sid_msg_event_ctx); if (err) { - sid_hal_free(sid_msg->msg.data); - sid_hal_free(sid_msg); + free_sid_msg_event_ctx(sid_msg); LOG_ERR("Event send err %d", err); return -EIO; }; diff --git a/samples/sid_end_device/src/sidewalk.c b/samples/sid_end_device/src/sidewalk.c index 14e918e6f7..86f524430a 100644 --- a/samples/sid_end_device/src/sidewalk.c +++ b/samples/sid_end_device/src/sidewalk.c @@ -312,7 +312,6 @@ static void state_sidewalk_run(void *o) } memcpy(&sm->sid->last_status, p_status, sizeof(struct sid_status)); - sid_hal_free(p_status); } break; case SID_EVENT_SEND_MSG: { sidewalk_msg_t *p_msg = (sidewalk_msg_t *)sm->event.ctx; @@ -321,17 +320,37 @@ static void state_sidewalk_run(void *o) break; } - e = sid_put_msg(sm->sid->handle, &p_msg->msg, &p_msg->desc); + /* Making a copy of the data is a workaround for issue KRKNWK-18805 + When sid_put_msg makes intenal copy, the workaround can be removed. + */ + sidewalk_msg_t *p_msg_copy = sid_hal_malloc(sizeof(sidewalk_msg_t)); + memset(p_msg_copy, 0x0, sizeof(*p_msg_copy)); + + p_msg_copy->msg.data = sid_hal_malloc(p_msg->msg.size); + if (p_msg_copy->msg.data == NULL) { + sid_hal_free(p_msg_copy); + LOG_ERR("Failed to allocate message buffer"); + break; + } + memcpy(p_msg_copy->msg.data, p_msg->msg.data, p_msg->msg.size); + p_msg_copy->msg.size = p_msg->msg.size; + + e = sid_put_msg(sm->sid->handle, p_msg_copy->msg.data, &p_msg_copy->desc); if (e) { LOG_ERR("sid send err %d", (int)e); + sid_hal_free(p_msg_copy->msg.data); + sid_hal_free(p_msg_copy); + break; } LOG_DBG("sid send (type: %d, id: %u)", (int)p_msg->desc.type, p_msg->desc.id); int mutex_err = k_mutex_lock(&pending_message_list_mutex, K_FOREVER); if (mutex_err != 0) { LOG_ERR("Failed to lock mutex for message list"); + sid_hal_free(p_msg_copy->msg.data); + sid_hal_free(p_msg_copy); break; } - sys_slist_append(&pending_message_list, &p_msg->node); + sys_slist_append(&pending_message_list, &p_msg_copy->node); k_mutex_unlock(&pending_message_list_mutex); } break; case SID_EVENT_CONNECT: { @@ -393,7 +412,6 @@ static void state_sidewalk_run(void *o) LOG_ERR("sbdt cancel ret %s", SID_ERROR_T_STR(e)); } - sid_hal_free(transfer); break; } #endif /* CONFIG_SIDEWALK_FILE_TRANSFER_DFU */ @@ -407,8 +425,6 @@ static void state_sidewalk_run(void *o) LOG_ERR("sbdt release ret %s", SID_ERROR_T_STR(e)); } - // free event context - sid_hal_free(transfer); #endif /* CONFIG_SIDEWALK_FILE_TRANSFER */ } break; case SID_EVENT_REBOOT: { @@ -506,7 +522,11 @@ static void sid_thread_entry(void *context, void *unused, void *unused2) while (1) { int err = k_msgq_get(&sid_sm.msgq, &sid_sm.event, K_FOREVER); if (!err) { - if (smf_run_state(SMF_CTX(&sid_sm))) { + int32_t e = smf_run_state(SMF_CTX(&sid_sm)); + if (sid_sm.event.ctx_free) { + sid_sm.event.ctx_free(sid_sm.event.ctx); + } + if (e) { LOG_ERR("Sidewalk state machine termination"); break; } @@ -526,11 +546,12 @@ void sidewalk_start(sidewalk_ctx_t *context) (void)k_thread_name_set(&sid_thread, "sidewalk"); } -int sidewalk_event_send(sidewalk_event_t event, void *ctx) +int sidewalk_event_send(sidewalk_event_t event, void *ctx, ctx_free free) { sidewalk_ctx_event_t ctx_event = { .id = event, .ctx = ctx, + .ctx_free = free, }; k_timeout_t timeout = K_NO_WAIT; diff --git a/tests/unit_tests/sid_dut_shell/mock/sidewalk_mock.h b/tests/unit_tests/sid_dut_shell/mock/sidewalk_mock.h index ff3aa3808d..ea9c00fd46 100644 --- a/tests/unit_tests/sid_dut_shell/mock/sidewalk_mock.h +++ b/tests/unit_tests/sid_dut_shell/mock/sidewalk_mock.h @@ -15,7 +15,7 @@ DEFINE_FFF_GLOBALS; FAKE_VOID_FUNC(sidewalk_start, sidewalk_ctx_t *); FAKE_VALUE_FUNC(void *, sid_hal_malloc, size_t); FAKE_VOID_FUNC(sid_hal_free, void *); -FAKE_VALUE_FUNC(int, sidewalk_event_send, sidewalk_event_t, void *); +FAKE_VALUE_FUNC(int, sidewalk_event_send, sidewalk_event_t, void *, ctx_free); #define SIDEWALK_FAKES_LIST(FAKE) \ FAKE(sidewalk_start) \