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

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

MariuszSkamra 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_r202782955
 
 

 ##########
 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:
   In fact the current implementation causes double free.
   If the `streamer->tx_rsp_cb()` consumes the response buffer, then why there is double call to mgmt_streamer_free_buf to free rsp and req buffers in smp_process_request_packet && smp_on_err? ;) In smp_on_err even NULL is assigned to req and rsp still calling mgmt_streamer_free_buf with this NULL parameter two times.
   
   I know this patch is not perfect because it changes the concept by `streamer->tx_rsp_cb()` not consuming the buffer, so if you want to fix this in another way, please do or suggest me.
   
   I thought that it might be cleaner if function that allocates the buffer frees it, I think that it's good to have this in place since this is not placed on any FIFO or sth, but freed in the same function context.

----------------------------------------------------------------
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