Avoid deeply nested timestamps that can cause stack overflow. (#10916)

Author: ehsannas

Firestore/CHANGELOG.md

@@ -1,5 +1,6 @@
 # 10.7.0
 - [feature] Add support for disjunctions in queries (`OR` queries).
+- [fixed] Fixed stack overflow caused by deeply nested server timestamps.
 
 # 10.6.0
 - [fixed] Fix a potential high memory usage issue.

Firestore/core/src/model/server_timestamp_util.cc

@@ -35,6 +35,17 @@ const char kServerTimestampSentinel[] = "server_timestamp";
 Message<google_firestore_v1_Value> EncodeServerTimestamp(
     const Timestamp& local_write_time,
     absl::optional<google_firestore_v1_Value> previous_value) {
+  // We should avoid storing deeply nested server timestamp map values
+  // because we never use the intermediate "previous values".
+  // For example:
+  // previous: 42L, add: t1, result: t1 -> 42L
+  // previous: t1,  add: t2, result: t2 -> 42L (NOT t2 -> t1 -> 42L)
+  // previous: t2,  add: t3, result: t3 -> 42L (NOT t3 -> t2 -> t1 -> 42L)
+  // `getPreviousValue` recursively traverses server timestamps to find the
+  // least recent Value.
+  if (previous_value.has_value() && IsServerTimestamp(*previous_value)) {
+    previous_value = GetPreviousValue(*previous_value);
+  }
   pb_size_t count = previous_value ? 3 : 2;
 
   Message<google_firestore_v1_Value> result;

Firestore/core/test/unit/local/local_store_test.cc

@@ -16,6 +16,7 @@
 
 #include "Firestore/core/test/unit/local/local_store_test.h"
 
+#include <thread>  // NOLINT(build/c++11)
 #include <unordered_map>
 #include <utility>
 #include <vector>
@@ -34,10 +35,14 @@
 #include "Firestore/core/src/model/document.h"
 #include "Firestore/core/src/model/document_key.h"
 #include "Firestore/core/src/model/field_index.h"
+#include "Firestore/core/src/model/field_mask.h"
+#include "Firestore/core/src/model/field_path.h"
+#include "Firestore/core/src/model/field_transform.h"
 #include "Firestore/core/src/model/mutable_document.h"
 #include "Firestore/core/src/model/mutation.h"
 #include "Firestore/core/src/model/mutation_batch_result.h"
 #include "Firestore/core/src/model/patch_mutation.h"
+#include "Firestore/core/src/model/server_timestamp_util.h"
 #include "Firestore/core/src/model/set_mutation.h"
 #include "Firestore/core/src/model/transform_operation.h"
 #include "Firestore/core/src/remote/existence_filter.h"
@@ -92,6 +97,7 @@ using testutil::Key;
 using testutil::Map;
 using testutil::OverlayTypeMap;
 using testutil::Query;
+using testutil::ServerTimestamp;
 using testutil::UnknownDoc;
 using testutil::UpdateRemoteEvent;
 using testutil::UpdateRemoteEventWithLimboTargets;
@@ -1684,6 +1690,24 @@ TEST_P(LocalStoreTest, PatchMutationLeadsToPatchOverlay) {
       OverlayTypeMap({{Key("foo/baz"), model::Mutation::Type::Patch}}));
 }
 
+TEST_P(LocalStoreTest, DeeplyNestedTimestampDoesNotCauseStackOverflow) {
+  Timestamp timestamp = Timestamp::Now();
+  Message<_google_firestore_v1_Value> initialServerTimestamp =
+      model::EncodeServerTimestamp(timestamp, absl::nullopt);
+  model::FieldPath path = model::FieldPath::FromDotSeparatedString("timestamp");
+  auto makeDeeplyNestedTimestamp = [&]() {
+    for (int i = 0; i < 1000; ++i) {
+      WriteMutation(testutil::MergeMutation(
+          "foo/bar",
+          Map("timestamp",
+              model::EncodeServerTimestamp(timestamp, *initialServerTimestamp)),
+          {path}, {ServerTimestamp("timestamp")}));
+    }
+  };
+  std::thread t(makeDeeplyNestedTimestamp);
+  EXPECT_NO_FATAL_FAILURE(t.join());
+}
+
 }  // namespace local
 }  // namespace firestore
 }  // namespace firebase