SERVER-7952 Kill an authorized ClientCursor after processGetMore throws.

5 files changed, 208 insertions, 3 deletions

diff --git a/jstests/slowNightly/replsets_killop.js b/jstests/slowNightly/replsets_killop.js
new file mode 100644
index 00000000000..3d3ee51f709
--- /dev/null
+++ b/jstests/slowNightly/replsets_killop.js
@@ -0,0 +1,69 @@
+// Test correctness of replication while a secondary's get more requests are killed on the primary
+// using killop.  SERVER-7952
+
+numDocs = 1e5;
+
+// Set up a replica set.
+replTest = new ReplSetTest( { name:'test', nodes:3 } );
+nodes = replTest.startSet();
+replTest.initiate();
+primary = replTest.getMaster();
+secondary = replTest.getSecondary();
+db = primary.getDB( 'test' );
+db.test.save( { a:0 } );
+replTest.awaitReplication();
+assert.soon( function() { return secondary.getDB( 'test' ).test.count() == 1; } );
+
+// Start a parallel shell to insert new documents on the primary.
+inserter = startParallelShell(
+    'for( i = 1; i < ' + numDocs + '; ++i ) { \
+         db.test.save( { a:i } );             \
+         sleep( 1 );                          \
+     }                                        \
+     db.getLastError();'
+);
+
+// Periodically kill replication get mores.
+for( i = 0; i < 1e3; ++i ) {
+    allOps = db.currentOp();
+    for( j in allOps.inprog ) {
+        op = allOps.inprog[ j ];
+        if ( op.ns == 'local.oplog.rs' && op.op == 'getmore' ) {
+            db.killOp( op.opid );
+        }
+    }
+    sleep( 100 );
+}
+
+// Wait for the inserter to finish.
+inserter();
+
+assert.eq( numDocs, db.test.count() );
+
+// Return true when the correct number of documents are present on the secondary.  Otherwise print
+// which documents are missing and return false.
+function allReplicated() {
+    count = secondary.getDB( 'test' ).test.count();
+    if ( count == numDocs ) {
+        // Return true if the count is as expected.
+        return true;
+    }
+    
+    // Identify and print the missing a-values.
+    foundSet = {};
+    c = secondary.getDB( 'test' ).test.find();
+    while( c.hasNext() ) {
+        foundSet[ '' + c.next().a ] = true;
+    }
+    missing = [];
+    for( i = 0; i < numDocs; ++i ) {
+        if ( !( ( '' + i ) in foundSet ) ) {
+            missing.push( i );
+        }
+    }
+    print( 'count: ' + count + ' missing: ' + missing );
+    return false;
+}
+
+// Wait for the correct number of (replicated) documents to be present on the secondary.
+assert.soon( allReplicated );
diff --git a/src/mongo/db/instance.cpp b/src/mongo/db/instance.cpp
index 098467d4388..86ba6ead40c 100644
--- a/src/mongo/db/instance.cpp
+++ b/src/mongo/db/instance.cpp
@@ -670,6 +670,7 @@ namespace mongo {
         QueryResult* msgdata = 0;
         OpTime last;
         while( 1 ) {
+            bool isCursorAuthorized = false;
             try {
                 const NamespaceString nsString( ns );
                 uassert( 16258, str::stream() << "Invalid ns [" << ns << "]", nsString.isValid() );
@@ -691,9 +692,23 @@ namespace mongo {
                     }
                 }
 
-                msgdata = processGetMore(ns, ntoreturn, cursorid, curop, pass, exhaust);
+                msgdata = processGetMore(ns,
+                                         ntoreturn,
+                                         cursorid,
+                                         curop,
+                                         pass,
+                                         exhaust,
+                                         &isCursorAuthorized);
             }
             catch ( AssertionException& e ) {
+                if ( isCursorAuthorized ) {
+                    // If a cursor with id 'cursorid' was authorized, it may have been advanced
+                    // before an exception terminated processGetMore.  Erase the ClientCursor
+                    // because it may now be out of sync with the client's iteration state.
+                    // SERVER-7952
+                    // TODO Temporary code, see SERVER-4563 for a cleanup overview.
+                    ClientCursor::erase( cursorid );
+                }
                 ex.reset( new AssertionException( e.getInfo().msg, e.getCode() ) );
                 ok = false;
                 break;
diff --git a/src/mongo/db/ops/query.cpp b/src/mongo/db/ops/query.cpp
index 69154969df9..48a191b1bbb 100644
--- a/src/mongo/db/ops/query.cpp
+++ b/src/mongo/db/ops/query.cpp
@@ -83,7 +83,13 @@ namespace mongo {
         return qr;
     }
 
-    QueryResult* processGetMore(const char *ns, int ntoreturn, long long cursorid , CurOp& curop, int pass, bool& exhaust ) {
+    QueryResult* processGetMore(const char* ns,
+                                int ntoreturn,
+                                long long cursorid,
+                                CurOp& curop,
+                                int pass,
+                                bool& exhaust,
+                                bool* isCursorAuthorized ) {
         exhaust = false;
 
         int bufSize = 512 + sizeof( QueryResult ) + MaxBytesToReturnToClientAtOnce;
@@ -111,6 +117,8 @@ namespace mongo {
             // check for spoofing of the ns such that it does not match the one originally there for the cursor
             uassert(14833, "auth error", str::equals(ns, cc->ns().c_str()));
 
+            *isCursorAuthorized = true;
+
             if ( pass == 0 )
                 cc->updateSlaveLocation( curop );
 
diff --git a/src/mongo/db/ops/query.h b/src/mongo/db/ops/query.h
index cb1a7f3c7a0..1b67de7e247 100644
--- a/src/mongo/db/ops/query.h
+++ b/src/mongo/db/ops/query.h
@@ -34,7 +34,18 @@ namespace mongo {
     class QueryOptimizerCursor;
     class QueryPlanSummary;
     
-    QueryResult* processGetMore(const char *ns, int ntoreturn, long long cursorid , CurOp& op, int pass, bool& exhaust);
+    /**
+     * Return a batch of results from a client OP_GET_MORE request.
+     * 'cursorid' - The id of the cursor producing results.
+     * 'isCursorAuthorized' - Set to true after a cursor with id 'cursorid' is authorized for use.
+     */
+    QueryResult* processGetMore(const char* ns,
+                                int ntoreturn,
+                                long long cursorid,
+                                CurOp& op,
+                                int pass,
+                                bool& exhaust,
+                                bool* isCursorAuthorized);
 
     string runQuery(Message& m, QueryMessage& q, CurOp& curop, Message &result);
 
diff --git a/src/mongo/dbtests/querytests.cpp b/src/mongo/dbtests/querytests.cpp
index d85c7b00d3b..31f7f43e67d 100644
--- a/src/mongo/dbtests/querytests.cpp
+++ b/src/mongo/dbtests/querytests.cpp
@@ -25,6 +25,7 @@
 #include "mongo/db/dbhelpers.h"
 #include "mongo/db/instance.h"
 #include "mongo/db/json.h"
+#include "mongo/db/kill_current_op.h"
 #include "mongo/db/lasterror.h"
 #include "mongo/db/oplog.h"
 #include "mongo/db/scanandorder.h"
@@ -217,6 +218,105 @@ namespace QueryTests {
         }
     };
 
+    /**
+     * An exception triggered during a get more request destroys the ClientCursor used by the get
+     * more, preventing further iteration of the cursor in subsequent get mores.
+     */
+    class GetMoreKillOp : public ClientBase {
+    public:
+        ~GetMoreKillOp() {
+            killCurrentOp.reset();
+            client().dropCollection( "unittests.querytests.GetMoreKillOp" );
+        }
+        void run() {
+            
+            // Create a collection with some data.
+            const char* ns = "unittests.querytests.GetMoreKillOp";
+            for( int i = 0; i < 1000; ++i ) {
+                insert( ns, BSON( "a" << i ) );
+            }
+
+            // Create a cursor on the collection, with a batch size of 200.
+            auto_ptr<DBClientCursor> cursor = client().query( ns, "", 0, 0, 0, 0, 200 );
+            CursorId cursorId = cursor->getCursorId();
+            
+            // Count 500 results, spanning a few batches of documents.
+            for( int i = 0; i < 500; ++i ) {
+                ASSERT( cursor->more() );
+                cursor->next();
+            }
+            
+            // Set the killop kill all flag, forcing the next get more to fail with a kill op
+            // exception.
+            killCurrentOp.killAll();
+            while( cursor->more() ) {
+                cursor->next();
+            }
+            
+            // Revert the killop kill all flag.
+            killCurrentOp.reset();
+
+            // Check that the cursor has been removed.
+            set<CursorId> ids;
+            ClientCursor::find( ns, ids );
+            ASSERT_EQUALS( 0U, ids.count( cursorId ) );
+
+            // Check that a subsequent get more fails with the cursor removed.
+            ASSERT_THROWS( client().getMore( ns, cursorId ), UserException );
+        }
+    };
+
+    /**
+     * A get more exception caused by an invalid or unauthorized get more request does not cause
+     * the get more's ClientCursor to be destroyed.  This prevents an unauthorized user from
+     * improperly killing a cursor by issuing an invalid get more request.
+     */
+    class GetMoreInvalidRequest : public ClientBase {
+    public:
+        ~GetMoreInvalidRequest() {
+            killCurrentOp.reset();
+            client().dropCollection( "unittests.querytests.GetMoreInvalidRequest" );
+        }
+        void run() {
+
+            // Create a collection with some data.
+            const char* ns = "unittests.querytests.GetMoreInvalidRequest";
+            for( int i = 0; i < 1000; ++i ) {
+                insert( ns, BSON( "a" << i ) );
+            }
+            
+            // Create a cursor on the collection, with a batch size of 200.
+            auto_ptr<DBClientCursor> cursor = client().query( ns, "", 0, 0, 0, 0, 200 );
+            CursorId cursorId = cursor->getCursorId();
+
+            // Count 500 results, spanning a few batches of documents.
+            int count = 0;
+            for( int i = 0; i < 500; ++i ) {
+                ASSERT( cursor->more() );
+                cursor->next();
+                ++count;
+            }
+
+            // Send a get more with a namespace that is incorrect ('spoofed') for this cursor id.
+            // This is the invalaid get more request described in the comment preceding this class.
+            client().getMore
+                    ( "unittests.querytests.GetMoreInvalidRequest_WRONG_NAMESPACE_FOR_CURSOR",
+                      cursor->getCursorId() );
+
+            // Check that the cursor still exists
+            set<CursorId> ids;
+            ClientCursor::find( ns, ids );
+            ASSERT_EQUALS( 1U, ids.count( cursorId ) );
+            
+            // Check that the cursor can be iterated until all documents are returned.
+            while( cursor->more() ) {
+                cursor->next();
+                ++count;
+            }
+            ASSERT_EQUALS( 1000, count );
+        }
+    };
+    
     class PositiveLimit : public ClientBase {
     public:
         const char* ns;
@@ -1569,6 +1669,8 @@ namespace QueryTests {
             add< FindOneEmptyObj >();
             add< BoundedKey >();
             add< GetMore >();
+            add< GetMoreKillOp >();
+            add< GetMoreInvalidRequest >();
             add< PositiveLimit >();
             add< ReturnOneOfManyAndTail >();
             add< TailNotAtEnd >();