[trafficserver] branch master updated: Traffic Dump: fix a use after free of a mutex (#10521)

[bneradt]

This is an automated email from the ASF dual-hosted git repository.

bneradt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git


The following commit(s) were added to refs/heads/master by this push:
     new 43c58a3415 Traffic Dump: fix a use after free of a mutex (#10521)
43c58a3415 is described below

commit 43c58a34150f2590e39a87c4f2db9dee5cc27e5e
Author: Brian Neradt <brian.ner...@gmail.com>
AuthorDate: Tue Sep 26 19:46:01 2023 -0500

    Traffic Dump: fix a use after free of a mutex (#10521)
    
    Coverity found that Traffic Dump had code that locked a mutex via
    std::lock_guard, freed the owner of the mutex (and thus the mutex), and
    then, when the std::lock_guard went out of scope, unlocked the free'd
    mutex. This use after free is addressed in this patch by extending the
    lifetime of the SessionData object owning the mutex until after the lock
    is released.
    
    Fixes: #10331
---
 plugins/traffic_dump/session_data.cc | 38 +++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/plugins/traffic_dump/session_data.cc 
b/plugins/traffic_dump/session_data.cc
index 335424cc4e..25d7b82979 100644
--- a/plugins/traffic_dump/session_data.cc
+++ b/plugins/traffic_dump/session_data.cc
@@ -414,25 +414,31 @@ SessionData::session_aio_handler(TSCont contp, TSEvent 
event, void *edata)
       Dbg(dbg_ctl, "session_aio_handler(): No valid ssnData. Abort.");
       return TS_ERROR;
     }
-    char *buf = TSAIOBufGet(cb);
-    const std::lock_guard<std::recursive_mutex> _(ssnData->disk_io_mutex);
+    char *buf         = TSAIOBufGet(cb);
+    bool free_ssnData = false;
+    {
+      const std::lock_guard<std::recursive_mutex> _(ssnData->disk_io_mutex);
 
-    // Free the allocated buffer and update aio_count
-    if (buf) {
-      TSfree(buf);
-      if (--ssnData->aio_count == 0 && ssnData->ssn_closed) {
-        // check for ssn close, if closed, do clean up
-        TSContDataSet(contp, nullptr);
-        close(ssnData->log_fd);
-        std::error_code ec;
-        swoc::file::file_status st = swoc::file::status(ssnData->log_name, ec);
-        if (!ec) {
-          disk_usage += swoc::file::file_size(st);
-          Dbg(dbg_ctl, "Finish a session with log file of %" PRIuMAX " bytes", 
swoc::file::file_size(st));
+      // Free the allocated buffer and update aio_count
+      if (buf) {
+        TSfree(buf);
+        if (--ssnData->aio_count == 0 && ssnData->ssn_closed) {
+          // check for ssn close, if closed, do clean up
+          TSContDataSet(contp, nullptr);
+          close(ssnData->log_fd);
+          std::error_code ec;
+          swoc::file::file_status st = swoc::file::status(ssnData->log_name, 
ec);
+          if (!ec) {
+            disk_usage += swoc::file::file_size(st);
+            Dbg(dbg_ctl, "Finish a session with log file of %" PRIuMAX " 
bytes", swoc::file::file_size(st));
+          }
+          // We have to free ssnData outside of holding the lock whose 
lifetime is managed by ssnData.
+          free_ssnData = true;
         }
-        delete ssnData;
-        return TS_SUCCESS;
       }
+    } // Release ssnData->disk_io_mutex.
+    if (free_ssnData) {
+      delete ssnData;
     }
     return TS_SUCCESS;
   }