Skip to content

Commit 8631afa

Browse files
Damian-NordicDamian-Nordic
committed
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>
1 parent 4bd5e19 commit 8631afa

File tree

3 files changed

+86
-37
lines changed

3 files changed

+86
-37
lines changed

‎subsys/net/openthread/rpc/server/ot_rpc_coap.c

+22-21
Original file line numberDiff line numberDiff line change
@@ -71,23 +71,18 @@ static void ot_rpc_cmd_coap_new_message(const struct nrf_rpc_group *group,
7171

7272
openthread_api_mutex_lock(openthread_get_default_context());
7373
message = otCoapNewMessage(openthread_get_default_instance(), settings);
74-
openthread_api_mutex_unlock(openthread_get_default_context());
75-
76-
if (!message) {
77-
goto out;
78-
}
79-
8074
message_rep = ot_reg_msg_alloc(message);
8175

82-
if (!message_rep) {
76+
if ((message != NULL) && !message_rep) {
8377
/*
84-
* If failed to allocate the message handle, the ownership can't be passed to the
85-
* RPC client. Therefore, free the message.
78+
* Failed to allocate the message handle, so the ownership can't be passed to
79+
* the RPC client. Therefore, free the message.
8680
*/
8781
otMessageFree(message);
8882
}
8983

90-
out:
84+
openthread_api_mutex_unlock(openthread_get_default_context());
85+
9186
nrf_rpc_rsp_send_uint(group, message_rep);
9287
}
9388

@@ -180,7 +175,7 @@ static void ot_rpc_cmd_coap_message_append_uri_path_options(const struct nrf_rpc
180175
message_rep = nrf_rpc_decode_uint(ctx);
181176
nrf_rpc_decode_str(ctx, uri, sizeof(uri));
182177

183-
if (!nrf_rpc_decode_valid(ctx)) {
178+
if (!nrf_rpc_decoding_done_and_check(group, ctx)) {
184179
ot_rpc_report_cmd_decoding_error(OT_RPC_CMD_COAP_MESSAGE_APPEND_URI_PATH_OPTIONS);
185180
return;
186181
}
@@ -194,7 +189,6 @@ static void ot_rpc_cmd_coap_message_append_uri_path_options(const struct nrf_rpc
194189

195190
openthread_api_mutex_lock(openthread_get_default_context());
196191
error = otCoapMessageAppendUriPathOptions(message, uri);
197-
nrf_rpc_cbor_decoding_done(group, ctx);
198192
openthread_api_mutex_unlock(openthread_get_default_context());
199193

200194
nrf_rpc_rsp_send_uint(group, error);
@@ -455,6 +449,8 @@ static void ot_rpc_coap_resource_handler(void *aContext, otMessage *aMessage,
455449
message_rep = ot_reg_msg_alloc(aMessage);
456450

457451
if (!message_rep) {
452+
nrf_rpc_err(-ENOMEM, NRF_RPC_ERR_SRC_SEND, &ot_group,
453+
OT_RPC_CMD_COAP_RESOURCE_HANDLER, NRF_RPC_PACKET_TYPE_CMD);
458454
return;
459455
}
460456

@@ -559,6 +555,8 @@ static void ot_rpc_coap_default_handler(void *aContext, otMessage *aMessage,
559555
message_rep = ot_reg_msg_alloc(aMessage);
560556

561557
if (!message_rep) {
558+
nrf_rpc_err(-ENOMEM, NRF_RPC_ERR_SRC_SEND, &ot_group,
559+
OT_RPC_CMD_COAP_DEFAULT_HANDLER, NRF_RPC_PACKET_TYPE_CMD);
562560
return;
563561
}
564562

@@ -606,13 +604,12 @@ static void ot_rpc_coap_response_handler(void *context, otMessage *message,
606604
struct nrf_rpc_cbor_ctx ctx;
607605
size_t cbor_buffer_size = 0;
608606

609-
if (message) {
610-
message_rep = ot_reg_msg_alloc(message);
611-
612-
if (!message_rep) {
613-
return;
614-
}
615-
}
607+
message_rep = ot_reg_msg_alloc(message);
608+
/*
609+
* Ignore message handle allocation failure. It seems safer to call the client's response
610+
* handler without the response (which indicates response timeout) than not to call the
611+
* handler at all and make the client wait for the response indefinitely.
612+
*/
616613

617614
cbor_buffer_size += 1 + sizeof(ot_rpc_coap_request_key);
618615
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,
657654
openthread_api_mutex_lock(openthread_get_default_context());
658655
error = otCoapSendRequest(openthread_get_default_instance(), message, &message_info,
659656
ot_rpc_coap_response_handler, (void *)request_rep);
660-
openthread_api_mutex_unlock(openthread_get_default_context());
661657

662658
if (error == OT_ERROR_NONE) {
663659
ot_msg_free(message_rep);
664660
}
665661

662+
openthread_api_mutex_unlock(openthread_get_default_context());
666663
nrf_rpc_rsp_send_uint(group, error);
667664
}
668665

@@ -694,8 +691,12 @@ static void ot_rpc_cmd_coap_send_response(const struct nrf_rpc_group *group,
694691

695692
openthread_api_mutex_lock(openthread_get_default_context());
696693
error = otCoapSendResponse(openthread_get_default_instance(), message, &message_info);
697-
openthread_api_mutex_unlock(openthread_get_default_context());
698694

695+
if (error == OT_ERROR_NONE) {
696+
ot_msg_free(message_rep);
697+
}
698+
699+
openthread_api_mutex_unlock(openthread_get_default_context());
699700
nrf_rpc_rsp_send_uint(group, error);
700701
}
701702

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

+5-10
Original file line numberDiff line numberDiff line change
@@ -37,25 +37,20 @@ ot_msg_key ot_reg_msg_alloc(otMessage *msg)
3737

3838
void ot_msg_free(ot_msg_key key)
3939
{
40-
key--;
41-
if (key >= OT_MESSAGES_POOL) {
40+
if (key == 0 || key > OT_MESSAGES_POOL) {
4241
return;
4342
}
4443

45-
if (ot_message_registry[key] != NULL) {
46-
ot_message_registry[key] = NULL;
47-
}
44+
ot_message_registry[key - 1] = NULL;
4845
}
4946

5047
otMessage *ot_msg_get(ot_msg_key key)
5148
{
52-
key--;
53-
54-
if (key < OT_MESSAGES_POOL) {
55-
return ot_message_registry[key];
49+
if (key == 0 || key > OT_MESSAGES_POOL) {
50+
return NULL;
5651
}
5752

58-
return NULL;
53+
return ot_message_registry[key - 1];
5954
}
6055

6156
static void ot_rpc_msg_free(const struct nrf_rpc_group *group, struct nrf_rpc_cbor_ctx *ctx,

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

+59-6
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ ZTEST(ot_rpc_coap, test_otCoapNewMessage_max)
125125
mock_nrf_rpc_tr_expect_done();
126126

127127
zassert_equal(otCoapNewMessage_fake.call_count, 1);
128+
zassert_not_null(ot_msg_get(1));
128129
}
129130

130131
/*
@@ -361,7 +362,7 @@ static void otCoapRemoveResource_custom_func(otInstance *instance, otCoapResourc
361362
ZTEST(ot_rpc_coap, test_otCoapAddResource_otCoapRemoveResource)
362363
{
363364
/*
364-
* Message key is allocated by the default handler encoder function but we know that
365+
* Message key is allocated by the resource handler encoder function but we know that
365366
* the first free slot will be selected so it will be 1.
366367
*/
367368
ot_msg_key message_rep = 1;
@@ -393,6 +394,7 @@ ZTEST(ot_rpc_coap, test_otCoapAddResource_otCoapRemoveResource)
393394
otCoapAddResource_fake.arg1_val->mHandler(otCoapAddResource_fake.arg1_val->mContext,
394395
(otMessage *)MSG_ADDR, &message_info);
395396
mock_nrf_rpc_tr_expect_done();
397+
zassert_is_null(ot_msg_get(message_rep));
396398

397399
/* Test reception of otCoapRemoveResource() */
398400
otCoapRemoveResource_fake.custom_fake = otCoapRemoveResource_custom_func;
@@ -440,6 +442,7 @@ ZTEST(ot_rpc_coap, test_otCoapSetDefaultHandler)
440442
otCoapSetDefaultHandler_fake.arg1_val(otCoapSetDefaultHandler_fake.arg2_val,
441443
(otMessage *)MSG_ADDR, &message_info);
442444
mock_nrf_rpc_tr_expect_done();
445+
zassert_is_null(ot_msg_get(message_rep));
443446

444447
/* Test reception of otCoapSetDefaultHandler() command that unsets the default handler */
445448
RESET_FAKE(otCoapSetDefaultHandler);
@@ -454,6 +457,8 @@ ZTEST(ot_rpc_coap, test_otCoapSetDefaultHandler)
454457

455458
/*
456459
* Test reception of otCoapSendRequest().
460+
* Test serialization of the result: OT_ERROR_NONE.
461+
* Test response handler invocation.
457462
*/
458463
ZTEST(ot_rpc_coap, test_otCoapSendRequest)
459464
{
@@ -484,9 +489,9 @@ ZTEST(ot_rpc_coap, test_otCoapSendRequest)
484489
mock_nrf_rpc_tr_expect_done();
485490

486491
zassert_equal(otCoapSendRequestWithParameters_fake.call_count, 1);
487-
zassert_equal(otCoapSendRequestWithParameters_fake.arg1_val, (otMessage *)MSG_ADDR);
488-
zassert_not_null(otCoapSendRequestWithParameters_fake.arg2_val);
489-
zassert_not_null(otCoapSendRequestWithParameters_fake.arg3_val);
492+
zexpect_equal(otCoapSendRequestWithParameters_fake.arg1_val, (otMessage *)MSG_ADDR);
493+
zexpect_not_null(otCoapSendRequestWithParameters_fake.arg2_val);
494+
zexpect_not_null(otCoapSendRequestWithParameters_fake.arg3_val);
490495

491496
/* Test serialization of the response handler call */
492497
mock_nrf_rpc_tr_expect_add(RPC_CMD(OT_RPC_CMD_COAP_RESPONSE_HANDLER, request_rep,
@@ -498,22 +503,70 @@ ZTEST(ot_rpc_coap, test_otCoapSendRequest)
498503
mock_nrf_rpc_tr_expect_done();
499504
}
500505

506+
/*
507+
* Test reception of otCoapSendRequest().
508+
* Test serialization of the result: OT_ERROR_INVALID_STATE.
509+
* Test that the message handle has not been released on failure.
510+
*/
511+
ZTEST(ot_rpc_coap, test_otCoapSendRequest_failed)
512+
{
513+
ot_rpc_coap_request_key request_rep = 3;
514+
ot_msg_key message_rep = ot_reg_msg_alloc((otMessage *)MSG_ADDR);
515+
516+
otCoapSendRequestWithParameters_fake.return_val = OT_ERROR_INVALID_STATE;
517+
518+
mock_nrf_rpc_tr_expect_add(RPC_RSP(OT_ERROR_INVALID_STATE), NO_RSP);
519+
mock_nrf_rpc_tr_receive(
520+
RPC_CMD(OT_RPC_CMD_COAP_SEND_REQUEST, message_rep, CBOR_MSG_INFO, request_rep));
521+
mock_nrf_rpc_tr_expect_done();
522+
523+
zassert_equal(otCoapSendRequestWithParameters_fake.call_count, 1);
524+
zexpect_equal(otCoapSendRequestWithParameters_fake.arg1_val, (otMessage *)MSG_ADDR);
525+
zexpect_not_null(otCoapSendRequestWithParameters_fake.arg2_val);
526+
zexpect_not_null(otCoapSendRequestWithParameters_fake.arg3_val);
527+
zassert_not_null(ot_msg_get(message_rep));
528+
}
529+
501530
/*
502531
* Test reception of otCoapSendResponse().
532+
* Test serialization of the result: OT_ERROR_NONE.
533+
* Test that the response message handle has been released on success.
503534
*/
504535
ZTEST(ot_rpc_coap, test_otCoapSendResponse)
505536
{
506537
ot_msg_key message_rep = ot_reg_msg_alloc((otMessage *)MSG_ADDR);
507538

539+
otCoapSendResponseWithParameters_fake.return_val = OT_ERROR_NONE;
540+
541+
mock_nrf_rpc_tr_expect_add(RPC_RSP(OT_ERROR_NONE), NO_RSP);
542+
mock_nrf_rpc_tr_receive(RPC_CMD(OT_RPC_CMD_COAP_SEND_RESPONSE, message_rep, CBOR_MSG_INFO));
543+
mock_nrf_rpc_tr_expect_done();
544+
545+
zassert_equal(otCoapSendResponseWithParameters_fake.call_count, 1);
546+
zexpect_equal(otCoapSendResponseWithParameters_fake.arg1_val, (otMessage *)MSG_ADDR);
547+
zexpect_not_null(otCoapSendResponseWithParameters_fake.arg2_val);
548+
zassert_is_null(ot_msg_get(message_rep));
549+
}
550+
551+
/*
552+
* Test reception of otCoapSendResponse().
553+
* Test serialization of the result: OT_ERROR_INVALID_STATE.
554+
* Test that the response message handle hasn't been released on failure.
555+
*/
556+
ZTEST(ot_rpc_coap, test_otCoapSendResponse_failed)
557+
{
558+
ot_msg_key message_rep = ot_reg_msg_alloc((otMessage *)MSG_ADDR);
559+
508560
otCoapSendResponseWithParameters_fake.return_val = OT_ERROR_INVALID_STATE;
509561

510562
mock_nrf_rpc_tr_expect_add(RPC_RSP(OT_ERROR_INVALID_STATE), NO_RSP);
511563
mock_nrf_rpc_tr_receive(RPC_CMD(OT_RPC_CMD_COAP_SEND_RESPONSE, message_rep, CBOR_MSG_INFO));
512564
mock_nrf_rpc_tr_expect_done();
513565

514566
zassert_equal(otCoapSendResponseWithParameters_fake.call_count, 1);
515-
zassert_equal(otCoapSendResponseWithParameters_fake.arg1_val, (otMessage *)MSG_ADDR);
516-
zassert_not_null(otCoapSendResponseWithParameters_fake.arg2_val);
567+
zexpect_equal(otCoapSendResponseWithParameters_fake.arg1_val, (otMessage *)MSG_ADDR);
568+
zexpect_not_null(otCoapSendResponseWithParameters_fake.arg2_val);
569+
zassert_not_null(ot_msg_get(message_rep));
517570
}
518571

519572
ZTEST_SUITE(ot_rpc_coap, NULL, NULL, tc_setup, NULL, NULL);

0 commit comments

Comments
 (0)