read_snapshot_obj: Pass user_ctx by reference to fix race condition (eBay#836)

There is a race condition where read_snapshot_obj may access a freed
user_ctx pointer due to concurrent free_user_snp_ctx calls. This occurs
because user_ctx was stored in the snapshot_obj struct as an
intermediate copy, making it vulnerable to being freed while still in
use.

Fix by passing user_ctx as a reference parameter, allowing the listener
to modify the caller's context directly without intermediate storage.

Signed-off-by: Jilong Kou <jkou@ebay.com>

Author: koujl

conanfile.py

@@ -9,7 +9,7 @@
 
 class HomestoreConan(ConanFile):
     name = "homestore"
-    version = "7.0.4"
+    version = "7.0.5"
 
     homepage = "https://github.com/eBay/Homestore"
     description = "HomeStore Storage Engine"

src/include/homestore/replication/repl_dev.h

@@ -99,11 +99,13 @@ class snapshot_context {
 };
 
 struct snapshot_obj {
-    void* user_ctx{nullptr};
+    void *&user_ctx;
     uint64_t offset{0};
     sisl::io_blob_safe blob;
     bool is_first_obj{false};
     bool is_last_obj{false};
+
+    snapshot_obj(void *&ctx) : user_ctx(ctx) {}
 };
 
 // HomeStore has some meta information to be transmitted during the baseline resync,
@@ -402,7 +404,7 @@ class ReplDevListener {
     /// times on the leader till all the data is transferred to the follower. is_last_obj in
     /// snapshot_obj will be true once all the data has been trasnferred. After this the raft on
     /// the follower side can do the incremental resync.
-    virtual int read_snapshot_obj(shared< snapshot_context > context, shared< snapshot_obj > snp_obj) = 0;
+    virtual int read_snapshot_obj(shared<snapshot_context> context, shared<snapshot_obj> snp_obj) = 0;
 
     /// @brief Called on the follower when the leader sends the data during the baseline resyc.
     /// is_last_obj in in snapshot_obj will be true once all the data has been transfered.

src/lib/replication/repl_dev/raft_state_machine.cpp

@@ -365,14 +365,12 @@ int RaftStateMachine::read_logical_snp_obj(nuraft::snapshot& s, void*& user_ctx,
         return 0;
     }
     auto snp_ctx = std::make_shared< nuraft_snapshot_context >(s);
-    auto snp_data = std::make_shared< snapshot_obj >();
-    snp_data->user_ctx = user_ctx;
+    auto snp_data = std::make_shared< snapshot_obj >(user_ctx);
     snp_data->offset = obj_id;
     snp_data->is_last_obj = is_last_obj;
 
-    // Listener will read the snapshot data and we pass through the same.
+    // Listener will read the snapshot data and modify user_ctx through the reference
     int ret = m_rd.m_listener->read_snapshot_obj(snp_ctx, snp_data);
-    user_ctx = snp_data->user_ctx; // Have to pass the user_ctx to NuRaft even if ret<0 to get it freed later
     if (ret < 0) return ret;
 
     is_last_obj = snp_data->is_last_obj;
@@ -395,7 +393,8 @@ void RaftStateMachine::save_logical_snp_obj(nuraft::snapshot& s, ulong& obj_id,
         return;
     }
     auto snp_ctx = std::make_shared< nuraft_snapshot_context >(s);
-    auto snp_data = std::make_shared< snapshot_obj >();
+    void* dummy_ctx = nullptr;
+    auto snp_data = std::make_shared< snapshot_obj >(dummy_ctx);
     snp_data->offset = obj_id;
     snp_data->is_first_obj = is_first_obj;
     snp_data->is_last_obj = is_last_obj;