net: openthread: rpc: allocate minimal buffer for read

Damian-Nordic · anangl · commit 966864b02a1e · 2025-08-25T15:15:25.000+02:00
otMessageRead() accepts the len argument, which can be
specified to a larger number than the actual size. In such
a case, OT RPC server would allocate needlessly large buffer
before calling otMessageRead().

Optimize the heap usage by checking otMessageGetLength()
before otMessageRead() and allocating only necessary space.

Also, make a few other enhancements for message APIs:
1. Significantly increase unit test coverage.
2. Make sure message table is always checked or modified
   with OT RPC mutex locked.
3. Use nrf_rpc_rsp_send_xxx() helpers.

Rework otMessageRead() implementation in OT RPC server to

Signed-off-by: Damian Krolik &lt;damian.krolik@nordicsemi.no&gt;
diff --git a/subsys/net/openthread/rpc/server/ot_rpc_message.c b/subsys/net/openthread/rpc/server/ot_rpc_message.c
@@ -24,33 +24,32 @@ OT_RPC_RESOURCE_TABLE_REGISTER(msg, otMessage, CONFIG_OPENTHREAD_RPC_MESSAGE_POO
 static void ot_rpc_msg_free(const struct nrf_rpc_group *group, struct nrf_rpc_cbor_ctx *ctx,
 			    void *handler_data)
 {
-	ot_rpc_res_tab_key key = 0;
+	ot_rpc_res_tab_key key;
 	otMessage *message;
 
 	key = nrf_rpc_decode_uint(ctx);
+
 	if (!nrf_rpc_decoding_done_and_check(group, ctx)) {
 		ot_rpc_report_cmd_decoding_error(OT_RPC_CMD_MESSAGE_FREE);
 		return;
 	}
 
+	ot_rpc_mutex_lock();
 	message = ot_res_tab_msg_get(key);
 
 	if (message != NULL) {
-		ot_rpc_mutex_lock();
 		otMessageFree(message);
-		ot_rpc_mutex_unlock();
+		ot_res_tab_msg_free(key);
 	}
 
-	ot_res_tab_msg_free(key);
-
+	ot_rpc_mutex_unlock();
 	nrf_rpc_rsp_send_void(group);
 }
 
 static void ot_rpc_msg_append(const struct nrf_rpc_group *group, struct nrf_rpc_cbor_ctx *ctx,
 			      void *handler_data)
 {
 	otError error = OT_ERROR_INVALID_ARGS;
-	struct nrf_rpc_cbor_ctx rsp_ctx;
 	uint32_t key;
 	const void *data;
 	size_t size = 0;
@@ -59,31 +58,29 @@ static void ot_rpc_msg_append(const struct nrf_rpc_group *group, struct nrf_rpc_
 	key = nrf_rpc_decode_uint(ctx);
 	data = nrf_rpc_decode_buffer_ptr_and_size(ctx, &size);
 
-	if (data && size && nrf_rpc_decode_valid(ctx)) {
+	if (data) {
+		ot_rpc_mutex_lock();
 		message = ot_res_tab_msg_get(key);
 
 		if (message != NULL) {
-			ot_rpc_mutex_lock();
 			error = otMessageAppend(message, data, size);
-			ot_rpc_mutex_unlock();
 		}
+
+		ot_rpc_mutex_unlock();
 	}
 
 	if (!nrf_rpc_decoding_done_and_check(group, ctx)) {
 		ot_rpc_report_cmd_decoding_error(OT_RPC_CMD_MESSAGE_APPEND);
 		return;
 	}
 
-	NRF_RPC_CBOR_ALLOC(group, rsp_ctx, sizeof(otError) + 1);
-	nrf_rpc_encode_uint(&rsp_ctx, error);
-	nrf_rpc_cbor_rsp_no_err(group, &rsp_ctx);
+	nrf_rpc_rsp_send_uint(group, error);
 }
 
 static void ot_rpc_msg_udp_new(const struct nrf_rpc_group *group, struct nrf_rpc_cbor_ctx *ctx,
 			       void *handler_data)
 {
 	ot_rpc_res_tab_key key;
-	struct nrf_rpc_cbor_ctx rsp_ctx;
 	otMessageSettings settings;
 	otMessageSettings *p_settings = &settings;
 
@@ -96,122 +93,114 @@ static void ot_rpc_msg_udp_new(const struct nrf_rpc_group *group, struct nrf_rpc
 
 	ot_rpc_mutex_lock();
 	key = ot_res_tab_msg_alloc(otUdpNewMessage(openthread_get_default_instance(), p_settings));
-	ot_rpc_mutex_unlock();
 
 	if (ot_res_tab_msg_get(key) == NULL) {
 		key = 0;
 	}
 
-	NRF_RPC_CBOR_ALLOC(group, rsp_ctx, sizeof(key) + 1);
-	nrf_rpc_encode_uint(&rsp_ctx, key);
-	nrf_rpc_cbor_rsp_no_err(group, &rsp_ctx);
+	ot_rpc_mutex_unlock();
+	nrf_rpc_rsp_send_uint(group, key);
 }
 
 static void ot_rpc_msg_length(const struct nrf_rpc_group *group, struct nrf_rpc_cbor_ctx *ctx,
 			      void *handler_data)
 {
 	ot_rpc_res_tab_key key;
-	struct nrf_rpc_cbor_ctx rsp_ctx;
 	uint16_t length = 0;
 	otMessage *message;
 
 	key = nrf_rpc_decode_uint(ctx);
+
 	if (!nrf_rpc_decoding_done_and_check(group, ctx)) {
 		ot_rpc_report_cmd_decoding_error(OT_RPC_CMD_MESSAGE_GET_LENGTH);
 		return;
 	}
 
+	ot_rpc_mutex_lock();
 	message = ot_res_tab_msg_get(key);
 
 	if (message != NULL) {
-		ot_rpc_mutex_lock();
 		length = otMessageGetLength(message);
-		ot_rpc_mutex_unlock();
 	}
 
-	NRF_RPC_CBOR_ALLOC(group, rsp_ctx, sizeof(length) + 1);
-	nrf_rpc_encode_uint(&rsp_ctx, length);
-	nrf_rpc_cbor_rsp_no_err(group, &rsp_ctx);
+	ot_rpc_mutex_unlock();
+	nrf_rpc_rsp_send_uint(group, length);
 }
 
 static void ot_rpc_get_offset(const struct nrf_rpc_group *group, struct nrf_rpc_cbor_ctx *ctx,
 			      void *handler_data)
 {
 	ot_rpc_res_tab_key key;
-	struct nrf_rpc_cbor_ctx rsp_ctx;
 	uint16_t offset = 0;
 	otMessage *message;
 
 	key = nrf_rpc_decode_uint(ctx);
+
 	if (!nrf_rpc_decoding_done_and_check(group, ctx)) {
 		ot_rpc_report_cmd_decoding_error(OT_RPC_CMD_MESSAGE_GET_OFFSET);
 		return;
 	}
 
+	ot_rpc_mutex_lock();
 	message = ot_res_tab_msg_get(key);
 
 	if (message != NULL) {
-		ot_rpc_mutex_lock();
 		offset = otMessageGetOffset(message);
-		ot_rpc_mutex_unlock();
 	}
 
-	NRF_RPC_CBOR_ALLOC(group, rsp_ctx, sizeof(offset) + 1);
-	nrf_rpc_encode_uint(&rsp_ctx, offset);
-	nrf_rpc_cbor_rsp_no_err(group, &rsp_ctx);
+	ot_rpc_mutex_unlock();
+	nrf_rpc_rsp_send_uint(group, offset);
 }
 
 static void ot_rpc_msg_read(const struct nrf_rpc_group *group, struct nrf_rpc_cbor_ctx *ctx,
 			    void *handler_data)
 {
+	ot_rpc_res_tab_key key;
 	uint16_t offset;
 	uint16_t length;
-	ot_rpc_res_tab_key key;
+	otMessage *message;
 	struct nrf_rpc_cbor_ctx rsp_ctx;
-	const uint16_t chunk_size = 64;
-	uint8_t buf[chunk_size];
+	uint16_t message_length;
 	uint16_t read = 0;
-	otMessage *message;
 
 	key = nrf_rpc_decode_uint(ctx);
 	offset = nrf_rpc_decode_uint(ctx);
 	length = nrf_rpc_decode_uint(ctx);
+
 	if (!nrf_rpc_decoding_done_and_check(group, ctx)) {
 		ot_rpc_report_cmd_decoding_error(OT_RPC_CMD_MESSAGE_READ);
 		return;
 	}
 
-	NRF_RPC_CBOR_ALLOC(group, rsp_ctx, length + 2);
-
+	ot_rpc_mutex_lock();
 	message = ot_res_tab_msg_get(key);
 
 	if (message == NULL) {
+		NRF_RPC_CBOR_ALLOC(group, rsp_ctx, 1);
 		nrf_rpc_encode_null(&rsp_ctx);
 		goto exit;
 	}
 
+	/* Get the actual message size before reading to allocate as small buffer as necessary. */
+	message_length = otMessageGetLength(message);
+	offset = MIN(offset, message_length);
+	length = MIN(length, message_length - offset);
+
+	NRF_RPC_CBOR_ALLOC(group, rsp_ctx, length + 3);
+
 	if (!zcbor_bstr_start_encode(rsp_ctx.zs)) {
 		goto exit;
 	}
 
-	ot_rpc_mutex_lock();
-
-	do {
-		read = otMessageRead(message, offset, buf,
-				     (chunk_size < length) ? chunk_size : length);
-		memcpy(rsp_ctx.zs[0].payload_mut, buf, read);
-		rsp_ctx.zs->payload_mut += read;
-		length -= read;
-		offset += read;
-	} while (read > 0 && length > 0);
-
-	ot_rpc_mutex_unlock();
+	read = otMessageRead(message, offset, rsp_ctx.zs->payload_mut, length);
+	rsp_ctx.zs->payload_mut += read;
 
 	if (!zcbor_bstr_end_encode(rsp_ctx.zs, NULL)) {
 		goto exit;
 	}
 
 exit:
+	ot_rpc_mutex_unlock();
 	nrf_rpc_cbor_rsp_no_err(group, &rsp_ctx);
 }
 
@@ -232,14 +221,15 @@ static void ot_rpc_msg_get_thread_link_info(const struct nrf_rpc_group *group,
 		return;
 	}
 
+	ot_rpc_mutex_lock();
 	message = ot_res_tab_msg_get(key);
 
 	if (!message) {
+		ot_rpc_mutex_unlock();
 		ot_rpc_report_cmd_decoding_error(OT_RPC_CMD_MESSAGE_GET_THREAD_LINK_INFO);
 		return;
 	}
 
-	ot_rpc_mutex_lock();
 	error = otMessageGetThreadLinkInfo(message, &link_info);
 	ot_rpc_mutex_unlock();
 
diff --git a/tests/subsys/net/openthread/rpc/common/test_rpc_env.h b/tests/subsys/net/openthread/rpc/common/test_rpc_env.h
@@ -20,9 +20,17 @@
 /* for value bigger than 0x17 */
 #define CBOR_UINT8(value)  0x18, (value)
 
-#define CBOR_LIST(...) 0x9F, __VA_ARGS__, 0xFF
-#define CBOR_EMPTY_BYTES 0x40
-#define CBOR_EMPTY_STRING 0x60
+#define CBOR_INT64(value) 0x3B, BT_BYTES_LIST_LE64(BSWAP_64(-1 - value))
+#define CBOR_INT32(value) 0x3A, BT_BYTES_LIST_LE32(BSWAP_32(-1 - value))
+#define CBOR_INT16(value) 0x39, BT_BYTES_LIST_LE16(BSWAP_16(-1 - value))
+/* for value smaller than -0x17 */
+#define CBOR_INT8(value)  0x38, (-1 - value)
+
+#define CBOR_LIST(...)	      0x9F __VA_OPT__(,) __VA_ARGS__, 0xFF
+#define CBOR_BSTR16(len, ...) (0x40 | 25), BT_BYTES_LIST_BE16(len) __VA_OPT__(,) __VA_ARGS__
+#define CBOR_BSTR8(len, ...)  (0x40 | 24), len __VA_OPT__(,) __VA_ARGS__
+#define CBOR_BSTR(len, ...)   (0x40 | len) __VA_OPT__(,) __VA_ARGS__
+#define CBOR_EMPTY_STRING     0x60
 
 /* Macros for constructing nRF RPC packets for the OpenThread command group. */
 
diff --git a/tests/subsys/net/openthread/rpc/server/src/coap_suite.c b/tests/subsys/net/openthread/rpc/server/src/coap_suite.c
@@ -53,11 +53,6 @@ FAKE_VALUE_FUNC(otError, otCoapSendResponseWithParameters, otInstance *, otMessa
 		const otMessageInfo *, const otCoapTxParameters *);
 
 #define FOREACH_FAKE(f)                                                                            \
-	f(otMessageGetLength);                                                                     \
-	f(otMessageGetOffset);                                                                     \
-	f(otMessageRead);                                                                          \
-	f(otMessageFree);                                                                          \
-	f(otMessageAppend);                                                                        \
 	f(otCoapNewMessage);                                                                       \
 	f(otCoapMessageInit);                                                                      \
 	f(otCoapMessageInitResponse);                                                              \
@@ -89,8 +84,11 @@ static void tc_setup(void *f)
 
 	FOREACH_FAKE(RESET_FAKE);
 	FFF_RESET_HISTORY();
+}
 
-	/* Unit tests take up to two message slots. */
+static void tc_cleanup(void *f)
+{
+	/* This suite uses two messages at most. */
 	ot_res_tab_msg_free(1);
 	ot_res_tab_msg_free(2);
 }
@@ -576,4 +574,4 @@ ZTEST(ot_rpc_coap, test_otCoapSendResponse_failed)
 	zassert_not_null(ot_res_tab_msg_get(message_rep));
 }
 
-ZTEST_SUITE(ot_rpc_coap, NULL, NULL, tc_setup, NULL, NULL);
+ZTEST_SUITE(ot_rpc_coap, NULL, NULL, tc_setup, tc_cleanup, NULL);
diff --git a/tests/subsys/net/openthread/rpc/server/src/common_fakes.c b/tests/subsys/net/openthread/rpc/server/src/common_fakes.c
@@ -14,3 +14,5 @@ DEFINE_FAKE_VOID_FUNC(otMessageFree, otMessage *);
 DEFINE_FAKE_VALUE_FUNC(otError, otMessageAppend, otMessage *, const void *, uint16_t);
 DEFINE_FAKE_VALUE_FUNC(otMessage *, otUdpNewMessage, otInstance *, const otMessageSettings *);
 DEFINE_FAKE_VALUE_FUNC(void *, nrf_rpc_cbkproxy_out_get, int, void *);
+DEFINE_FAKE_VOID_FUNC(otCliInit, otInstance *, otCliOutputCallback, void *);
+DEFINE_FAKE_VOID_FUNC(otCliInputLine, char *);
diff --git a/tests/subsys/net/openthread/rpc/server/src/common_fakes.h b/tests/subsys/net/openthread/rpc/server/src/common_fakes.h
@@ -6,7 +6,9 @@
 
 #ifndef __COMMON_FAKES_H__
 #define __COMMON_FAKES_H__
+
 #include <zephyr/fff.h>
+#include <openthread/cli.h>
 #include <openthread/message.h>
 
 DECLARE_FAKE_VALUE_FUNC(otMessage *, otUdpNewMessage, otInstance *, const otMessageSettings *);
@@ -17,5 +19,7 @@ DECLARE_FAKE_VALUE_FUNC(uint16_t, otMessageRead, const otMessage *, uint16_t, vo
 DECLARE_FAKE_VOID_FUNC(otMessageFree, otMessage *);
 DECLARE_FAKE_VALUE_FUNC(otError, otMessageAppend, otMessage *, const void *, uint16_t);
 DECLARE_FAKE_VALUE_FUNC(void *, nrf_rpc_cbkproxy_out_get, int, void *);
+DECLARE_FAKE_VOID_FUNC(otCliInit, otInstance *, otCliOutputCallback, void *);
+DECLARE_FAKE_VOID_FUNC(otCliInputLine, char *);
 
 #endif
diff --git a/tests/subsys/net/openthread/rpc/server/src/ip6_suite.c b/tests/subsys/net/openthread/rpc/server/src/ip6_suite.c
@@ -20,8 +20,6 @@
 /* Fake functions */
 
 DEFINE_FFF_GLOBALS;
-FAKE_VOID_FUNC(otCliInit, otInstance *, otCliOutputCallback, void *);
-FAKE_VOID_FUNC(otCliInputLine, char *);
 FAKE_VALUE_FUNC(const otNetifAddress *, otIp6GetUnicastAddresses, otInstance *);
 FAKE_VALUE_FUNC(const otNetifMulticastAddress *, otIp6GetMulticastAddresses, otInstance *);
 FAKE_VALUE_FUNC(otError, otIp6SubscribeMulticastAddress, otInstance *, const otIp6Address *);
@@ -30,8 +28,6 @@ FAKE_VALUE_FUNC(otError, otIp6SetEnabled, otInstance *, bool);
 FAKE_VALUE_FUNC(bool, otIp6IsEnabled, otInstance *);
 
 #define FOREACH_FAKE(f)                                                                            \
-	f(otCliInit);                                                                              \
-	f(otCliInputLine);                                                                         \
 	f(otIp6GetUnicastAddresses);                                                               \
 	f(otIp6GetMulticastAddresses);                                                             \
 	f(otIp6SubscribeMulticastAddress);                                                         \
@@ -233,6 +229,9 @@ ZTEST(ot_rpc_ip6, test_otIp6UnsubscribeMulticastAddress_failed)
 
 /*
  * Test reception of otIp6GetUnicastAddresses() command.
+ * Test serialization of the result:
+ * - fe80:aabb:aabb:aabb:aabb:aabb:aabb:aa01
+ * - fe80:aabb:aabb:aabb:aabb:aabb:aabb:aa02
  */
 ZTEST(ot_rpc_ip6, test_otIp6GetUnicastAddresses)
 {
@@ -278,6 +277,9 @@ ZTEST(ot_rpc_ip6, test_otIp6GetUnicastAddresses)
 
 /*
  * Test reception of otIp6GetMulticastAddresses() command.
+ * Test serialization of the result:
+ * - fe02::1
+ * - fe02::2
  */
 ZTEST(ot_rpc_ip6, test_otIp6GetMulticastAddresses)
 {
diff --git a/tests/subsys/net/openthread/rpc/server/src/message_suite.c b/tests/subsys/net/openthread/rpc/server/src/message_suite.c
diff --git a/tests/subsys/net/openthread/rpc/server/src/srp_client_suite.c b/tests/subsys/net/openthread/rpc/server/src/srp_client_suite.c
