Skip to content

Commit b1c9efb

Browse files
author
Tero Heinonen
authored
Fix coap_connection_handler_send_data() return values (#81)
Before, in some error cases coap_connection_handler_send_data() returned value -2 that caused data pointer to be saved. That might leak memory, if data pointer was already allocated.
1 parent f9cb04f commit b1c9efb

File tree

6 files changed

+66
-35
lines changed

6 files changed

+66
-35
lines changed

‎source/coap_connection_handler.c

Lines changed: 31 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,7 @@ static secure_session_t *secure_session_create(internal_socket_t *parent, const
193193
}
194194
timer_id++;
195195
}
196+
this->last_contact_time = coap_service_get_internal_timer_ticks();
196197
this->timer.id = timer_id;
197198
this->remote_host.type = ADDRESS_IPV6;
198199
memcpy(this->remote_host.address, address_ptr, 16);
@@ -884,50 +885,59 @@ int coap_connection_handler_send_data(coap_conn_handler_t *handler, const ns_add
884885
if (!handler || !handler->socket || !dest_addr) {
885886
return -1;
886887
}
888+
889+
/* Secure send */
887890
if (handler->socket->is_secure) {
888891
handler->socket->bypass_link_sec = bypass_link_sec;
889892
secure_session_t *session = secure_session_find(handler->socket, dest_addr->address, dest_addr->identifier);
890893
if (!session) {
891894
coap_security_keys_t security_material;
895+
int ret_val = 0;
896+
892897
memset(&security_material, 0, sizeof(coap_security_keys_t));
893898

894899
if (!handler->_get_password_cb || 0 != handler->_get_password_cb(handler->socket->socket, (uint8_t*)dest_addr->address, dest_addr->identifier, &security_material)) {
895900
return -1;
896901
}
897902

898903
session = secure_session_create(handler->socket, dest_addr->address, dest_addr->identifier, security_material.mode);
899-
if (!session) {
900-
ns_dyn_mem_free(security_material._key);
901-
return -1;
904+
if (!session || (0 > coap_security_handler_connect_non_blocking(session->sec_handler, false, DTLS, security_material, handler->socket->timeout_min, handler->socket->timeout_max))) {
905+
ret_val = -1;
902906
}
903-
session->last_contact_time = coap_service_get_internal_timer_ticks();
904907

905-
coap_security_handler_connect_non_blocking(session->sec_handler, false, DTLS, security_material, handler->socket->timeout_min, handler->socket->timeout_max);
906908
ns_dyn_mem_free(security_material._key);
907-
return -2;
909+
return ret_val;
908910

909911
} else if (session->session_state == SECURE_SESSION_OK) {
910-
if (coap_security_handler_send_message(session->sec_handler, data_ptr, data_len ) > 0 ) {
911-
session->last_contact_time = coap_service_get_internal_timer_ticks();
912-
return 0;
912+
session->last_contact_time = coap_service_get_internal_timer_ticks();
913+
if (0 > coap_security_handler_send_message(session->sec_handler, data_ptr, data_len )) {
914+
return -1;
913915
}
914916
}
915-
return -1;
916-
}else{
917+
/* Unsecure */
918+
} else {
919+
/* Virtual socket */
917920
if (!handler->socket->real_socket && handler->_send_cb) {
918-
return handler->_send_cb((int8_t)handler->socket->socket, dest_addr->address, dest_addr->identifier, data_ptr, data_len);
919-
}
920-
int opt_name = SOCKET_IPV6_PREFER_SRC_6LOWPAN_SHORT;
921-
int8_t securityLinkLayer = 1;
922-
if (bypass_link_sec) {
923-
securityLinkLayer = 0;
924-
}
921+
if (handler->_send_cb((int8_t)handler->socket->socket, dest_addr->address, dest_addr->identifier, data_ptr, data_len) < 0) {
922+
return -1;
923+
}
924+
} else {
925+
int opt_name = SOCKET_IPV6_PREFER_SRC_6LOWPAN_SHORT;
926+
int8_t securityLinkLayer = 1;
927+
if (bypass_link_sec) {
928+
securityLinkLayer = 0;
929+
}
925930

926-
socket_setsockopt(handler->socket->socket, SOCKET_IPPROTO_IPV6, SOCKET_IPV6_ADDR_PREFERENCES, &opt_name, sizeof(int));
927-
socket_setsockopt(handler->socket->socket, SOCKET_IPPROTO_IPV6, SOCKET_LINK_LAYER_SECURITY, &securityLinkLayer, sizeof(int8_t));
931+
socket_setsockopt(handler->socket->socket, SOCKET_IPPROTO_IPV6, SOCKET_IPV6_ADDR_PREFERENCES, &opt_name, sizeof(int));
932+
socket_setsockopt(handler->socket->socket, SOCKET_IPPROTO_IPV6, SOCKET_LINK_LAYER_SECURITY, &securityLinkLayer, sizeof(int8_t));
928933

929-
return send_to_real_socket(handler->socket->socket, dest_addr, src_address, data_ptr, data_len);
934+
if (0 > send_to_real_socket(handler->socket->socket, dest_addr, src_address, data_ptr, data_len)) {
935+
return -1;
936+
}
937+
}
930938
}
939+
940+
return 1;
931941
}
932942

933943
bool coap_connection_handler_socket_belongs_to(coap_conn_handler_t *handler, int8_t socket_id)

‎source/coap_message_handler.c

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,7 @@ int16_t coap_message_handler_coap_msg_process(coap_msg_handler_t *handle, int8_t
283283
}
284284
if (!this) {
285285
tr_error("client transaction not found");
286+
ret_val = -1;
286287
goto exit;
287288
}
288289
tr_debug("Service %d, response received", this->service_id);
@@ -421,7 +422,9 @@ int8_t coap_message_handler_response_send(coap_msg_handler_t *handle, int8_t ser
421422

422423
ret_val = coap_message_handler_resp_build_and_send(handle, response, transaction_ptr);
423424
sn_coap_parser_release_allocated_coap_msg_mem(handle->coap, response);
424-
transaction_delete(transaction_ptr);
425+
if(ret_val == 0) {
426+
transaction_delete(transaction_ptr);
427+
}
425428

426429
return ret_val;
427430
}
@@ -458,8 +461,9 @@ int8_t coap_message_handler_response_send_by_msg_id(coap_msg_handler_t *handle,
458461
}
459462

460463
ret_val = coap_message_handler_resp_build_and_send(handle, &response, transaction_ptr);
461-
462-
transaction_delete(transaction_ptr);
464+
if(ret_val == 0) {
465+
transaction_delete(transaction_ptr);
466+
}
463467

464468
return ret_val;
465469
}

‎source/coap_service_api.c

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@ static uint8_t coap_tx_function(uint8_t *data_ptr, uint16_t data_len, sn_nsdl_ad
146146
coap_service_t *this;
147147
coap_transaction_t *transaction_ptr = coap_message_handler_transaction_valid(param);
148148
ns_address_t dest_addr;
149+
int ret_val;
149150

150151
if (!transaction_ptr || !data_ptr) {
151152
return 0;
@@ -162,16 +163,19 @@ static uint8_t coap_tx_function(uint8_t *data_ptr, uint16_t data_len, sn_nsdl_ad
162163
dest_addr.identifier = address_ptr->port;
163164
dest_addr.type = ADDRESS_IPV6;
164165

165-
if (-2 == coap_connection_handler_send_data(this->conn_handler, &dest_addr, transaction_ptr->local_address,
166-
data_ptr, data_len, (this->service_options & COAP_SERVICE_OPTIONS_SECURE_BYPASS) == COAP_SERVICE_OPTIONS_SECURE_BYPASS)) {
167-
transaction_ptr->data_ptr = ns_dyn_mem_alloc(data_len);
166+
ret_val = coap_connection_handler_send_data(this->conn_handler, &dest_addr, transaction_ptr->local_address,
167+
data_ptr, data_len, (this->service_options & COAP_SERVICE_OPTIONS_SECURE_BYPASS) == COAP_SERVICE_OPTIONS_SECURE_BYPASS);
168+
if (ret_val == 0) {
168169
if (!transaction_ptr->data_ptr) {
169-
tr_debug("coap tx out of memory");
170-
return 0;
170+
transaction_ptr->data_ptr = ns_dyn_mem_alloc(data_len);
171+
if (!transaction_ptr->data_ptr) {
172+
tr_debug("coap tx out of memory");
173+
return 0;
174+
}
175+
memcpy(transaction_ptr->data_ptr, data_ptr, data_len);
176+
transaction_ptr->data_len = data_len;
171177
}
172-
memcpy(transaction_ptr->data_ptr, data_ptr, data_len);
173-
transaction_ptr->data_len = data_len;
174-
} else if (transaction_ptr->resp_cb == NULL ) {
178+
} else if ((ret_val == -1) || (transaction_ptr->resp_cb == NULL)) {
175179
transaction_delete(transaction_ptr);
176180
}
177181

‎source/include/coap_connection_handler.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,11 @@ void connection_handler_close_secure_connection( coap_conn_handler_t *handler, u
6565
int coap_connection_handler_open_connection(coap_conn_handler_t *handler, uint16_t listen_port, bool use_ephemeral_port, bool is_secure, bool real_socket, bool bypassSec);
6666

6767
//If returns -2, it means security was started and data was not send
68+
/*
69+
* \return > 0 in OK
70+
* \return 0 Session started, data not send
71+
* \return -1 failure
72+
*/
6873
int coap_connection_handler_send_data(coap_conn_handler_t *handler, const ns_address_t *dest_addr, const uint8_t src_address[static 16], uint8_t *data_ptr, uint16_t data_len, bool bypass_link_sec);
6974

7075
int coap_connection_handler_virtual_recv(coap_conn_handler_t *handler, uint8_t address[static 16], uint16_t port, uint8_t *data_ptr, uint16_t data_len);

‎test/coap-service/unittest/coap_connection_handler/test_coap_connection_handler.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -161,7 +161,7 @@ bool test_coap_connection_handler_send_data()
161161
return false;
162162

163163
socket_api_stub.int8_value = 7;
164-
if( 7 != coap_connection_handler_send_data(handler, &addr, ns_in6addr_any, NULL, 0, true))
164+
if( 1 != coap_connection_handler_send_data(handler, &addr, ns_in6addr_any, NULL, 0, true))
165165
return false;
166166
connection_handler_destroy(handler, false);
167167

‎test/coap-service/unittest/coap_message_handler/test_coap_message_handler.c

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,7 @@ bool test_coap_message_handler_coap_msg_process()
119119
{
120120
uint8_t buf[16];
121121
memset(&buf, 1, 16);
122+
/*Handler is null*/
122123
if( -1 != coap_message_handler_coap_msg_process(NULL, 0, buf, 22, ns_in6addr_any, NULL, 0, NULL))
123124
return false;
124125

@@ -128,12 +129,14 @@ bool test_coap_message_handler_coap_msg_process()
128129
coap_msg_handler_t *handle = coap_message_handler_init(&own_alloc, &own_free, &coap_tx_function);
129130

130131
sn_coap_protocol_stub.expectedHeader = NULL;
132+
/* Coap parse returns null */
131133
if( -1 != coap_message_handler_coap_msg_process(handle, 0, buf, 22, ns_in6addr_any, NULL, 0, process_cb))
132134
return false;
133135

134136
sn_coap_protocol_stub.expectedHeader = (sn_coap_hdr_s *)malloc(sizeof(sn_coap_hdr_s));
135137
memset(sn_coap_protocol_stub.expectedHeader, 0, sizeof(sn_coap_hdr_s));
136138
sn_coap_protocol_stub.expectedHeader->coap_status = 66;
139+
/* Coap library responds */
137140
if( -1 != coap_message_handler_coap_msg_process(handle, 0, buf, 22, ns_in6addr_any, NULL, 0, process_cb))
138141
return false;
139142

@@ -142,12 +145,17 @@ bool test_coap_message_handler_coap_msg_process()
142145
sn_coap_protocol_stub.expectedHeader->coap_status = COAP_STATUS_OK;
143146
sn_coap_protocol_stub.expectedHeader->msg_code = 1;
144147
retValue = 0;
148+
/* request received */
145149
if( 0 != coap_message_handler_coap_msg_process(handle, 0, buf, 22, ns_in6addr_any, NULL, 0, process_cb))
146150
return false;
147151

152+
sn_coap_protocol_stub.expectedHeader = (sn_coap_hdr_s *)malloc(sizeof(sn_coap_hdr_s));
153+
memset(sn_coap_protocol_stub.expectedHeader, 0, sizeof(sn_coap_hdr_s));
154+
sn_coap_protocol_stub.expectedHeader->coap_status = COAP_STATUS_OK;
155+
sn_coap_protocol_stub.expectedHeader->msg_code = 1;
148156
nsdynmemlib_stub.returnCounter = 1;
149157
retValue = -1;
150-
if( -1 != coap_message_handler_coap_msg_process(handle, 0, buf, 22, ns_in6addr_any, NULL, 0, process_cb))
158+
if( 0 != coap_message_handler_coap_msg_process(handle, 0, buf, 22, ns_in6addr_any, NULL, 0, process_cb))
151159
return false;
152160

153161
sn_coap_protocol_stub.expectedHeader = (sn_coap_hdr_s *)malloc(sizeof(sn_coap_hdr_s));
@@ -310,7 +318,7 @@ bool test_coap_message_handler_response_send()
310318
if( 0 != coap_message_handler_response_send(handle, 2, 0, header, 1,3,NULL, 0))
311319
return false;
312320

313-
// free(header);
321+
free(header);
314322
free(sn_coap_protocol_stub.expectedCoap);
315323
sn_coap_protocol_stub.expectedCoap = NULL;
316324
coap_message_handler_destroy(handle);

0 commit comments

Comments
 (0)