MB-43028: [1/2] Make overhead tracking safe at VBucket destruction

Cherry-picks http://review.couchbase.org/c/kv_engine/+/141492 Note: This patch is cherry-picked as the issue it resolves prevents clean forward-merging of a change _earlier_ in mad-hatter history. Merging http://review.couchbase.org/c/kv_engine/+/136495 into master uncovered santizer issues (mad-hatter CV runs an older Clang and did not identify these issues). This patch resolves one of these issues, before the above patch may be merged to master. WARNING: ThreadSanitizer: heap-use-after-free (pid=73551) Read of size 8 at 0x7b5800000310 by main thread: #0 operator() ../kv_engine/engines/ep/src/ephemeral_bucket.cc:301 (libep.so+0x00000034705f) #1 std::_Function_handler<void (long), EphemeralBucket::makeVBucket(...>::_M_invoke(std::_Any_data const&, long&&) #3 ~Checkpoint ../kv_engine/engines/ep/src/checkpoint.cc:228 (libep.so+0x00000019dc97) #10 ~CheckpointManager ../kv_engine/engines/ep/src/checkpoint_manager.h:73 (libep.so+0x00000042f782) ... #13 ~VBucket ../kv_engine/engines/ep/src/vbucket.cc:286 (libep.so+0x00000041689a) During VBucket destruction, the CheckpointManager member, and any remaining Checkpoints it holds are destroyed. This can trigger the memOverheadChangedCallback, as destroying Checkpoints reduces the memory overhead. This was unsafe, as the state member had already been destroyed as it is declared later in VBucket. Move the CheckpointManager to be declared after state, so it is destroyed first. Change-Id: I0a34d3a11cd053f18f3168d6fbbb9dc974bbd2fc Reviewed-on: http://review.couchbase.org/c/kv_engine/+/141492 Reviewed-by: Dave Rigby <[email protected]> Tested-by: Build Bot <[email protected]> Well-Formed: Build Bot <[email protected]> Reviewed-on: http://review.couchbase.org/c/kv_engine/+/141498
couchbase · Dec 4, 2020 · e96a9f5 · e96a9f5
1 parent 02c3f75
commit e96a9f5
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 11 deletions.
diff --git a/engines/ep/src/vbucket.cc b/engines/ep/src/vbucket.cc
@@ -190,14 +190,6 @@ VBucket::VBucket(Vbid i,
                  const nlohmann::json* replTopology,
                  uint64_t maxVisibleSeqno)
     : ht(st, std::move(valFact), config.getHtSize(), config.getHtLocks()),
-      checkpointManager(std::make_unique<CheckpointManager>(st,
-                                                            i,
-                                                            chkConfig,
-                                                            lastSeqno,
-                                                            lastSnapStart,
-                                                            lastSnapEnd,
-                                                            maxVisibleSeqno,
-                                                            flusherCb)),
       failovers(std::move(table)),
       opsCreate(0),
       opsDelete(0),
@@ -220,6 +212,14 @@ VBucket::VBucket(Vbid i,
       id(i),
       state(newState),
       initialState(initState),
+      checkpointManager(std::make_unique<CheckpointManager>(st,
+                                                            i,
+                                                            chkConfig,
+                                                            lastSeqno,
+                                                            lastSnapStart,
+                                                            lastSnapEnd,
+                                                            maxVisibleSeqno,
+                                                            flusherCb)),
       replicationTopology(std::make_unique<nlohmann::json>()),
       purge_seqno(purgeSeqno),
       takeover_backed_up(false),

diff --git a/engines/ep/src/vbucket.h b/engines/ep/src/vbucket.h
@@ -837,9 +837,6 @@ class VBucket : public std::enable_shared_from_this<VBucket> {
 
     HashTable ht;
 
-    /// Manager of this vBucket's checkpoints. unique_ptr for pimpl.
-    std::unique_ptr<CheckpointManager> checkpointManager;
-
     /**
      * Searches for a 'valid' StoredValue in the VBucket.
      *
@@ -2442,6 +2439,15 @@ class VBucket : public std::enable_shared_from_this<VBucket> {
 
     vbucket_state_t                 initialState;
 
+public:
+    /**
+     * Manager of this vBucket's checkpoints. unique_ptr for pimpl.
+     * Declared after state as Checkpoint destruction may update stats
+     * based on the vbucket's current state.
+     */
+    std::unique_ptr<CheckpointManager> checkpointManager;
+
+private:
     /**
      * The replication topology, set as part of SET_VBUCKET_STATE.
      * It is encoded as nlohmann::json array of (max 2) replication chains.