SERVER-83161 Fix concurrent read to _errMsg from MigrationDestinationManager without acquiring mutex

Author: atesteve

src/mongo/db/s/migration_destination_manager.cpp

@@ -131,6 +131,7 @@
 #include "mongo/util/namespace_string_util.h"
 #include "mongo/util/out_of_line_executor.h"
 #include "mongo/util/producer_consumer_queue.h"
+#include "mongo/util/scopeguard.h"
 #include "mongo/util/str.h"
 #include "mongo/util/time_support.h"
 
@@ -1245,12 +1246,19 @@ void MigrationDestinationManager::_migrateDriver(OperationContext* outerOpCtx,
     invariant(!_min.isEmpty());
     invariant(!_max.isEmpty());
 
-    boost::optional<MoveTimingHelper> timing;
     boost::optional<Timer> timeInCriticalSection;
+    boost::optional<MoveTimingHelper> timing;
+    mongo::ScopeGuard timingSetMsgGuard{[this, &timing] {
+        // Set the error message to MoveTimingHelper just before it is destroyed. The destructor
+        // sends that message (among other things) to the ShardingLogging.
+        if (timing) {
+            stdx::lock_guard<Latch> sl(_mutex);
+            timing->setCmdErrMsg(_errmsg);
+        }
+    }};
 
     if (!skipToCritSecTaken) {
-        timing.emplace(
-            outerOpCtx, "to", _nss, _min, _max, 8 /* steps */, &_errmsg, _toShard, _fromShard);
+        timing.emplace(outerOpCtx, "to", _nss, _min, _max, 8 /* steps */, _toShard, _fromShard);
 
         LOGV2(22000,
               "Starting receiving end of chunk migration",

src/mongo/db/s/migration_source_manager.cpp

@@ -122,8 +122,6 @@ const WriteConcernOptions kMajorityWriteConcern(WriteConcernOptions::kMajority,
                                                 WriteConcernOptions::SyncMode::UNSET,
                                                 WriteConcernOptions::kWriteConcernTimeoutMigration);
 
-std::string kEmptyErrMsgForMoveTimingHelper;
-
 /*
  * Calculates the max or min bound perform split+move in case the chunk in question is splittable.
  * If the chunk is not splittable, returns the bound of the existing chunk for the max or min.Finds
@@ -211,7 +209,6 @@ MigrationSourceManager::MigrationSourceManager(OperationContext* opCtx,
                         _args.getMin(),
                         _args.getMax(),
                         6,  // Total number of steps
-                        &kEmptyErrMsgForMoveTimingHelper,
                         _args.getToShard(),
                         _args.getFromShard()) {
     invariant(!shard_role_details::getLocker(_opCtx)->isLocked());

src/mongo/db/s/move_timing_helper.cpp

@@ -57,7 +57,6 @@ MoveTimingHelper::MoveTimingHelper(OperationContext* opCtx,
                                    const boost::optional<BSONObj>& min,
                                    const boost::optional<BSONObj>& max,
                                    int totalNumSteps,
-                                   std::string* cmdErrmsg,
                                    const ShardId& toShard,
                                    const ShardId& fromShard)
     : _opCtx(opCtx),
@@ -68,7 +67,6 @@ MoveTimingHelper::MoveTimingHelper(OperationContext* opCtx,
       _min(min),
       _max(max),
       _totalNumSteps(totalNumSteps),
-      _cmdErrmsg(cmdErrmsg),
       _nextStep(0) {}
 
 MoveTimingHelper::~MoveTimingHelper() {
@@ -92,8 +90,8 @@ MoveTimingHelper::~MoveTimingHelper() {
             _b.append("note", "success");
         }
 
-        if (!_cmdErrmsg->empty()) {
-            _b.append("errmsg", *_cmdErrmsg);
+        if (!_cmdErrmsg.empty()) {
+            _b.append("errmsg", _cmdErrmsg);
         }
 
         ShardingLogging::get(_opCtx)->logChange(_opCtx,

src/mongo/db/s/move_timing_helper.h

@@ -29,6 +29,7 @@
 
 #pragma once
 
+#include "mongo/db/namespace_string.h"
 #include <boost/move/utility_core.hpp>
 #include <boost/optional/optional.hpp>
 #include <string>
@@ -52,7 +53,6 @@ class MoveTimingHelper {
                      const boost::optional<BSONObj>& min,
                      const boost::optional<BSONObj>& max,
                      int totalNumSteps,
-                     std::string* cmdErrmsg,
                      const ShardId& toShard,
                      const ShardId& fromShard);
     ~MoveTimingHelper();
@@ -65,6 +65,10 @@ class MoveTimingHelper {
         _max.emplace(max);
     }
 
+    void setCmdErrMsg(std::string cmdErrMsg) {
+        _cmdErrmsg = std::move(cmdErrMsg);
+    }
+
     void done(int step);
 
 private:
@@ -79,7 +83,7 @@ class MoveTimingHelper {
 
     boost::optional<BSONObj> _min, _max;
     const int _totalNumSteps;
-    const std::string* _cmdErrmsg;
+    std::string _cmdErrmsg;
 
     int _nextStep;
     BSONObjBuilder _b;