Fix coap_connection_handler_send_data() return values (ARMmbed#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.
artokin · Nov 6, 2017 · b1c9efb · b1c9efb
1 parent f9cb04f
commit b1c9efb
Show file tree

Hide file tree

Showing 6 changed files with 66 additions and 35 deletions.
diff --git a/source/coap_connection_handler.c b/source/coap_connection_handler.c
@@ -193,6 +193,7 @@ static secure_session_t *secure_session_create(internal_socket_t *parent, const
         }
         timer_id++;
     }
+    this->last_contact_time = coap_service_get_internal_timer_ticks();
     this->timer.id = timer_id;
     this->remote_host.type = ADDRESS_IPV6;
     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
     if (!handler || !handler->socket || !dest_addr) {
         return -1;
     }
+
+    /* Secure send */
     if (handler->socket->is_secure) {
         handler->socket->bypass_link_sec = bypass_link_sec;
         secure_session_t *session = secure_session_find(handler->socket, dest_addr->address, dest_addr->identifier);
         if (!session) {
             coap_security_keys_t security_material;
+            int ret_val = 0;
+
             memset(&security_material, 0, sizeof(coap_security_keys_t));
 
             if (!handler->_get_password_cb || 0 != handler->_get_password_cb(handler->socket->socket, (uint8_t*)dest_addr->address, dest_addr->identifier, &security_material)) {
                 return -1;
             }
 
             session = secure_session_create(handler->socket, dest_addr->address, dest_addr->identifier, security_material.mode);
-            if (!session) {
-                ns_dyn_mem_free(security_material._key);
-                return -1;
+            if (!session || (0 > coap_security_handler_connect_non_blocking(session->sec_handler, false, DTLS, security_material, handler->socket->timeout_min, handler->socket->timeout_max))) {
+                ret_val = -1;
             }
-            session->last_contact_time = coap_service_get_internal_timer_ticks();
 
-            coap_security_handler_connect_non_blocking(session->sec_handler, false, DTLS, security_material, handler->socket->timeout_min, handler->socket->timeout_max);
             ns_dyn_mem_free(security_material._key);
-            return -2;
+            return ret_val;
 
         } else if (session->session_state == SECURE_SESSION_OK) {
-            if (coap_security_handler_send_message(session->sec_handler, data_ptr, data_len ) > 0 ) {
-                session->last_contact_time = coap_service_get_internal_timer_ticks();
-                return 0;
+            session->last_contact_time = coap_service_get_internal_timer_ticks();
+            if (0 > coap_security_handler_send_message(session->sec_handler, data_ptr, data_len )) {
+                return -1;
             }
         }
-        return -1;
-    }else{
+    /* Unsecure */
+    } else {
+        /* Virtual socket */
         if (!handler->socket->real_socket && handler->_send_cb) {
-            return handler->_send_cb((int8_t)handler->socket->socket, dest_addr->address, dest_addr->identifier, data_ptr, data_len);
-        }
-        int opt_name = SOCKET_IPV6_PREFER_SRC_6LOWPAN_SHORT;
-        int8_t securityLinkLayer = 1;
-        if (bypass_link_sec) {
-            securityLinkLayer = 0;
-        }
+            if (handler->_send_cb((int8_t)handler->socket->socket, dest_addr->address, dest_addr->identifier, data_ptr, data_len) < 0) {
+                return -1;
+            }
+        } else {
+            int opt_name = SOCKET_IPV6_PREFER_SRC_6LOWPAN_SHORT;
+            int8_t securityLinkLayer = 1;
+            if (bypass_link_sec) {
+                securityLinkLayer = 0;
+            }
 
-        socket_setsockopt(handler->socket->socket, SOCKET_IPPROTO_IPV6, SOCKET_IPV6_ADDR_PREFERENCES, &opt_name, sizeof(int));
-        socket_setsockopt(handler->socket->socket, SOCKET_IPPROTO_IPV6, SOCKET_LINK_LAYER_SECURITY, &securityLinkLayer, sizeof(int8_t));
+            socket_setsockopt(handler->socket->socket, SOCKET_IPPROTO_IPV6, SOCKET_IPV6_ADDR_PREFERENCES, &opt_name, sizeof(int));
+            socket_setsockopt(handler->socket->socket, SOCKET_IPPROTO_IPV6, SOCKET_LINK_LAYER_SECURITY, &securityLinkLayer, sizeof(int8_t));
 
-        return send_to_real_socket(handler->socket->socket, dest_addr, src_address, data_ptr, data_len);
+            if (0 > send_to_real_socket(handler->socket->socket, dest_addr, src_address, data_ptr, data_len)) {
+                return -1;
+            }
+        }
     }
+
+    return 1;
 }
 
 bool coap_connection_handler_socket_belongs_to(coap_conn_handler_t *handler, int8_t socket_id)

diff --git a/source/coap_message_handler.c b/source/coap_message_handler.c
@@ -283,6 +283,7 @@ int16_t coap_message_handler_coap_msg_process(coap_msg_handler_t *handle, int8_t
         }
         if (!this) {
             tr_error("client transaction not found");
+            ret_val = -1;
             goto exit;
         }
         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
 
     ret_val =  coap_message_handler_resp_build_and_send(handle, response, transaction_ptr);
     sn_coap_parser_release_allocated_coap_msg_mem(handle->coap, response);
-    transaction_delete(transaction_ptr);
+    if(ret_val == 0) {
+        transaction_delete(transaction_ptr);
+    }
 
     return ret_val;
 }
@@ -458,8 +461,9 @@ int8_t coap_message_handler_response_send_by_msg_id(coap_msg_handler_t *handle,
     }
 
     ret_val = coap_message_handler_resp_build_and_send(handle, &response, transaction_ptr);
-
-    transaction_delete(transaction_ptr);
+    if(ret_val == 0) {
+        transaction_delete(transaction_ptr);
+    }
 
     return ret_val;
 }

diff --git a/source/coap_service_api.c b/source/coap_service_api.c
@@ -146,6 +146,7 @@ static uint8_t coap_tx_function(uint8_t *data_ptr, uint16_t data_len, sn_nsdl_ad
     coap_service_t *this;
     coap_transaction_t *transaction_ptr = coap_message_handler_transaction_valid(param);
     ns_address_t dest_addr;
+    int ret_val;
 
     if (!transaction_ptr || !data_ptr) {
         return 0;
@@ -162,16 +163,19 @@ static uint8_t coap_tx_function(uint8_t *data_ptr, uint16_t data_len, sn_nsdl_ad
     dest_addr.identifier = address_ptr->port;
     dest_addr.type = ADDRESS_IPV6;
 
-    if (-2 == coap_connection_handler_send_data(this->conn_handler, &dest_addr, transaction_ptr->local_address,
-            data_ptr, data_len, (this->service_options & COAP_SERVICE_OPTIONS_SECURE_BYPASS) == COAP_SERVICE_OPTIONS_SECURE_BYPASS)) {
-        transaction_ptr->data_ptr = ns_dyn_mem_alloc(data_len);
+    ret_val = coap_connection_handler_send_data(this->conn_handler, &dest_addr, transaction_ptr->local_address,
+            data_ptr, data_len, (this->service_options & COAP_SERVICE_OPTIONS_SECURE_BYPASS) == COAP_SERVICE_OPTIONS_SECURE_BYPASS);
+    if (ret_val == 0) {
         if (!transaction_ptr->data_ptr) {
-            tr_debug("coap tx out of memory");
-            return 0;
+            transaction_ptr->data_ptr = ns_dyn_mem_alloc(data_len);
+            if (!transaction_ptr->data_ptr) {
+                tr_debug("coap tx out of memory");
+                return 0;
+            }
+            memcpy(transaction_ptr->data_ptr, data_ptr, data_len);
+            transaction_ptr->data_len = data_len;
         }
-        memcpy(transaction_ptr->data_ptr, data_ptr, data_len);
-        transaction_ptr->data_len = data_len;
-    } else if (transaction_ptr->resp_cb == NULL ) {
+    } else if ((ret_val == -1) || (transaction_ptr->resp_cb == NULL)) {
         transaction_delete(transaction_ptr);
     }
 

diff --git a/source/include/coap_connection_handler.h b/source/include/coap_connection_handler.h
@@ -65,6 +65,11 @@ void connection_handler_close_secure_connection( coap_conn_handler_t *handler, u
 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);
 
 //If returns -2, it means security was started and data was not send
+/*
+ * \return > 0 in OK
+ * \return 0 Session started, data not send
+ * \return -1 failure
+ */
 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);
 
 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);

diff --git a/test/coap-service/unittest/coap_connection_handler/test_coap_connection_handler.c b/test/coap-service/unittest/coap_connection_handler/test_coap_connection_handler.c
@@ -161,7 +161,7 @@ bool test_coap_connection_handler_send_data()
         return false;
 
     socket_api_stub.int8_value = 7;
-    if( 7 != coap_connection_handler_send_data(handler, &addr, ns_in6addr_any, NULL, 0, true))
+    if( 1 != coap_connection_handler_send_data(handler, &addr, ns_in6addr_any, NULL, 0, true))
         return false;
     connection_handler_destroy(handler, false);
 

diff --git a/test/coap-service/unittest/coap_message_handler/test_coap_message_handler.c b/test/coap-service/unittest/coap_message_handler/test_coap_message_handler.c
@@ -119,6 +119,7 @@ bool test_coap_message_handler_coap_msg_process()
 {
     uint8_t buf[16];
     memset(&buf, 1, 16);
+    /*Handler is null*/
     if( -1 != coap_message_handler_coap_msg_process(NULL, 0, buf, 22, ns_in6addr_any, NULL, 0, NULL))
         return false;
 
@@ -128,12 +129,14 @@ bool test_coap_message_handler_coap_msg_process()
     coap_msg_handler_t *handle = coap_message_handler_init(&own_alloc, &own_free, &coap_tx_function);
 
     sn_coap_protocol_stub.expectedHeader = NULL;
+    /* Coap parse returns null */
     if( -1 != coap_message_handler_coap_msg_process(handle, 0, buf, 22, ns_in6addr_any, NULL, 0, process_cb))
         return false;
 
     sn_coap_protocol_stub.expectedHeader = (sn_coap_hdr_s *)malloc(sizeof(sn_coap_hdr_s));
     memset(sn_coap_protocol_stub.expectedHeader, 0, sizeof(sn_coap_hdr_s));
     sn_coap_protocol_stub.expectedHeader->coap_status = 66;
+    /* Coap library responds */
     if( -1 != coap_message_handler_coap_msg_process(handle, 0, buf, 22, ns_in6addr_any, NULL, 0, process_cb))
         return false;
 
@@ -142,12 +145,17 @@ bool test_coap_message_handler_coap_msg_process()
     sn_coap_protocol_stub.expectedHeader->coap_status = COAP_STATUS_OK;
     sn_coap_protocol_stub.expectedHeader->msg_code = 1;
     retValue = 0;
+    /* request received */
     if( 0 != coap_message_handler_coap_msg_process(handle, 0, buf, 22, ns_in6addr_any, NULL, 0, process_cb))
         return false;
 
+    sn_coap_protocol_stub.expectedHeader = (sn_coap_hdr_s *)malloc(sizeof(sn_coap_hdr_s));
+    memset(sn_coap_protocol_stub.expectedHeader, 0, sizeof(sn_coap_hdr_s));
+    sn_coap_protocol_stub.expectedHeader->coap_status = COAP_STATUS_OK;
+    sn_coap_protocol_stub.expectedHeader->msg_code = 1;
     nsdynmemlib_stub.returnCounter = 1;
     retValue = -1;
-    if( -1 != coap_message_handler_coap_msg_process(handle, 0, buf, 22, ns_in6addr_any, NULL, 0, process_cb))
+    if( 0 != coap_message_handler_coap_msg_process(handle, 0, buf, 22, ns_in6addr_any, NULL, 0, process_cb))
         return false;
 
     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()
     if( 0 != coap_message_handler_response_send(handle, 2, 0, header, 1,3,NULL, 0))
         return false;
 
-//    free(header);
+    free(header);
     free(sn_coap_protocol_stub.expectedCoap);
     sn_coap_protocol_stub.expectedCoap = NULL;
     coap_message_handler_destroy(handle);