From 8631afa312f149e61118884745d4389da8696bd9 Mon Sep 17 00:00:00 2001 From: Damian Krolik Date: Mon, 24 Mar 2025 13:11:48 +0100 Subject: [PATCH] net: openthread: rpc: fix handle leak in CoAP RPC server 1. Fix message handle leak after sending a CoAP response. 2. Add unit tests to verify that handle is allocated/freed. 3. Improve CoAP RPC server thread safety. 4. Other minor fixes and improvements. Signed-off-by: Damian Krolik --- .../net/openthread/rpc/server/ot_rpc_coap.c | 43 ++++++------ .../openthread/rpc/server/ot_rpc_message.c | 15 ++--- .../openthread/rpc/server/src/coap_suite.c | 65 +++++++++++++++++-- 3 files changed, 86 insertions(+), 37 deletions(-) diff --git a/subsys/net/openthread/rpc/server/ot_rpc_coap.c b/subsys/net/openthread/rpc/server/ot_rpc_coap.c index e36f0909c372..3e56a20231ba 100644 --- a/subsys/net/openthread/rpc/server/ot_rpc_coap.c +++ b/subsys/net/openthread/rpc/server/ot_rpc_coap.c @@ -71,23 +71,18 @@ static void ot_rpc_cmd_coap_new_message(const struct nrf_rpc_group *group, openthread_api_mutex_lock(openthread_get_default_context()); message = otCoapNewMessage(openthread_get_default_instance(), settings); - openthread_api_mutex_unlock(openthread_get_default_context()); - - if (!message) { - goto out; - } - message_rep = ot_reg_msg_alloc(message); - if (!message_rep) { + if ((message != NULL) && !message_rep) { /* - * If failed to allocate the message handle, the ownership can't be passed to the - * RPC client. Therefore, free the message. + * Failed to allocate the message handle, so the ownership can't be passed to + * the RPC client. Therefore, free the message. */ otMessageFree(message); } -out: + openthread_api_mutex_unlock(openthread_get_default_context()); + nrf_rpc_rsp_send_uint(group, message_rep); } @@ -180,7 +175,7 @@ static void ot_rpc_cmd_coap_message_append_uri_path_options(const struct nrf_rpc message_rep = nrf_rpc_decode_uint(ctx); nrf_rpc_decode_str(ctx, uri, sizeof(uri)); - if (!nrf_rpc_decode_valid(ctx)) { + if (!nrf_rpc_decoding_done_and_check(group, ctx)) { ot_rpc_report_cmd_decoding_error(OT_RPC_CMD_COAP_MESSAGE_APPEND_URI_PATH_OPTIONS); return; } @@ -194,7 +189,6 @@ static void ot_rpc_cmd_coap_message_append_uri_path_options(const struct nrf_rpc openthread_api_mutex_lock(openthread_get_default_context()); error = otCoapMessageAppendUriPathOptions(message, uri); - nrf_rpc_cbor_decoding_done(group, ctx); openthread_api_mutex_unlock(openthread_get_default_context()); nrf_rpc_rsp_send_uint(group, error); @@ -455,6 +449,8 @@ static void ot_rpc_coap_resource_handler(void *aContext, otMessage *aMessage, message_rep = ot_reg_msg_alloc(aMessage); if (!message_rep) { + nrf_rpc_err(-ENOMEM, NRF_RPC_ERR_SRC_SEND, &ot_group, + OT_RPC_CMD_COAP_RESOURCE_HANDLER, NRF_RPC_PACKET_TYPE_CMD); return; } @@ -559,6 +555,8 @@ static void ot_rpc_coap_default_handler(void *aContext, otMessage *aMessage, message_rep = ot_reg_msg_alloc(aMessage); if (!message_rep) { + nrf_rpc_err(-ENOMEM, NRF_RPC_ERR_SRC_SEND, &ot_group, + OT_RPC_CMD_COAP_DEFAULT_HANDLER, NRF_RPC_PACKET_TYPE_CMD); return; } @@ -606,13 +604,12 @@ static void ot_rpc_coap_response_handler(void *context, otMessage *message, struct nrf_rpc_cbor_ctx ctx; size_t cbor_buffer_size = 0; - if (message) { - message_rep = ot_reg_msg_alloc(message); - - if (!message_rep) { - return; - } - } + message_rep = ot_reg_msg_alloc(message); + /* + * Ignore message handle allocation failure. It seems safer to call the client's response + * handler without the response (which indicates response timeout) than not to call the + * handler at all and make the client wait for the response indefinitely. + */ cbor_buffer_size += 1 + sizeof(ot_rpc_coap_request_key); cbor_buffer_size += 1 + sizeof(ot_msg_key); @@ -657,12 +654,12 @@ static void ot_rpc_cmd_coap_send_request(const struct nrf_rpc_group *group, openthread_api_mutex_lock(openthread_get_default_context()); error = otCoapSendRequest(openthread_get_default_instance(), message, &message_info, ot_rpc_coap_response_handler, (void *)request_rep); - openthread_api_mutex_unlock(openthread_get_default_context()); if (error == OT_ERROR_NONE) { ot_msg_free(message_rep); } + openthread_api_mutex_unlock(openthread_get_default_context()); nrf_rpc_rsp_send_uint(group, error); } @@ -694,8 +691,12 @@ static void ot_rpc_cmd_coap_send_response(const struct nrf_rpc_group *group, openthread_api_mutex_lock(openthread_get_default_context()); error = otCoapSendResponse(openthread_get_default_instance(), message, &message_info); - openthread_api_mutex_unlock(openthread_get_default_context()); + if (error == OT_ERROR_NONE) { + ot_msg_free(message_rep); + } + + openthread_api_mutex_unlock(openthread_get_default_context()); nrf_rpc_rsp_send_uint(group, error); } diff --git a/subsys/net/openthread/rpc/server/ot_rpc_message.c b/subsys/net/openthread/rpc/server/ot_rpc_message.c index ca8f6880eed4..818bd0f4cf98 100644 --- a/subsys/net/openthread/rpc/server/ot_rpc_message.c +++ b/subsys/net/openthread/rpc/server/ot_rpc_message.c @@ -37,25 +37,20 @@ ot_msg_key ot_reg_msg_alloc(otMessage *msg) void ot_msg_free(ot_msg_key key) { - key--; - if (key >= OT_MESSAGES_POOL) { + if (key == 0 || key > OT_MESSAGES_POOL) { return; } - if (ot_message_registry[key] != NULL) { - ot_message_registry[key] = NULL; - } + ot_message_registry[key - 1] = NULL; } otMessage *ot_msg_get(ot_msg_key key) { - key--; - - if (key < OT_MESSAGES_POOL) { - return ot_message_registry[key]; + if (key == 0 || key > OT_MESSAGES_POOL) { + return NULL; } - return NULL; + return ot_message_registry[key - 1]; } static void ot_rpc_msg_free(const struct nrf_rpc_group *group, struct nrf_rpc_cbor_ctx *ctx, diff --git a/tests/subsys/net/openthread/rpc/server/src/coap_suite.c b/tests/subsys/net/openthread/rpc/server/src/coap_suite.c index f0a54dbf811b..819b05b5ceff 100644 --- a/tests/subsys/net/openthread/rpc/server/src/coap_suite.c +++ b/tests/subsys/net/openthread/rpc/server/src/coap_suite.c @@ -125,6 +125,7 @@ ZTEST(ot_rpc_coap, test_otCoapNewMessage_max) mock_nrf_rpc_tr_expect_done(); zassert_equal(otCoapNewMessage_fake.call_count, 1); + zassert_not_null(ot_msg_get(1)); } /* @@ -361,7 +362,7 @@ static void otCoapRemoveResource_custom_func(otInstance *instance, otCoapResourc ZTEST(ot_rpc_coap, test_otCoapAddResource_otCoapRemoveResource) { /* - * Message key is allocated by the default handler encoder function but we know that + * Message key is allocated by the resource handler encoder function but we know that * the first free slot will be selected so it will be 1. */ ot_msg_key message_rep = 1; @@ -393,6 +394,7 @@ ZTEST(ot_rpc_coap, test_otCoapAddResource_otCoapRemoveResource) otCoapAddResource_fake.arg1_val->mHandler(otCoapAddResource_fake.arg1_val->mContext, (otMessage *)MSG_ADDR, &message_info); mock_nrf_rpc_tr_expect_done(); + zassert_is_null(ot_msg_get(message_rep)); /* Test reception of otCoapRemoveResource() */ otCoapRemoveResource_fake.custom_fake = otCoapRemoveResource_custom_func; @@ -440,6 +442,7 @@ ZTEST(ot_rpc_coap, test_otCoapSetDefaultHandler) otCoapSetDefaultHandler_fake.arg1_val(otCoapSetDefaultHandler_fake.arg2_val, (otMessage *)MSG_ADDR, &message_info); mock_nrf_rpc_tr_expect_done(); + zassert_is_null(ot_msg_get(message_rep)); /* Test reception of otCoapSetDefaultHandler() command that unsets the default handler */ RESET_FAKE(otCoapSetDefaultHandler); @@ -454,6 +457,8 @@ ZTEST(ot_rpc_coap, test_otCoapSetDefaultHandler) /* * Test reception of otCoapSendRequest(). + * Test serialization of the result: OT_ERROR_NONE. + * Test response handler invocation. */ ZTEST(ot_rpc_coap, test_otCoapSendRequest) { @@ -484,9 +489,9 @@ ZTEST(ot_rpc_coap, test_otCoapSendRequest) mock_nrf_rpc_tr_expect_done(); zassert_equal(otCoapSendRequestWithParameters_fake.call_count, 1); - zassert_equal(otCoapSendRequestWithParameters_fake.arg1_val, (otMessage *)MSG_ADDR); - zassert_not_null(otCoapSendRequestWithParameters_fake.arg2_val); - zassert_not_null(otCoapSendRequestWithParameters_fake.arg3_val); + zexpect_equal(otCoapSendRequestWithParameters_fake.arg1_val, (otMessage *)MSG_ADDR); + zexpect_not_null(otCoapSendRequestWithParameters_fake.arg2_val); + zexpect_not_null(otCoapSendRequestWithParameters_fake.arg3_val); /* Test serialization of the response handler call */ mock_nrf_rpc_tr_expect_add(RPC_CMD(OT_RPC_CMD_COAP_RESPONSE_HANDLER, request_rep, @@ -498,13 +503,60 @@ ZTEST(ot_rpc_coap, test_otCoapSendRequest) mock_nrf_rpc_tr_expect_done(); } +/* + * Test reception of otCoapSendRequest(). + * Test serialization of the result: OT_ERROR_INVALID_STATE. + * Test that the message handle has not been released on failure. + */ +ZTEST(ot_rpc_coap, test_otCoapSendRequest_failed) +{ + ot_rpc_coap_request_key request_rep = 3; + ot_msg_key message_rep = ot_reg_msg_alloc((otMessage *)MSG_ADDR); + + otCoapSendRequestWithParameters_fake.return_val = OT_ERROR_INVALID_STATE; + + mock_nrf_rpc_tr_expect_add(RPC_RSP(OT_ERROR_INVALID_STATE), NO_RSP); + mock_nrf_rpc_tr_receive( + RPC_CMD(OT_RPC_CMD_COAP_SEND_REQUEST, message_rep, CBOR_MSG_INFO, request_rep)); + mock_nrf_rpc_tr_expect_done(); + + zassert_equal(otCoapSendRequestWithParameters_fake.call_count, 1); + zexpect_equal(otCoapSendRequestWithParameters_fake.arg1_val, (otMessage *)MSG_ADDR); + zexpect_not_null(otCoapSendRequestWithParameters_fake.arg2_val); + zexpect_not_null(otCoapSendRequestWithParameters_fake.arg3_val); + zassert_not_null(ot_msg_get(message_rep)); +} + /* * Test reception of otCoapSendResponse(). + * Test serialization of the result: OT_ERROR_NONE. + * Test that the response message handle has been released on success. */ ZTEST(ot_rpc_coap, test_otCoapSendResponse) { ot_msg_key message_rep = ot_reg_msg_alloc((otMessage *)MSG_ADDR); + otCoapSendResponseWithParameters_fake.return_val = OT_ERROR_NONE; + + mock_nrf_rpc_tr_expect_add(RPC_RSP(OT_ERROR_NONE), NO_RSP); + mock_nrf_rpc_tr_receive(RPC_CMD(OT_RPC_CMD_COAP_SEND_RESPONSE, message_rep, CBOR_MSG_INFO)); + mock_nrf_rpc_tr_expect_done(); + + zassert_equal(otCoapSendResponseWithParameters_fake.call_count, 1); + zexpect_equal(otCoapSendResponseWithParameters_fake.arg1_val, (otMessage *)MSG_ADDR); + zexpect_not_null(otCoapSendResponseWithParameters_fake.arg2_val); + zassert_is_null(ot_msg_get(message_rep)); +} + +/* + * Test reception of otCoapSendResponse(). + * Test serialization of the result: OT_ERROR_INVALID_STATE. + * Test that the response message handle hasn't been released on failure. + */ +ZTEST(ot_rpc_coap, test_otCoapSendResponse_failed) +{ + ot_msg_key message_rep = ot_reg_msg_alloc((otMessage *)MSG_ADDR); + otCoapSendResponseWithParameters_fake.return_val = OT_ERROR_INVALID_STATE; mock_nrf_rpc_tr_expect_add(RPC_RSP(OT_ERROR_INVALID_STATE), NO_RSP); @@ -512,8 +564,9 @@ ZTEST(ot_rpc_coap, test_otCoapSendResponse) mock_nrf_rpc_tr_expect_done(); zassert_equal(otCoapSendResponseWithParameters_fake.call_count, 1); - zassert_equal(otCoapSendResponseWithParameters_fake.arg1_val, (otMessage *)MSG_ADDR); - zassert_not_null(otCoapSendResponseWithParameters_fake.arg2_val); + zexpect_equal(otCoapSendResponseWithParameters_fake.arg1_val, (otMessage *)MSG_ADDR); + zexpect_not_null(otCoapSendResponseWithParameters_fake.arg2_val); + zassert_not_null(ot_msg_get(message_rep)); } ZTEST_SUITE(ot_rpc_coap, NULL, NULL, tc_setup, NULL, NULL);