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 <damian.krolik@nordicsemi.no>

Author: Damian-Nordic

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);
 }
 

subsys/net/openthread/rpc/server/ot_rpc_message.c

@@ -37,11 +37,12 @@ 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;
 	}
 
+	key--;
+
 	if (ot_message_registry[key] != NULL) {
 		ot_message_registry[key] = NULL;
 	}

tests/subsys/net/openthread/rpc/server/src/coap_suite.c

@@ -454,6 +454,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 +486,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,22 +500,70 @@ 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);
 	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);
-	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);