Fix double free bug inside IN Filter (#10535)

cherylEnkidu · web-flow · commit 10295bad6ba1 · 2022-11-30T12:52:59.000-08:00
diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md
@@ -1,3 +1,6 @@
+# Unreleased
+- [fixed] Fix App crashed when there are nested data structures inside IN Filter.
+
 # 10.3.0
 - [feature] Add MultiDb support.
 
diff --git a/Firestore/Example/Tests/API/FSTUserDataReaderTests.mm b/Firestore/Example/Tests/API/FSTUserDataReaderTests.mm
@@ -36,10 +36,10 @@
 namespace nanopb = firebase::firestore::nanopb;
 using firebase::Timestamp;
 using firebase::firestore::GeoPoint;
-using firebase::firestore::api::MakeGeoPoint;
-using firebase::firestore::api::MakeTimestamp;
 using firebase::firestore::google_firestore_v1_ArrayValue;
 using firebase::firestore::google_firestore_v1_Value;
+using firebase::firestore::api::MakeGeoPoint;
+using firebase::firestore::api::MakeTimestamp;
 using firebase::firestore::model::ArrayTransform;
 using firebase::firestore::model::DatabaseId;
 using firebase::firestore::model::FieldPath;
diff --git a/Firestore/Example/Tests/Integration/API/FIRBundlesTests.mm b/Firestore/Example/Tests/Integration/API/FIRBundlesTests.mm
@@ -27,8 +27,8 @@
 #include "Firestore/core/test/unit/testutil/bundle_builder.h"
 
 using firebase::firestore::testutil::CreateBundle;
-using firebase::firestore::util::MakeString;
 using firebase::firestore::util::MakeNSString;
+using firebase::firestore::util::MakeString;
 
 @interface FIRBundlesTests : FSTIntegrationTestCase
 @end
diff --git a/Firestore/Example/Tests/Integration/FSTDatastoreTests.mm b/Firestore/Example/Tests/Integration/FSTDatastoreTests.mm
@@ -55,11 +55,11 @@
 #include "absl/memory/memory.h"
 
 using firebase::Timestamp;
+using firebase::firestore::google_firestore_v1_Value;
 using firebase::firestore::core::DatabaseInfo;
 using firebase::firestore::credentials::EmptyAppCheckCredentialsProvider;
 using firebase::firestore::credentials::EmptyAuthCredentialsProvider;
 using firebase::firestore::credentials::User;
-using firebase::firestore::google_firestore_v1_Value;
 using firebase::firestore::local::LocalStore;
 using firebase::firestore::local::MemoryPersistence;
 using firebase::firestore::local::Persistence;
diff --git a/Firestore/Example/Tests/SpecTests/FSTMemorySpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTMemorySpecTests.mm
@@ -24,8 +24,8 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
-using firebase::firestore::local::MemoryPersistenceWithLruGcForTesting;
 using firebase::firestore::local::MemoryPersistenceWithEagerGcForTesting;
+using firebase::firestore::local::MemoryPersistenceWithLruGcForTesting;
 using firebase::firestore::local::Persistence;
 
 /**
diff --git a/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm b/Firestore/Example/Tests/SpecTests/FSTSpecTests.mm
@@ -75,19 +75,18 @@
 
 namespace objc = firebase::firestore::objc;
 using firebase::firestore::Error;
+using firebase::firestore::google_firestore_v1_ArrayValue;
+using firebase::firestore::google_firestore_v1_Value;
 using firebase::firestore::api::LoadBundleTask;
 using firebase::firestore::bundle::BundleReader;
 using firebase::firestore::bundle::BundleSerializer;
 using firebase::firestore::core::DocumentViewChange;
 using firebase::firestore::core::Query;
 using firebase::firestore::credentials::User;
-using firebase::firestore::google_firestore_v1_ArrayValue;
-using firebase::firestore::google_firestore_v1_Value;
 using firebase::firestore::local::Persistence;
 using firebase::firestore::local::QueryPurpose;
 using firebase::firestore::local::TargetData;
 using firebase::firestore::model::Document;
-using firebase::firestore::model::Document;
 using firebase::firestore::model::DocumentKey;
 using firebase::firestore::model::DocumentKeySet;
 using firebase::firestore::model::MutableDocument;
@@ -117,8 +116,8 @@
 using firebase::firestore::util::Path;
 using firebase::firestore::util::Status;
 using firebase::firestore::util::TimerId;
-using firebase::firestore::util::WrapCompare;
 using firebase::firestore::util::ToString;
+using firebase::firestore::util::WrapCompare;
 
 NS_ASSUME_NONNULL_BEGIN
 
diff --git a/Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.mm b/Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.mm
@@ -60,8 +60,8 @@
 #include "Firestore/core/test/unit/testutil/async_testing.h"
 #include "absl/memory/memory.h"
 
-using firebase::firestore::api::LoadBundleTask;
 using firebase::firestore::Error;
+using firebase::firestore::api::LoadBundleTask;
 using firebase::firestore::bundle::BundleReader;
 using firebase::firestore::core::DatabaseInfo;
 using firebase::firestore::core::EventListener;
@@ -75,9 +75,9 @@
 using firebase::firestore::credentials::EmptyAuthCredentialsProvider;
 using firebase::firestore::credentials::HashUser;
 using firebase::firestore::credentials::User;
-using firebase::firestore::local::QueryEngine;
 using firebase::firestore::local::LocalStore;
 using firebase::firestore::local::Persistence;
+using firebase::firestore::local::QueryEngine;
 using firebase::firestore::local::TargetData;
 using firebase::firestore::model::DatabaseId;
 using firebase::firestore::model::DocumentKey;
@@ -87,9 +87,9 @@
 using firebase::firestore::model::OnlineState;
 using firebase::firestore::model::SnapshotVersion;
 using firebase::firestore::model::TargetId;
+using firebase::firestore::remote::ConnectivityMonitor;
 using firebase::firestore::remote::CreateFirebaseMetadataProviderNoOp;
 using firebase::firestore::remote::CreateNoOpConnectivityMonitor;
-using firebase::firestore::remote::ConnectivityMonitor;
 using firebase::firestore::remote::FirebaseMetadataProvider;
 using firebase::firestore::remote::MockDatastore;
 using firebase::firestore::remote::RemoteStore;
diff --git a/Firestore/Example/Tests/Util/FSTHelpers.mm b/Firestore/Example/Tests/Util/FSTHelpers.mm
@@ -33,16 +33,16 @@
 
 #import "Firestore/core/test/unit/testutil/testutil.h"
 
+using firebase::firestore::google_firestore_v1_Value;
 using firebase::firestore::core::ParsedSetData;
 using firebase::firestore::core::ParsedUpdateData;
-using firebase::firestore::google_firestore_v1_Value;
 using firebase::firestore::model::DatabaseId;
 using firebase::firestore::model::DeleteMutation;
 using firebase::firestore::model::DocumentKey;
 using firebase::firestore::model::FieldPath;
 using firebase::firestore::model::GetTypeOrder;
-using firebase::firestore::model::ObjectValue;
 using firebase::firestore::model::Mutation;
+using firebase::firestore::model::ObjectValue;
 using firebase::firestore::model::PatchMutation;
 using firebase::firestore::model::Precondition;
 using firebase::firestore::model::SetMutation;
diff --git a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm
@@ -57,8 +57,8 @@
 
 using firebase::firestore::core::DatabaseInfo;
 using firebase::firestore::credentials::CredentialChangeListener;
-using firebase::firestore::credentials::EmptyAuthCredentialsProvider;
 using firebase::firestore::credentials::EmptyAppCheckCredentialsProvider;
+using firebase::firestore::credentials::EmptyAuthCredentialsProvider;
 using firebase::firestore::credentials::User;
 using firebase::firestore::local::LevelDbOpener;
 using firebase::firestore::model::DatabaseId;
diff --git a/Firestore/Source/API/FIRDocumentReference.mm b/Firestore/Source/API/FIRDocumentReference.mm
@@ -50,8 +50,8 @@
 using firebase::firestore::api::DocumentSnapshotListener;
 using firebase::firestore::api::Firestore;
 using firebase::firestore::api::ListenerRegistration;
-using firebase::firestore::api::Source;
 using firebase::firestore::api::MakeSource;
+using firebase::firestore::api::Source;
 using firebase::firestore::core::EventListener;
 using firebase::firestore::core::ListenOptions;
 using firebase::firestore::core::ParsedSetData;
@@ -62,7 +62,6 @@
 using firebase::firestore::util::MakeNSString;
 using firebase::firestore::util::MakeString;
 using firebase::firestore::util::StatusOr;
-using firebase::firestore::util::StatusOr;
 using firebase::firestore::util::StatusOrCallback;
 using firebase::firestore::util::ThrowInvalidArgument;
 
diff --git a/Firestore/Source/API/FIRDocumentSnapshot.mm b/Firestore/Source/API/FIRDocumentSnapshot.mm
@@ -56,12 +56,11 @@
 using firebase::firestore::model::DocumentKey;
 using firebase::firestore::model::FieldPath;
 using firebase::firestore::model::ObjectValue;
-using firebase::firestore::remote::Serializer;
 using firebase::firestore::nanopb::MakeNSData;
+using firebase::firestore::remote::Serializer;
 using firebase::firestore::util::MakeNSString;
 using firebase::firestore::util::MakeString;
 using firebase::firestore::util::ThrowInvalidArgument;
-using firebase::firestore::google_firestore_v1_Value;
 
 NS_ASSUME_NONNULL_BEGIN
 
diff --git a/Firestore/Source/API/FIRFilter.mm b/Firestore/Source/API/FIRFilter.mm
@@ -17,10 +17,10 @@
 #import "FIRFilter+Internal.h"
 #import "Firestore/Source/API/FIRFieldPath+Internal.h"
 
+using firebase::firestore::core::CompositeFilter;
+using firebase::firestore::core::FieldFilter;
 using firebase::firestore::model::FieldPath;
 using firebase::firestore::util::MakeString;
-using firebase::firestore::core::FieldFilter;
-using firebase::firestore::core::CompositeFilter;
 
 NS_ASSUME_NONNULL_BEGIN
 
diff --git a/Firestore/Source/API/FIRGeoPoint.mm b/Firestore/Source/API/FIRGeoPoint.mm
@@ -20,9 +20,9 @@
 #include "Firestore/core/src/util/comparison.h"
 #include "Firestore/core/src/util/exception.h"
 
-using firebase::firestore::util::ThrowInvalidArgument;
 using firebase::firestore::util::DoubleBitwiseEquals;
 using firebase::firestore::util::DoubleBitwiseHash;
+using firebase::firestore::util::ThrowInvalidArgument;
 using firebase::firestore::util::WrapCompare;
 
 NS_ASSUME_NONNULL_BEGIN
diff --git a/Firestore/Source/API/FIRQuery.mm b/Firestore/Source/API/FIRQuery.mm
@@ -65,6 +65,9 @@
 #include "absl/strings/match.h"
 
 namespace nanopb = firebase::firestore::nanopb;
+using firebase::firestore::google_firestore_v1_ArrayValue;
+using firebase::firestore::google_firestore_v1_Value;
+using firebase::firestore::google_firestore_v1_Value_fields;
 using firebase::firestore::api::Firestore;
 using firebase::firestore::api::Query;
 using firebase::firestore::api::QueryListenerRegistration;
@@ -74,18 +77,15 @@
 using firebase::firestore::api::Source;
 using firebase::firestore::core::AsyncEventListener;
 using firebase::firestore::core::Bound;
+using firebase::firestore::core::CompositeFilter;
 using firebase::firestore::core::Direction;
 using firebase::firestore::core::EventListener;
 using firebase::firestore::core::FieldFilter;
 using firebase::firestore::core::Filter;
-using firebase::firestore::core::CompositeFilter;
 using firebase::firestore::core::ListenOptions;
 using firebase::firestore::core::OrderBy;
 using firebase::firestore::core::QueryListener;
 using firebase::firestore::core::ViewSnapshot;
-using firebase::firestore::google_firestore_v1_ArrayValue;
-using firebase::firestore::google_firestore_v1_Value;
-using firebase::firestore::google_firestore_v1_Value_fields;
 using firebase::firestore::model::DatabaseId;
 using firebase::firestore::model::DeepClone;
 using firebase::firestore::model::Document;
@@ -98,10 +98,10 @@
 using firebase::firestore::model::TypeOrder;
 using firebase::firestore::nanopb::CheckedSize;
 using firebase::firestore::nanopb::MakeArray;
+using firebase::firestore::nanopb::MakeSharedMessage;
 using firebase::firestore::nanopb::MakeString;
 using firebase::firestore::nanopb::Message;
 using firebase::firestore::nanopb::SharedMessage;
-using firebase::firestore::nanopb::MakeSharedMessage;
 using firebase::firestore::util::MakeNSError;
 using firebase::firestore::util::MakeString;
 using firebase::firestore::util::StatusOr;
diff --git a/Firestore/Source/API/FSTUserDataReader.mm b/Firestore/Source/API/FSTUserDataReader.mm
@@ -61,6 +61,13 @@
 using firebase::Timestamp;
 using firebase::TimestampInternal;
 using firebase::firestore::GeoPoint;
+using firebase::firestore::google_firestore_v1_ArrayValue;
+using firebase::firestore::google_firestore_v1_MapValue;
+using firebase::firestore::google_firestore_v1_MapValue_FieldsEntry;
+using firebase::firestore::google_firestore_v1_Value;
+using firebase::firestore::google_protobuf_NullValue_NULL_VALUE;
+using firebase::firestore::google_protobuf_Timestamp;
+using firebase::firestore::google_type_LatLng;
 using firebase::firestore::core::ParseAccumulator;
 using firebase::firestore::core::ParseContext;
 using firebase::firestore::core::ParsedSetData;
@@ -83,15 +90,8 @@
 using firebase::firestore::nanopb::Message;
 using firebase::firestore::remote::Serializer;
 using firebase::firestore::util::MakeString;
-using firebase::firestore::util::ThrowInvalidArgument;
 using firebase::firestore::util::ReadContext;
-using firebase::firestore::google_firestore_v1_Value;
-using firebase::firestore::google_firestore_v1_MapValue;
-using firebase::firestore::google_firestore_v1_ArrayValue;
-using firebase::firestore::google_protobuf_NullValue_NULL_VALUE;
-using firebase::firestore::google_firestore_v1_MapValue_FieldsEntry;
-using firebase::firestore::google_type_LatLng;
-using firebase::firestore::google_protobuf_Timestamp;
+using firebase::firestore::util::ThrowInvalidArgument;
 using nanopb::StringReader;
 
 NS_ASSUME_NONNULL_BEGIN
diff --git a/Firestore/Source/API/FSTUserDataWriter.mm b/Firestore/Source/API/FSTUserDataWriter.mm
@@ -55,8 +55,8 @@
 using model::GetPreviousValue;
 using model::GetTypeOrder;
 using model::TypeOrder;
-using nanopb::MakeByteString;
 using nanopb::MakeBytesArray;
+using nanopb::MakeByteString;
 using nanopb::MakeNSData;
 using nanopb::MakeString;
 using nanopb::MakeStringView;
diff --git a/Firestore/core/src/util/logic_utils.cc b/Firestore/core/src/util/logic_utils.cc
@@ -21,6 +21,7 @@
 
 #include "Firestore/core/src/core/composite_filter.h"
 #include "Firestore/core/src/core/field_filter.h"
+#include "Firestore/core/src/model/value_util.h"
 #include "Firestore/core/src/nanopb/message.h"
 #include "Firestore/core/src/util/hard_assert.h"
 
@@ -267,10 +268,11 @@ Filter LogicUtils::ComputeInExpansion(const Filter& filter) {
       FieldFilter in_filter(filter);
       for (pb_size_t i = 0; i < in_filter.value().array_value.values_count;
            ++i) {
-        expanded_filters.push_back(
-            FieldFilter::Create(in_filter.field(), FieldFilter::Operator::Equal,
-                                nanopb::MakeSharedMessage(
-                                    in_filter.value().array_value.values[i])));
+        expanded_filters.push_back(FieldFilter::Create(
+            in_filter.field(), FieldFilter::Operator::Equal,
+            nanopb::MakeSharedMessage(
+                *model::DeepClone(in_filter.value().array_value.values[i])
+                     .release())));
       }
       return CompositeFilter::Create(std::move(expanded_filters),
                                      CompositeFilter::Operator::Or);
diff --git a/Firestore/core/test/unit/local/query_engine_test.cc b/Firestore/core/test/unit/local/query_engine_test.cc
@@ -929,6 +929,54 @@ TEST_P(QueryEngineTest, OrderByEquality) {
   });
 }
 
+TEST_P(QueryEngineTest, InAndNotInFiltersWithObjectValues) {
+  persistence_->Run("InAndNotInFiltersWithObjectValues", [&] {
+    mutation_queue_->Start();
+    index_manager_->Start();
+
+    MutableDocument doc1 = Doc("coll/1", 1, Map("a", 1, "b", 0));
+    MutableDocument doc2 = Doc("coll/2", 1, Map("b", 1));
+    MutableDocument doc3 = Doc("coll/3", 1, Map("a", 3, "b", 2));
+    MutableDocument doc4 = Doc("coll/4", 1, Map("a", 1, "b", 3));
+    MutableDocument doc5 =
+        Doc("coll/5", 1, Map("a", Array(1, 2), "b", Array(1, Array(2, 3))));
+    MutableDocument doc6 = Doc("coll/6", 1, Map("b", Map("c", 2)));
+    AddDocuments({doc1, doc2, doc3, doc4, doc5, doc6});
+
+    // a IN [1,[1,2]] && b IN [2,3]
+    auto query1 = testutil::Query("coll").AddingFilter(
+        AndFilters({Filter("a", "in", Array(1, Array(1, 2))),
+                    Filter("b", "in", Array(2, 3))}));
+    DocumentSet result1 = ExpectFullCollectionScan<DocumentSet>(
+        [&] { return RunQuery(query1, kMissingLastLimboFreeSnapshot); });
+    EXPECT_EQ(result1, DocSet(query1.Comparator(), {doc4}));
+
+    // a != [1,2] && b IN [1, [1,[2,3]]]
+    auto query2 = testutil::Query("coll").AddingFilter(
+        AndFilters({Filter("a", "not-in", Array(Array(1, 2))),
+                    Filter("b", "in", Array(1, Array(1, Array(2, 3))))}));
+    DocumentSet result2 = ExpectFullCollectionScan<DocumentSet>(
+        [&] { return RunQuery(query2, kMissingLastLimboFreeSnapshot); });
+    EXPECT_EQ(result2, DocSet(query2.Comparator(), {}));
+
+    // a IN [1,[1,2]] || b == {c : 2}
+    auto query3 = testutil::Query("coll").AddingFilter(
+        OrFilters({Filter("a", "in", Array(1, Array(1, 2))),
+                   Filter("b", "in", Array(Map("c", 2)))}));
+    DocumentSet result3 = ExpectFullCollectionScan<DocumentSet>(
+        [&] { return RunQuery(query3, kMissingLastLimboFreeSnapshot); });
+    EXPECT_EQ(result3, DocSet(query3.Comparator(), {doc1, doc4, doc5, doc6}));
+
+    // (a != 1 && a != [1,2]) || (b != [1,[2,3]] && b != {c : 2})
+    auto query4 = testutil::Query("coll").AddingFilter(OrFilters(
+        {Filter("a", "not-in", Array(1, Array(1, 2))),
+         Filter("b", "not-in", Array(Array(1, Array(2, 3)), Map("c", 2)))}));
+    DocumentSet result4 = ExpectFullCollectionScan<DocumentSet>(
+        [&] { return RunQuery(query4, kMissingLastLimboFreeSnapshot); });
+    EXPECT_EQ(result4, DocSet(query4.Comparator(), {doc1, doc3, doc4}));
+  });
+}
+
 }  // namespace local
 }  // namespace firestore
 }  // namespace firebase
