diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index 7662f744712..3e2032dc7df 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased - [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. diff --git a/Firestore/core/src/model/server_timestamp_util.cc b/Firestore/core/src/model/server_timestamp_util.cc index d723fcb28b3..80a413d9a5d 100644 --- a/Firestore/core/src/model/server_timestamp_util.cc +++ b/Firestore/core/src/model/server_timestamp_util.cc @@ -35,6 +35,17 @@ const char kServerTimestampSentinel[] = "server_timestamp"; Message EncodeServerTimestamp( const Timestamp& local_write_time, absl::optional 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 result; diff --git a/Firestore/core/test/unit/local/local_store_test.cc b/Firestore/core/test/unit/local/local_store_test.cc index 0c3e1a1e595..76020a5f257 100644 --- a/Firestore/core/test/unit/local/local_store_test.cc +++ b/Firestore/core/test/unit/local/local_store_test.cc @@ -16,6 +16,7 @@ #include "Firestore/core/test/unit/local/local_store_test.h" +#include // NOLINT(build/c++11) #include #include #include @@ -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