SERVER-36663 Prevent killOperation from running without lock

11 files changed, 15 insertions, 14 deletions

diff --git a/src/mongo/db/commands/kill_op_cmd_base.cpp b/src/mongo/db/commands/kill_op_cmd_base.cpp
index 91824925137..2696beb53f6 100644
--- a/src/mongo/db/commands/kill_op_cmd_base.cpp
+++ b/src/mongo/db/commands/kill_op_cmd_base.cpp
@@ -116,7 +116,7 @@ void KillOpCmdBase::killLocalOperation(OperationContext* opCtx, unsigned int opT
     std::tie(lk, opCtxToKill) = std::move(*lockAndOpCtx);
 
     invariant(lk);
-    opCtx->getServiceContext()->killOperation(opCtxToKill);
+    opCtx->getServiceContext()->killOperation(lk, opCtxToKill);
 }
 
 unsigned int KillOpCmdBase::parseOpId(const BSONObj& cmdObj) {
diff --git a/src/mongo/db/cursor_manager.cpp b/src/mongo/db/cursor_manager.cpp
index 6c8fcf0c29a..853830e413c 100644
--- a/src/mongo/db/cursor_manager.cpp
+++ b/src/mongo/db/cursor_manager.cpp
@@ -617,7 +617,7 @@ Status CursorManager::killCursor(OperationContext* opCtx, CursorId id, bool shou
         {
             stdx::unique_lock<Client> lk(*cursor->_operationUsingCursor->getClient());
             cursor->_operationUsingCursor->getServiceContext()->killOperation(
-                cursor->_operationUsingCursor, ErrorCodes::CursorKilled);
+                lk, cursor->_operationUsingCursor, ErrorCodes::CursorKilled);
         }
 
         if (shouldAudit) {
diff --git a/src/mongo/db/kill_sessions_common.cpp b/src/mongo/db/kill_sessions_common.cpp
index a069db54fe5..086ca576faa 100644
--- a/src/mongo/db/kill_sessions_common.cpp
+++ b/src/mongo/db/kill_sessions_common.cpp
@@ -60,7 +60,7 @@ SessionKiller::Result killSessionsLocalKillOps(OperationContext* opCtx,
                     log() << "killing op: " << opCtxToKill->getOpID()
                           << " as part of killing session: " << lsid->toBSON();
 
-                    opCtx->getServiceContext()->killOperation(opCtxToKill);
+                    opCtx->getServiceContext()->killOperation(lk, opCtxToKill);
                 }
             }
         }
diff --git a/src/mongo/db/operation_context_group.cpp b/src/mongo/db/operation_context_group.cpp
index 5997852aea1..b3fe1f8e763 100644
--- a/src/mongo/db/operation_context_group.cpp
+++ b/src/mongo/db/operation_context_group.cpp
@@ -50,7 +50,7 @@ auto find(ContextTable& contexts, OperationContext* cp) {
 
 void interruptOne(OperationContext* opCtx, ErrorCodes::Error code) {
     stdx::lock_guard<Client> lk(*opCtx->getClient());
-    opCtx->getServiceContext()->killOperation(opCtx, code);
+    opCtx->getServiceContext()->killOperation(lk, opCtx, code);
 }
 
 }  // namespace
diff --git a/src/mongo/db/repl/replication_coordinator_impl.cpp b/src/mongo/db/repl/replication_coordinator_impl.cpp
index b168b7710d7..765f3ee6db0 100644
--- a/src/mongo/db/repl/replication_coordinator_impl.cpp
+++ b/src/mongo/db/repl/replication_coordinator_impl.cpp
@@ -1772,7 +1772,7 @@ void ReplicationCoordinatorImpl::_killUserOperationsOnStepDown(
                 GlobalLockAcquisitionTracker::get(toKill);
             if (closeConnectionsOnStepdown.load() || globalLockTracker.getGlobalWriteLocked() ||
                 globalLockTracker.getGlobalSharedLockTaken()) {
-                serviceCtx->killOperation(toKill, ErrorCodes::InterruptedDueToStepDown);
+                serviceCtx->killOperation(lk, toKill, ErrorCodes::InterruptedDueToStepDown);
             }
         }
     }
diff --git a/src/mongo/db/repl/replication_coordinator_impl_test.cpp b/src/mongo/db/repl/replication_coordinator_impl_test.cpp
index 351bf22dbe7..ca554d8c1b3 100644
--- a/src/mongo/db/repl/replication_coordinator_impl_test.cpp
+++ b/src/mongo/db/repl/replication_coordinator_impl_test.cpp
@@ -110,7 +110,7 @@ struct OpTimeWithTermOne {
  */
 void killOperation(OperationContext* opCtx) {
     stdx::lock_guard<Client> lkClient(*opCtx->getClient());
-    opCtx->getServiceContext()->killOperation(opCtx);
+    opCtx->getServiceContext()->killOperation(lkClient, opCtx);
 }
 
 TEST_F(ReplCoordTest, IsMasterIsFalseDuringStepdown) {
diff --git a/src/mongo/db/s/migration_destination_manager.cpp b/src/mongo/db/s/migration_destination_manager.cpp
index 13488bfd5b3..e5fd84ce884 100644
--- a/src/mongo/db/s/migration_destination_manager.cpp
+++ b/src/mongo/db/s/migration_destination_manager.cpp
@@ -402,7 +402,7 @@ void MigrationDestinationManager::cloneDocumentsFromDonor(
             }
         } catch (...) {
             stdx::lock_guard<Client> lk(*opCtx->getClient());
-            opCtx->getServiceContext()->killOperation(opCtx, ErrorCodes::Error(51008));
+            opCtx->getServiceContext()->killOperation(lk, opCtx, ErrorCodes::Error(51008));
             log() << "Batch insertion failed " << causedBy(redact(exceptionToStatus()));
         }
     }};
diff --git a/src/mongo/db/service_context.cpp b/src/mongo/db/service_context.cpp
index 47415f498d7..8ffa987a49d 100644
--- a/src/mongo/db/service_context.cpp
+++ b/src/mongo/db/service_context.cpp
@@ -287,7 +287,7 @@ void ServiceContext::setKillAllOperations() {
         stdx::lock_guard<Client> lk(*client);
         auto opCtxToKill = client->getOperationContext();
         if (opCtxToKill) {
-            killOperation(opCtxToKill, ErrorCodes::InterruptedAtShutdown);
+            killOperation(lk, opCtxToKill, ErrorCodes::InterruptedAtShutdown);
         }
     }
 
@@ -301,7 +301,7 @@ void ServiceContext::setKillAllOperations() {
     }
 }
 
-void ServiceContext::killOperation(OperationContext* opCtx, ErrorCodes::Error killCode) {
+void ServiceContext::killOperation(WithLock, OperationContext* opCtx, ErrorCodes::Error killCode) {
     opCtx->markKilled(killCode);
 
     for (const auto listener : _killOpListeners) {
diff --git a/src/mongo/db/service_context.h b/src/mongo/db/service_context.h
index e0014143da9..32091a06ca1 100644
--- a/src/mongo/db/service_context.h
+++ b/src/mongo/db/service_context.h
@@ -47,6 +47,7 @@
 #include "mongo/transport/service_executor.h"
 #include "mongo/transport/session.h"
 #include "mongo/util/clock_source.h"
+#include "mongo/util/concurrency/with_lock.h"
 #include "mongo/util/decorable.h"
 #include "mongo/util/periodic_runner.h"
 #include "mongo/util/tick_source.h"
@@ -355,10 +356,10 @@ public:
     /**
      * Kills the operation "opCtx" with the code "killCode", if opCtx has not already been killed.
      * Caller must own the lock on opCtx->getClient, and opCtx->getServiceContext() must be the same
-     *as
-     * this service context.
+     * as this service context. WithLock expects that the client lock be passed in.
      **/
-    void killOperation(OperationContext* opCtx,
+    void killOperation(WithLock,
+                       OperationContext* opCtx,
                        ErrorCodes::Error killCode = ErrorCodes::Interrupted);
 
     /**
diff --git a/src/mongo/db/session_catalog.cpp b/src/mongo/db/session_catalog.cpp
index 98b32ea6fe6..df565f6e43b 100644
--- a/src/mongo/db/session_catalog.cpp
+++ b/src/mongo/db/session_catalog.cpp
@@ -189,7 +189,7 @@ SessionCatalog::KillToken ObservableSession::kill(ErrorCodes::Error reason) cons
         stdx::lock_guard<Client> lg(*_session->_checkoutOpCtx->getClient());
 
         const auto serviceContext = _session->_checkoutOpCtx->getServiceContext();
-        serviceContext->killOperation(_session->_checkoutOpCtx, reason);
+        serviceContext->killOperation(lg, _session->_checkoutOpCtx, reason);
     }
 
     return SessionCatalog::KillToken(getSessionId());
diff --git a/src/mongo/s/query/cluster_cursor_manager.cpp b/src/mongo/s/query/cluster_cursor_manager.cpp
index 8ce0c769651..860327f6216 100644
--- a/src/mongo/s/query/cluster_cursor_manager.cpp
+++ b/src/mongo/s/query/cluster_cursor_manager.cpp
@@ -435,7 +435,7 @@ void ClusterCursorManager::killOperationUsingCursor(WithLock, CursorEntry* entry
     // Interrupt any operation currently using the cursor.
     OperationContext* opUsingCursor = entry->getOperationUsingCursor();
     stdx::lock_guard<Client> lk(*opUsingCursor->getClient());
-    opUsingCursor->getServiceContext()->killOperation(opUsingCursor, ErrorCodes::CursorKilled);
+    opUsingCursor->getServiceContext()->killOperation(lk, opUsingCursor, ErrorCodes::CursorKilled);
 
     // Don't delete the cursor, as an operation is using it. It will be cleaned up when the
     // operation is done.