SERVER-31721 invalid $expr within $and/$or can segfault on call to optimize

3 files changed, 41 insertions, 2 deletions

diff --git a/jstests/core/expr.js b/jstests/core/expr.js
index dbd00d0c948..05e053959a6 100644
--- a/jstests/core/expr.js
+++ b/jstests/core/expr.js
@@ -64,6 +64,21 @@
         coll.find({$expr: {$eq: ["$a", "$$unbound"]}}).itcount();
     });
 
+    // $and with $expr child containing an invalid expression throws.
+    assert.throws(function() {
+        coll.find({$and: [{a: 0}, {$expr: {$anyElementTrue: undefined}}]}).itcount();
+    });
+
+    // $or with $expr child containing an invalid expression throws.
+    assert.throws(function() {
+        coll.find({$or: [{a: 0}, {$expr: {$anyElementTrue: undefined}}]}).itcount();
+    });
+
+    // $nor with $expr child containing an invalid expression throws.
+    assert.throws(function() {
+        coll.find({$nor: [{a: 0}, {$expr: {$anyElementTrue: undefined}}]}).itcount();
+    });
+
     // $expr with division by zero throws.
     assert.throws(function() {
         coll.find({$expr: {$divide: [1, "$a"]}}).itcount();
diff --git a/src/mongo/db/matcher/expression_expr_test.cpp b/src/mongo/db/matcher/expression_expr_test.cpp
index 4cb8be31e16..e357ba3ddca 100644
--- a/src/mongo/db/matcher/expression_expr_test.cpp
+++ b/src/mongo/db/matcher/expression_expr_test.cpp
@@ -491,6 +491,24 @@ TEST_F(ExprMatchTest, SetCollatorChangesCollationUsedForComparisons) {
                               << "cba")));
 }
 
+TEST_F(ExprMatchTest, FailGracefullyOnInvalidExpression) {
+    ASSERT_THROWS_CODE(createMatcher(fromjson("{$expr: {$anyElementTrue: undefined}}")),
+                       AssertionException,
+                       17041);
+    ASSERT_THROWS_CODE(
+        createMatcher(fromjson("{$and: [{x: 1},{$expr: {$anyElementTrue: undefined}}]}")),
+        AssertionException,
+        17041);
+    ASSERT_THROWS_CODE(
+        createMatcher(fromjson("{$or: [{x: 1},{$expr: {$anyElementTrue: undefined}}]}")),
+        AssertionException,
+        17041);
+    ASSERT_THROWS_CODE(
+        createMatcher(fromjson("{$nor: [{x: 1},{$expr: {$anyElementTrue: undefined}}]}")),
+        AssertionException,
+        17041);
+}
+
 TEST(ExprMatchTest, IdenticalPostOptimizedExpressionsAreEquivalent) {
     BSONObj expression = BSON("$expr" << BSON("$multiply" << BSON_ARRAY(2 << 2)));
     BSONObj expressionEquiv = BSON("$expr" << BSON("$const" << 4));
diff --git a/src/mongo/db/matcher/expression_tree.cpp b/src/mongo/db/matcher/expression_tree.cpp
index 375fe90c533..df8ab0b4012 100644
--- a/src/mongo/db/matcher/expression_tree.cpp
+++ b/src/mongo/db/matcher/expression_tree.cpp
@@ -38,8 +38,9 @@
 namespace mongo {
 
 ListOfMatchExpression::~ListOfMatchExpression() {
-    for (unsigned i = 0; i < _expressions.size(); i++)
+    for (unsigned i = 0; i < _expressions.size(); i++) {
         delete _expressions[i];
+    }
     _expressions.clear();
 }
 
@@ -70,7 +71,12 @@ MatchExpression::ExpressionOptimizerFunc ListOfMatchExpression::getOptimizer() c
         for (auto& childExpression : children) {
             // Since 'childExpression' is a reference to a member of the ListOfMatchExpression's
             // child array, this assignment replaces the original child with the optimized child.
+            // We must set this child's entry in '_expressions' to null after assigning ownership to
+            // 'childExpressionPtr'. Otherwise, if the call to optimize() throws we will attempt to
+            // free twice.
             std::unique_ptr<MatchExpression> childExpressionPtr(childExpression);
+            childExpression = nullptr;
+
             auto optimizedExpression = MatchExpression::optimize(std::move(childExpressionPtr));
             childExpression = optimizedExpression.release();
         }
@@ -85,7 +91,7 @@ MatchExpression::ExpressionOptimizerFunc ListOfMatchExpression::getOptimizer() c
                     // Move this child out of the children array.
                     std::unique_ptr<ListOfMatchExpression> childExpressionPtr(
                         static_cast<ListOfMatchExpression*>(childExpression));
-                    childExpression = nullptr;  // NULL out this child's entry in _expressions, so
+                    childExpression = nullptr;  // Null out this child's entry in _expressions, so
                                                 // that it will be deleted by the erase call below.
 
                     // Move all of the grandchildren from the child expression to