SERVER-61566 Fix and extract coercion to 32-bit int logic in expression parsing

5 files changed, 105 insertions, 74 deletions

diff --git a/src/mongo/bson/bsonelement.cpp b/src/mongo/bson/bsonelement.cpp
index d93f928861c..e02dfd07f33 100644
--- a/src/mongo/bson/bsonelement.cpp
+++ b/src/mongo/bson/bsonelement.cpp
@@ -515,7 +515,8 @@ StatusWith<long long> BSONElement::parseIntegerElementToNonNegativeLong() const
 
     if (number.getValue() < 0) {
         return Status(ErrorCodes::FailedToParse,
-                      str::stream() << "Expected a positive number in: " << toString(true, true));
+                      str::stream()
+                          << "Expected a non-negative number in: " << toString(true, true));
     }
 
     return number;
@@ -585,6 +586,21 @@ StatusWith<int> BSONElement::parseIntegerElementToInt() const {
     return static_cast<int>(valueLong);
 }
 
+StatusWith<int> BSONElement::parseIntegerElementToNonNegativeInt() const {
+    auto number = parseIntegerElementToInt();
+    if (!number.isOK()) {
+        return number;
+    }
+
+    if (number.getValue() < 0) {
+        return Status(ErrorCodes::FailedToParse,
+                      str::stream()
+                          << "Expected a non-negative number in: " << toString(true, true));
+    }
+
+    return number;
+}
+
 BSONObj BSONElement::embeddedObjectUserCheck() const {
     if (MONGO_likely(isABSONObj()))
         return BSONObj(value(), BSONObj::LargeSizeTrait{});
diff --git a/src/mongo/bson/bsonelement.h b/src/mongo/bson/bsonelement.h
index 11a8ad6994a..c99623c7e80 100644
--- a/src/mongo/bson/bsonelement.h
+++ b/src/mongo/bson/bsonelement.h
@@ -412,7 +412,7 @@ public:
     long long exactNumberLong() const;
 
     /**
-     * Parses a BSONElement of any numeric type into a positive long long, failing if the value
+     * Parses a BSONElement of any numeric type into a non-negative long long, failing if the value
      * is any of the following:
      *
      * - NaN.
@@ -433,6 +433,17 @@ public:
     StatusWith<long long> parseIntegerElementToLong() const;
 
     /**
+     * Parses a BSONElement of any numeric type into a non-negative int, failing if the value
+     * is any of the following:
+     *
+     * - NaN
+     * - Negative
+     * - a non-integral number
+     * - too large in the positive or negative direction to fit in an int
+     */
+    StatusWith<int> parseIntegerElementToNonNegativeInt() const;
+
+    /**
      * Parses a BSONElement of any numeric type into an integer, failing if the value is:
      *
      * - NaN
diff --git a/src/mongo/db/matcher/expression_parser.cpp b/src/mongo/db/matcher/expression_parser.cpp
index 887caf45535..e5dd4c3816f 100644
--- a/src/mongo/db/matcher/expression_parser.cpp
+++ b/src/mongo/db/matcher/expression_parser.cpp
@@ -696,58 +696,13 @@ StatusWith<std::vector<uint32_t>> parseBitPositionsArray(const BSONObj& theArray
 
     // Fill temporary bit position array with integers read from the BSON array.
     for (auto e : theArray) {
-        if (!e.isNumber()) {
-            return Status(ErrorCodes::BadValue,
-                          str::stream() << "bit positions must be an integer but got: " << e);
-        }
-
-        if (e.type() == BSONType::NumberDouble) {
-            auto eDouble = e.numberDouble();
-
-            // NaN doubles are rejected.
-            if (std::isnan(eDouble)) {
-                return Status(ErrorCodes::BadValue,
-                              str::stream() << "bit positions cannot take a NaN: " << e);
-            }
-
-            // This makes sure e does not overflow a 32-bit integer container.
-            if (eDouble > std::numeric_limits<int>::max() ||
-                eDouble < std::numeric_limits<int>::min()) {
-                return Status(
-                    ErrorCodes::BadValue,
-                    str::stream()
-                        << "bit positions cannot be represented as a 32-bit signed integer: " << e);
-            }
-
-            // This checks if e is integral.
-            if (eDouble != static_cast<double>(static_cast<long long>(eDouble))) {
-                return Status(ErrorCodes::BadValue,
-                              str::stream() << "bit positions must be an integer but got: " << e);
-            }
-        }
-
-        if (e.type() == BSONType::NumberLong) {
-            auto eLong = e.numberLong();
-
-            // This makes sure e does not overflow a 32-bit integer container.
-            if (eLong > std::numeric_limits<int>::max() ||
-                eLong < std::numeric_limits<int>::min()) {
-                return Status(
-                    ErrorCodes::BadValue,
-                    str::stream()
-                        << "bit positions cannot be represented as a 32-bit signed integer: " << e);
-            }
-        }
-
-        auto eValue = e.numberInt();
-
-        // No negatives.
-        if (eValue < 0) {
+        auto status = e.parseIntegerElementToNonNegativeInt();
+        if (!status.isOK()) {
             return Status(ErrorCodes::BadValue,
-                          str::stream() << "bit positions must be >= 0 but got: " << e);
+                          str::stream()
+                              << "Failed to parse bit position. " << status.getStatus().reason());
         }
-
-        bitPositions.push_back(eValue);
+        bitPositions.push_back(status.getValue());
     }
 
     return bitPositions;
@@ -1724,29 +1679,13 @@ StatusWithMatchExpression parseSubField(const BSONObj& context,
         }
 
         case PathAcceptingKeyword::SIZE: {
-            int size = 0;
-            if (e.type() == BSONType::NumberInt) {
-                size = e.numberInt();
-            } else if (e.type() == BSONType::NumberLong) {
-                if (e.numberInt() == e.numberLong()) {
-                    size = e.numberInt();
-                } else {
-                    return {Status(ErrorCodes::BadValue,
-                                   "$size must be representable as a 32-bit integer")};
-                }
-            } else if (e.type() == BSONType::NumberDouble) {
-                if (e.numberInt() == e.numberDouble()) {
-                    size = e.numberInt();
-                } else {
-                    return {Status(ErrorCodes::BadValue, "$size must be a whole number")};
-                }
-            } else {
-                return {Status(ErrorCodes::BadValue, "$size needs a number")};
-            }
-
-            if (size < 0) {
-                return {Status(ErrorCodes::BadValue, "$size may not be negative")};
+            auto status = e.parseIntegerElementToNonNegativeInt();
+            if (!status.isOK()) {
+                return Status(ErrorCodes::BadValue,
+                              str::stream()
+                                  << "Failed to parse $size. " << status.getStatus().reason());
             }
+            int size = status.getValue();
             return {std::make_unique<SizeMatchExpression>(
                 name,
                 size,
diff --git a/src/mongo/db/matcher/expression_parser_array_test.cpp b/src/mongo/db/matcher/expression_parser_array_test.cpp
index c519ee2b374..243bed553f0 100644
--- a/src/mongo/db/matcher/expression_parser_array_test.cpp
+++ b/src/mongo/db/matcher/expression_parser_array_test.cpp
@@ -109,6 +109,13 @@ TEST(MatchExpressionParserArrayTest, SizeWithNegativeIntegralDouble) {
     ASSERT_FALSE(result.isOK());
 }
 
+TEST(MatchExpressionParserArrayTest, SizeWithTooLargeDouble) {
+    BSONObj query = BSON("x" << BSON("$size" << 1e12));
+    boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
+    StatusWithMatchExpression result = MatchExpressionParser::parse(query, expCtx);
+    ASSERT_FALSE(result.isOK());
+}
+
 TEST(MatchExpressionParserArrayTest, SizeWithDouble) {
     BSONObj query = BSON("x" << BSON("$size" << 2.5));
     boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
diff --git a/src/mongo/db/matcher/expression_parser_test.cpp b/src/mongo/db/matcher/expression_parser_test.cpp
index a37e25c9542..af6e1ff997a 100644
--- a/src/mongo/db/matcher/expression_parser_test.cpp
+++ b/src/mongo/db/matcher/expression_parser_test.cpp
@@ -519,6 +519,64 @@ TEST(MatchExpressionParserTest, SampleRateFailureCases) {
     ASSERT_NOT_OK(result.getStatus());
 }
 
+TEST(MatchExpressionParserTest, BitwiseOperators) {
+    boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
+    std::vector<std::string> bitwiseOperators{
+        "$bitsAllClear", "$bitsAllSet", "$bitsAnyClear", "$bitsAnySet"};
+    for (auto& bitwiseOperator : bitwiseOperators) {
+        // Test accepting valid type coercion
+        auto result = MatchExpressionParser::parse(
+            BSON("x" << BSON(bitwiseOperator << BSON_ARRAY(1))), expCtx);
+        ASSERT_TRUE(result.isOK());
+        result = MatchExpressionParser::parse(BSON("x" << BSON(bitwiseOperator << BSON_ARRAY(1LL))),
+                                              expCtx);
+        ASSERT_TRUE(result.isOK());
+        result = MatchExpressionParser::parse(BSON("x" << BSON(bitwiseOperator << BSON_ARRAY(1.0))),
+                                              expCtx);
+        ASSERT_TRUE(result.isOK());
+
+        // Test rejecting overflow values.
+        result = MatchExpressionParser::parse(
+            BSON("x" << BSON(bitwiseOperator << BSON_ARRAY(std::numeric_limits<long long>::min()))),
+            expCtx);
+        ASSERT_EQ(result.getStatus(), ErrorCodes::BadValue);
+        result = MatchExpressionParser::parse(
+            BSON("x" << BSON(bitwiseOperator << BSON_ARRAY(std::numeric_limits<long long>::max()))),
+            expCtx);
+        ASSERT_EQ(result.getStatus(), ErrorCodes::BadValue);
+        result = MatchExpressionParser::parse(
+            BSON("x" << BSON(bitwiseOperator << BSON_ARRAY(1e30))), expCtx);
+        ASSERT_EQ(result.getStatus(), ErrorCodes::BadValue);
+        result = MatchExpressionParser::parse(
+            BSON("x" << BSON(bitwiseOperator << BSON_ARRAY(-1e30))), expCtx);
+        ASSERT_EQ(result.getStatus(), ErrorCodes::BadValue);
+
+        // Test rejecting non-integral values.
+        result = MatchExpressionParser::parse(BSON("x" << BSON(bitwiseOperator << BSON_ARRAY(1.5))),
+                                              expCtx);
+        ASSERT_EQ(result.getStatus(), ErrorCodes::BadValue);
+
+        // Test rejecting negative values.
+        result = MatchExpressionParser::parse(BSON("x" << BSON(bitwiseOperator << BSON_ARRAY(-1))),
+                                              expCtx);
+        ASSERT_EQ(result.getStatus(), ErrorCodes::BadValue);
+        result = MatchExpressionParser::parse(
+            BSON("x" << BSON(bitwiseOperator << BSON_ARRAY(-1LL))), expCtx);
+        ASSERT_EQ(result.getStatus(), ErrorCodes::BadValue);
+        result = MatchExpressionParser::parse(
+            BSON("x" << BSON(bitwiseOperator << BSON_ARRAY(-1.0))), expCtx);
+        ASSERT_EQ(result.getStatus(), ErrorCodes::BadValue);
+
+        // Test rejecting non-numeric values.
+        result = MatchExpressionParser::parse(
+            BSON("x" << BSON(bitwiseOperator << BSON_ARRAY("string"))), expCtx);
+        ASSERT_EQ(result.getStatus(), ErrorCodes::BadValue);
+        result = MatchExpressionParser::parse(BSON("x" << BSON(bitwiseOperator << BSON_ARRAY(NAN))),
+                                              expCtx);
+        ASSERT_EQ(result.getStatus(), ErrorCodes::BadValue);
+    }
+}
+
 TEST(InternalBinDataSubTypeMatchExpressionTest, SubTypeParsesCorrectly) {
     boost::intrusive_ptr<ExpressionContextForTest> expCtx(new ExpressionContextForTest());
     auto query = BSON("a" << BSON("$_internalSchemaBinDataSubType" << BinDataType::bdtCustom));