SERVER-13537 better handle numerical overflow of large limit values

7 files changed, 36 insertions, 12 deletions

diff --git a/jstests/core/skip1.js b/jstests/core/skip1.js
index c620fb01bca..c1895e8889a 100644
--- a/jstests/core/skip1.js
+++ b/jstests/core/skip1.js
@@ -1,8 +1,9 @@
 // SERVER-2845 When skipping objects without loading them, they shouldn't be
 // included in the nscannedObjects count.
 
+var t = db.jstests_skip1;
+
 if ( 0 ) { // SERVER-2845
-t = db.jstests_skip1;
 t.drop();
 
 t.ensureIndex( {a:1} );
@@ -12,4 +13,19 @@ t.save( {a:5} );
 
 assert.eq( 3, t.find( {a:5} ).skip( 2 ).explain().nscanned );
 assert.eq( 1, t.find( {a:5} ).skip( 2 ).explain().nscannedObjects );
-}
\ No newline at end of file
+}
+
+// SERVER-13537: Ensure that combinations of skip and limit don't crash
+// the server due to overflow.
+t.drop();
+for (var i = 0; i < 10; i++) {
+    t.save({a: i});
+}
+assert.eq( 9, t.find().sort({a: 1}).limit(2147483647).skip(1).itcount() );
+assert.eq( 0, t.find().sort({a: 1}).skip(2147483647).limit(1).itcount() );
+assert.throws( function() {
+    assert.eq( 0, t.find().sort({a: 1}).skip(2147483648).itcount() );
+});
+assert.throws( function() {
+    assert.eq( 0, t.find().sort({a: 1}).limit(2147483648).itcount() );
+});
diff --git a/src/mongo/db/exec/sort.cpp b/src/mongo/db/exec/sort.cpp
index e864c0ede85..db7b6a5777d 100644
--- a/src/mongo/db/exec/sort.cpp
+++ b/src/mongo/db/exec/sort.cpp
@@ -283,7 +283,6 @@ namespace mongo {
           _sorted(false),
           _resultIterator(_data.end()),
           _memUsage(0) {
-        dassert(_limit >= 0);
     }
 
     SortStage::~SortStage() { }
diff --git a/src/mongo/db/exec/sort.h b/src/mongo/db/exec/sort.h
index 6221a505524..91fbb0826f4 100644
--- a/src/mongo/db/exec/sort.h
+++ b/src/mongo/db/exec/sort.h
@@ -55,8 +55,8 @@ namespace mongo {
         // The query.  Used to create the IndexBounds for the sorting.
         BSONObj query;
 
-        // Must be >= 0.  Equal to 0 for no limit.
-        int limit;
+        // Equal to 0 for no limit.
+        size_t limit;
     };
 
     /**
@@ -164,8 +164,8 @@ namespace mongo {
         // The raw query as expressed by the user
         BSONObj _query;
 
-        // Must be >= 0.  Equal to 0 for no limit.
-        int _limit;
+        // Equal to 0 for no limit.
+        size_t _limit;
 
         //
         // Sort key generation
diff --git a/src/mongo/db/query/lite_parsed_query.cpp b/src/mongo/db/query/lite_parsed_query.cpp
index bdf282fa7c8..3635f3b442a 100644
--- a/src/mongo/db/query/lite_parsed_query.cpp
+++ b/src/mongo/db/query/lite_parsed_query.cpp
@@ -224,7 +224,12 @@ namespace mongo {
         if (_ntoskip < 0) {
             return Status(ErrorCodes::BadValue, "bad skip value in query");
         }
-        
+
+        if (_ntoreturn == std::numeric_limits<int>::min()) {
+            // _ntoreturn is negative but can't be negated.
+            return Status(ErrorCodes::BadValue, "bad limit value in query");
+        }
+
         if (_ntoreturn < 0) {
             // _ntoreturn greater than zero is simply a hint on how many objects to send back per
             // "cursor batch".  A negative number indicates a hard limit.
diff --git a/src/mongo/db/query/planner_analysis.cpp b/src/mongo/db/query/planner_analysis.cpp
index b9ee0f44613..23a45b3942d 100644
--- a/src/mongo/db/query/planner_analysis.cpp
+++ b/src/mongo/db/query/planner_analysis.cpp
@@ -406,8 +406,11 @@ namespace mongo {
         // the limit N and skip count M. The sort should return an ordered list
         // N + M items so that the skip stage can discard the first M results.
         if (0 != query.getParsed().getNumToReturn()) {
-            sort->limit = query.getParsed().getNumToReturn() +
-                          query.getParsed().getSkip();
+            // Overflow here would be bad and could cause a nonsense limit. Cast
+            // skip and limit values to unsigned ints to make sure that the
+            // sum is never stored as signed. (See SERVER-13537).
+            sort->limit = size_t(query.getParsed().getNumToReturn()) +
+                          size_t(query.getParsed().getSkip());
 
             // This is a SORT with a limit. The wire protocol has a single quantity
             // called "numToReturn" which could mean either limit or batchSize.
diff --git a/src/mongo/db/query/query_planner_test_lib.cpp b/src/mongo/db/query/query_planner_test_lib.cpp
index c18f2946287..2fe75527574 100644
--- a/src/mongo/db/query/query_planner_test_lib.cpp
+++ b/src/mongo/db/query/query_planner_test_lib.cpp
@@ -421,8 +421,9 @@ namespace mongo {
             BSONElement child = sortObj["node"];
             if (child.eoo() || !child.isABSONObj()) { return false; }
 
+            size_t expectedLimit = limitEl.numberInt();
             return (patternEl.Obj() == sn->pattern)
-                   && (limitEl.numberInt() == sn->limit)
+                   && (expectedLimit == sn->limit)
                    && solutionMatches(child.Obj(), sn->children[0]);
         }
         else if (STAGE_SORT_MERGE == trueSoln->getType()) {
diff --git a/src/mongo/db/query/query_solution.h b/src/mongo/db/query/query_solution.h
index b5ca922eb1a..7721e718f83 100644
--- a/src/mongo/db/query/query_solution.h
+++ b/src/mongo/db/query/query_solution.h
@@ -526,7 +526,7 @@ namespace mongo {
         BSONObj query;
 
         // Sum of both limit and skip count in the parsed query.
-        int limit;
+        size_t limit;
     };
 
     struct LimitNode : public QuerySolutionNode {