From 87ea815f63639b6513a56232e7c8516bb4fad707 Mon Sep 17 00:00:00 2001
From: James Stone <james.stone@mongodb.com>
Date: Tue, 28 Mar 2023 10:14:45 -0700
Subject: [PATCH] remove some recursion when parsing chained OR/AND queries
 (#6444)

* remove some recursion when parsing chained OR/AND queries

* enable device tests of recursive parsing
---
 CHANGELOG.md                |  3 +-
 src/realm/parser/driver.hpp | 35 ++++++++++++++++------
 test/test_parser.cpp        | 60 +++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 81cd4d14a06..3e9b9542dad 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,11 +5,10 @@
 * None.
 
 ### Fixed
-* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
+* Fix a stack overflow crash when using the query parser with long chains of AND/OR conditions. ([#6428](https://github.com/realm/realm-core/pull/6428), since v11.7.0)
 * Changing the log level on the fly would not affect the core level log output ([#6440](https://github.com/realm/realm-core/issues/6440), since 13.7.0)
 * `SyncManager::immediately_run_file_actions()` no longer ignores the result of trying to remove a realm. This could have resulted in a client reset action being reported as successful when it actually failed on windows if the `Realm` was still open ([#6050](https://github.com/realm/realm-core/issues/6050)).
 
-
 ### Breaking changes
 * None.
 
diff --git a/src/realm/parser/driver.hpp b/src/realm/parser/driver.hpp
index 6747e882146..39000d41366 100644
--- a/src/realm/parser/driver.hpp
+++ b/src/realm/parser/driver.hpp
@@ -55,21 +55,38 @@ class LogicalNode : public QueryNode {
     }
     void canonicalize() override
     {
-        std::vector<QueryNode*> newChildren;
+        std::vector<LogicalNode*> todo;
+        do_canonicalize(todo);
+        while (todo.size()) {
+            LogicalNode* cur = todo.back();
+            todo.pop_back();
+            cur->do_canonicalize(todo);
+        }
+    }
+
+    void do_canonicalize(std::vector<LogicalNode*>& todo)
+    {
         auto& my_type = typeid(*this);
-        for (auto& child : children) {
-            child->canonicalize();
-            if (typeid(*child) == my_type) {
+        size_t index = 0;
+        while (index < children.size()) {
+            QueryNode* child = *(children.begin() + index);
+            auto& child_type = typeid(*child);
+            if (child_type == my_type) {
                 auto logical_node = static_cast<LogicalNode*>(child);
-                for (auto c : logical_node->children) {
-                    newChildren.push_back(c);
-                }
+                REALM_ASSERT_EX(logical_node->children.size() == 2, logical_node->children.size());
+                children.push_back(logical_node->children[0]);
+                children.push_back(logical_node->children[1]);
+                children.erase(children.begin() + index);
+                continue; // do not ++index because of the delete
+            }
+            else if (auto ln = dynamic_cast<LogicalNode*>(child)) {
+                todo.push_back(ln);
             }
             else {
-                newChildren.push_back(child);
+                child->canonicalize();
             }
+            ++index;
         }
-        children = newChildren;
     }
 
 private:
diff --git a/test/test_parser.cpp b/test/test_parser.cpp
index c28f9794071..11ceeca75aa 100644
--- a/test/test_parser.cpp
+++ b/test/test_parser.cpp
@@ -5648,4 +5648,64 @@ TEST(Parser_PrimaryKey)
     CHECK_EQUAL(q.get_description(), query_string);
 }
 
+TEST(Parser_RecursiveLogial)
+{
+    using util::serializer::print_value;
+    Group g;
+    auto table = g.add_table_with_primary_key("table", type_ObjectId, "id");
+    table->create_object_with_primary_key(ObjectId());
+
+    {
+        std::vector<Mixed> args = {ObjectId(), ObjectId::gen(), ObjectId::gen()};
+        verify_query_sub(test_context, table,
+                         "TRUEPREDICATE AND (id == $1 OR id == $2 OR id == $0) AND TRUEPREDICATE", args, 1);
+        verify_query_sub(
+            test_context, table,
+            "(id == $1 OR id == $2 OR id == $0) AND (TRUEPREDICATE AND id == $0 AND id == $0) AND TRUEPREDICATE",
+            args, 1);
+        verify_query_sub(
+            test_context, table,
+            "(id == $1 OR id == $2 OR id == $0) AND (FALSEPREDICATE AND id == $0 AND id == $0) AND TRUEPREDICATE",
+            args, 0);
+        verify_query_sub(test_context, table,
+                         "(id == $1 OR id == $2 OR FALSEPREDICATE) AND (TRUEPREDICATE AND id == $0 AND id == $0) AND "
+                         "TRUEPREDICATE",
+                         args, 0);
+        verify_query_sub(
+            test_context, table,
+            "(id == $1 OR id == $2 OR id == $0) AND (TRUEPREDICATE AND id == $0 AND id == $0) AND FALSEPREDICATE",
+            args, 0);
+    }
+
+    constexpr size_t num_args = 1000;
+    std::vector<Mixed> args;
+    args.reserve(num_args);
+    for (size_t i = 0; i < num_args; ++i) {
+        args.push_back(ObjectId::gen());
+    }
+
+    std::string base_query;
+    for (size_t i = 0; i < 1000; ++i) {
+        base_query += util::format("%1id = $%2", i == 0 ? "" : " OR ", i);
+    }
+    // minimum size to trigger a stack overflow on "my" machine in debug mode
+    constexpr size_t num_repeats = 53;
+    std::string query = "FALSEPREDICATE";
+    query.reserve(query.size() + ((4 + base_query.size()) * num_repeats));
+    for (size_t i = 0; i < num_repeats; ++i) {
+        query.append(" OR ");
+        query.append(base_query);
+    }
+    Query q = table->query(query, args, {});
+    size_t q_count = q.count();
+    CHECK_EQUAL(q_count, 0);
+
+    query.append(util::format(" OR id = oid(%1)", ObjectId()));
+    query.append(" OR ");
+    query.append(base_query);
+    q = table->query(query, args, {});
+    q_count = q.count();
+    CHECK_EQUAL(q_count, 1);
+}
+
 #endif // TEST_PARSER