[GitHub] ccollins476ad commented on a change in pull request #8: smp: Refactor freeing request and response buffers

[GitBox <gi...@apache.org>]

ccollins476ad commented on a change in pull request #8: smp: Refactor freeing request and response buffers
URL: https://github.com/apache/mynewt-mcumgr/pull/8#discussion_r202767444
 
 

 ##########
 File path: smp/src/smp.c
 ##########
 @@ -322,58 +316,60 @@ smp_process_request_packet(struct smp_streamer *streamer, void *req)
 
     rsp = NULL;
     valid_hdr = true;
+    rc = 0;
 
-    while (1) {
+    while (rc == 0) {
         rc = mgmt_streamer_init_reader(&streamer->mgmt_stmr, req);
         if (rc != 0) {
             valid_hdr = false;
-            break;
+            goto error;
         }
 
         /* Read the management header and strip it from the request. */
         rc = smp_read_hdr(streamer, &req_hdr);
         if (rc != 0) {
             valid_hdr = false;
-            break;
+            goto error;
         }
         mgmt_ntoh_hdr(&req_hdr);
         mgmt_streamer_trim_front(&streamer->mgmt_stmr, req, MGMT_HDR_SIZE);
 
         rsp = mgmt_streamer_alloc_rsp(&streamer->mgmt_stmr, req);
         if (rsp == NULL) {
             rc = MGMT_ERR_ENOMEM;
-            break;
+            goto error;
         }
 
         rc = mgmt_streamer_init_writer(&streamer->mgmt_stmr, rsp);
         if (rc != 0) {
-            break;
+            goto error;
         }
 
         /* Process the request payload and build the response. */
         rc = smp_handle_single_req(streamer, &req_hdr);
         if (rc != 0) {
-            break;
+            goto error;
         }
 
         /* Send the response. */
         rc = streamer->tx_rsp_cb(streamer, rsp, streamer->mgmt_stmr.cb_arg);
-        rsp = NULL;
         if (rc != 0) {
-            break;
+            goto error;
         }
 
         /* Trim processed request to free up space for subsequent responses. */
         mgmt_streamer_trim_front(&streamer->mgmt_stmr, req,
                                  smp_align4(req_hdr.nh_len));
-    }
 
-    if (rc != 0 && valid_hdr) {
-        smp_on_err(streamer, &req_hdr, req, rsp, rc);
-        return rc;
+error:
+        if (rc != 0 && valid_hdr) {
+            smp_on_err(streamer, &req_hdr, req, rsp, rc);
+        }
+
+        mgmt_streamer_free_buf(&streamer->mgmt_stmr, rsp);
 
 Review comment:
   Won't this call to `mgmt_streamer_free_buf()` cause a double free?  The expectation is that `streamer->tx_rsp_cb()` consumes the response buffer (e.g., for Zephyr, this callback is `zephyr_smp_tx_rsp()`, which calls  `smp_bt_tx_pkt()`, which frees the buffer after use).

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services