diff options
author | Romans Kasperovics <romans.kasperovics@mongodb.com> | 2022-03-15 08:55:37 +0000 |
---|---|---|
committer | Evergreen Agent <no-reply@evergreen.mongodb.com> | 2022-03-15 09:33:32 +0000 |
commit | 80421c5b8e5ac71e16dc005fd961901884891c47 (patch) | |
tree | f10f3369dd60df12bea2c4e7060793061d4c7486 /src/mongo | |
parent | abe5428751586d14241f94f06261c7037690557f (diff) | |
download | mongo-80421c5b8e5ac71e16dc005fd961901884891c47.tar.gz |
SERVER-62710 Ensure that AsyncResultsMerger attempts to kill shard cursors when maxTimeMs is exceeded
Diffstat (limited to 'src/mongo')
-rw-r--r-- | src/mongo/db/commands/getmore_cmd.cpp | 117 | ||||
-rw-r--r-- | src/mongo/s/query/async_results_merger.cpp | 5 | ||||
-rw-r--r-- | src/mongo/s/query/async_results_merger_test.cpp | 13 |
3 files changed, 82 insertions, 53 deletions
diff --git a/src/mongo/db/commands/getmore_cmd.cpp b/src/mongo/db/commands/getmore_cmd.cpp index d78bc4e2434..9fe8b68b918 100644 --- a/src/mongo/db/commands/getmore_cmd.cpp +++ b/src/mongo/db/commands/getmore_cmd.cpp @@ -144,6 +144,57 @@ void validateTxnNumber(OperationContext* opCtx, int64_t cursorId, const ClientCu } /** + * Validate that the client has necessary privileges to call getMore on the given cursor. + */ +void validateAuthorization(const OperationContext* opCtx, const ClientCursor& cursor) { + + auto authzSession = AuthorizationSession::get(opCtx->getClient()); + // A user can only call getMore on their own cursor. If there were multiple users + // authenticated when the cursor was created, then at least one of them must be + // authenticated in order to run getMore on the cursor. + if (!authzSession->isCoauthorizedWith(cursor.getAuthenticatedUsers())) { + uasserted(ErrorCodes::Unauthorized, + str::stream() << "cursor id " << cursor.cursorid() + << " was not created by the authenticated user"); + } + + // Ensure that the client still has the privileges to run the originating command. + if (!authzSession->isAuthorizedForPrivileges(cursor.getOriginatingPrivileges())) { + uasserted(ErrorCodes::Unauthorized, + str::stream() << "not authorized for getMore with cursor id " + << cursor.cursorid()); + } +} + +/** + * Validate that the command's and cursor's namespaces match. + */ +void validateNamespace(const NamespaceString& commandNss, const ClientCursor& cursor) { + uassert(ErrorCodes::Unauthorized, + str::stream() << "Requested getMore on namespace '" << commandNss.ns() + << "', but cursor belongs to a different namespace " << cursor.nss().ns(), + commandNss == cursor.nss()); + + if (commandNss.isOplog() && MONGO_unlikely(rsStopGetMoreCmd.shouldFail())) { + uasserted(ErrorCodes::CommandFailed, + str::stream() << "getMore on " << commandNss.ns() + << " rejected due to active fail point rsStopGetMoreCmd"); + } +} + +/** + * Validate that the command's maxTimeMS is only set when the cursor is in awaitData mode. + */ +void validateMaxTimeMS(const boost::optional<std::int64_t>& commandMaxTimeMS, + const ClientCursor& cursor) { + if (commandMaxTimeMS.has_value()) { + uassert(ErrorCodes::BadValue, + "cannot set maxTimeMS on getMore command for a non-awaitData cursor", + cursor.isAwaitData()); + } +} + +/** * Apply the read concern from the cursor to this operation. */ void applyCursorReadConcern(OperationContext* opCtx, repl::ReadConcernArgs rcArgs) { @@ -403,7 +454,6 @@ public: void acquireLocksAndIterateCursor(OperationContext* opCtx, rpc::ReplyBuilderInterface* reply, - CursorManager* cursorManager, ClientCursorPin& cursorPin, CurOp* curOp) { // Cursors come in one of two flavors: @@ -427,7 +477,6 @@ public: boost::optional<AutoGetCollectionForReadMaybeLockFree> readLock; boost::optional<AutoStatsTracker> statsTracker; NamespaceString nss(_cmd.getDbName(), _cmd.getCollection()); - int64_t cursorId = _cmd.getCommandParameter(); const bool disableAwaitDataFailpointActive = MONGO_unlikely(disableAwaitDataForGetMoreCmd.shouldFail()); @@ -436,6 +485,9 @@ public: setUpOperationContextStateForGetMore( opCtx, *cursorPin.getCursor(), _cmd, disableAwaitDataFailpointActive); + // On early return, typically due to a failed assertion, delete the cursor. + ScopeGuard cursorDeleter([&] { cursorPin.deleteUnderlying(); }); + if (cursorPin->getExecutor()->lockPolicy() == PlanExecutor::LockPolicy::kLocksInternally) { if (!nss.isCollectionlessCursorNamespace()) { @@ -481,49 +533,6 @@ public: opCtx, nss, true)); } - // A user can only call getMore on their own cursor. If there were multiple users - // authenticated when the cursor was created, then at least one of them must be - // authenticated in order to run getMore on the cursor. - auto authzSession = AuthorizationSession::get(opCtx->getClient()); - if (!authzSession->isCoauthorizedWith(cursorPin->getAuthenticatedUsers())) { - uasserted(ErrorCodes::Unauthorized, - str::stream() << "cursor id " << cursorId - << " was not created by the authenticated user"); - } - - // Ensure that the client still has the privileges to run the originating command. - if (!authzSession->isAuthorizedForPrivileges(cursorPin->getOriginatingPrivileges())) { - uasserted(ErrorCodes::Unauthorized, - str::stream() - << "not authorized for getMore with cursor id " << cursorId); - } - - if (nss != cursorPin->nss()) { - uasserted(ErrorCodes::Unauthorized, - str::stream() << "Requested getMore on namespace '" << nss.ns() - << "', but cursor belongs to a different namespace " - << cursorPin->nss().ns()); - } - - if (nss.isOplog() && MONGO_unlikely(rsStopGetMoreCmd.shouldFail())) { - uasserted(ErrorCodes::CommandFailed, - str::stream() << "getMore on " << nss.ns() - << " rejected due to active fail point rsStopGetMoreCmd"); - } - - // Validation related to awaitData. - if (cursorPin->isAwaitData()) { - invariant(cursorPin->isTailable()); - } - - if (_cmd.getMaxTimeMS() && !cursorPin->isAwaitData()) { - uasserted(ErrorCodes::BadValue, - "cannot set maxTimeMS on getMore command for a non-awaitData cursor"); - } - - // On early return, get rid of the cursor. - ScopeGuard cursorFreer([&] { cursorPin.deleteUnderlying(); }); - // If the 'waitAfterPinningCursorBeforeGetMoreBatch' fail point is enabled, set the // 'msg' field of this operation's CurOp to signal that we've hit this point and then // repeatedly release and re-acquire the collection readLock at regular intervals until @@ -683,7 +692,7 @@ public: } if (shouldSaveCursor) { - respondWithId = cursorId; + respondWithId = cursorPin->cursorid(); exec->saveState(); exec->detachFromOperationContext(); @@ -713,7 +722,7 @@ public: curOp->debug().nreturned = numResults; if (respondWithId) { - cursorFreer.dismiss(); + cursorDeleter.dismiss(); if (opCtx->isExhaust()) { // Indicate that an exhaust message should be generated and the previous BSONObj @@ -751,21 +760,23 @@ public: opCtx->lockState()->skipAcquireTicket(); } - auto cursorManager = CursorManager::get(opCtx); - auto pinCheck = [opCtx, cursorId](const ClientCursor& cc) { - // Ensure the lsid and txnNumber of the getMore match that of the - // originating command. + // Perform validation checks which don't cause the cursor to be deleted on failure. + auto pinCheck = [&](const ClientCursor& cc) { validateLSID(opCtx, cursorId, &cc); validateTxnNumber(opCtx, cursorId, &cc); + validateAuthorization(opCtx, cc); + validateNamespace(nss, cc); + validateMaxTimeMS(_cmd.getMaxTimeMS(), cc); }; - auto cursorPin = uassertStatusOK(cursorManager->pinCursor(opCtx, cursorId, pinCheck)); + auto cursorPin = + uassertStatusOK(CursorManager::get(opCtx)->pinCursor(opCtx, cursorId, pinCheck)); // Get the read concern level here in case the cursor is exhausted while iterating. const auto isLinearizableReadConcern = cursorPin->getReadConcernArgs().getLevel() == repl::ReadConcernLevel::kLinearizableReadConcern; - acquireLocksAndIterateCursor(opCtx, reply, cursorManager, cursorPin, curOp); + acquireLocksAndIterateCursor(opCtx, reply, cursorPin, curOp); if (MONGO_unlikely(getMoreHangAfterPinCursor.shouldFail())) { LOGV2(20477, diff --git a/src/mongo/s/query/async_results_merger.cpp b/src/mongo/s/query/async_results_merger.cpp index 56f2deff96e..363e151fbfd 100644 --- a/src/mongo/s/query/async_results_merger.cpp +++ b/src/mongo/s/query/async_results_merger.cpp @@ -852,6 +852,11 @@ void AsyncResultsMerger::_scheduleKillCursors(WithLock, OperationContext* opCtx) executor::RemoteCommandRequest request( remote.getTargetHost(), _params.getNss().db().toString(), cmdObj, opCtx); + // The 'RemoteCommandRequest' takes the remaining time from the 'opCtx' parameter. If + // the cursor was killed due to a maxTimeMs timeout, the remaining time will be 0, and + // the remote request will not be sent. To avoid this, we remove the timeout for the + // remote 'killCursor' command. + request.timeout = executor::RemoteCommandRequestBase::kNoTimeout; // Send kill request; discard callback handle, if any, or failure report, if not. _executor->scheduleRemoteCommand(request, [](auto const&) {}).getStatus().ignore(); diff --git a/src/mongo/s/query/async_results_merger_test.cpp b/src/mongo/s/query/async_results_merger_test.cpp index 6caad31478e..d089ac691c9 100644 --- a/src/mongo/s/query/async_results_merger_test.cpp +++ b/src/mongo/s/query/async_results_merger_test.cpp @@ -961,6 +961,19 @@ TEST_F(AsyncResultsMergerTest, KillCalledTwice) { killFuture2.wait(); } +TEST_F(AsyncResultsMergerTest, KillCursorCmdHasNoTimeout) { + std::vector<RemoteCursor> cursors; + cursors.push_back( + makeRemoteCursor(kTestShardIds[0], kTestShardHosts[0], CursorResponse(kTestNss, 1, {}))); + auto arm = makeARMFromExistingCursors(std::move(cursors)); + + auto* opCtx = operationContext(); + opCtx->setDeadlineAfterNowBy(Microseconds::zero(), ErrorCodes::MaxTimeMSExpired); + auto killFuture = arm->kill(opCtx); + ASSERT_EQ(executor::RemoteCommandRequestBase::kNoTimeout, getNthPendingRequest(0u).timeout); + killFuture.wait(); +} + TEST_F(AsyncResultsMergerTest, TailableBasic) { BSONObj findCmd = fromjson("{find: 'testcoll', tailable: true}"); std::vector<RemoteCursor> cursors; |