Skip to content

Fix memory leak from ASAN checks #9750

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 23 commits into from
May 5, 2022
Merged

Fix memory leak from ASAN checks #9750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
d92db19
Upgrade grpc and abseil
wu-hui Mar 21, 2022
edbf7c1
Fixes
wu-hui Mar 21, 2022
0702f75
Pick up new patch version to fix int128.h warning.
wu-hui Mar 23, 2022
4ba0eb8
Full upgrade
wu-hui Mar 21, 2022
72779cb
CMake grpc upgrade to 1.44.0
wu-hui Mar 21, 2022
6492a01
Upgrade googletest while we are at it
wu-hui Apr 18, 2022
daf25bc
Merge branch 'master' into wuandy/GrpcCmake144
wu-hui Apr 18, 2022
9d3916e
Revert spm change.
wu-hui Apr 18, 2022
2a4a082
Merge branch 'master' into wuandy/GrpcCmake144
wu-hui Apr 19, 2022
319576e
Revert googletest.
wu-hui Apr 19, 2022
d41b7f5
Merge branch 'master' into wuandy/GrpcCmake144
wu-hui Apr 29, 2022
ecbaef1
Always use latest googletest
wu-hui Apr 30, 2022
9d6a5fa
Merge branch 'master' into wuandy/GrpcCmake144
wu-hui Apr 30, 2022
30b9d8e
Move sanitizers to Linux
wu-hui May 1, 2022
3be9350
Add macos-latest as well
wu-hui May 1, 2022
b62ce22
macos latest
wu-hui May 1, 2022
3d32085
Mac 12
wu-hui May 1, 2022
c936018
Move to ubuntu
wu-hui May 1, 2022
5f737bb
Fix license
wu-hui May 2, 2022
3cf8dc6
Attempt.
wu-hui May 3, 2022
ad9c2d2
Try fixing memory leaks
wu-hui May 3, 2022
8383906
Merge branch 'master' into origin/wuandy/FixMemoryLeak
wu-hui May 3, 2022
8e60f77
Add comments
wu-hui May 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions Firestore/core/src/model/value_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,26 @@ const char* kRawMaxValueFieldValue = "__max__";
pb_bytes_array_s* kMaxValueFieldValue =
nanopb::MakeBytesArray(kRawMaxValueFieldValue);

/** The special map field value entry of a maximum proto value. */
google_firestore_v1_MapValue_FieldsEntry kMaxValueFieldEntry = {
.key = kMaxValueFieldKey,
.value = {
.which_value_type = google_firestore_v1_Value_string_value_tag,
.string_value = const_cast<pb_bytes_array_t*>(kMaxValueFieldValue)}};

/** The special map value of a maximum proto value. */
_google_firestore_v1_MapValue kMaxValueMapValue = {
.fields_count = 1, .fields = &kMaxValueFieldEntry};

/**
* A maximum value that is larger than any other Firestore values. Underlying it
* is a map value with a special map field that SDK user cannot possibly
* construct.
*/
google_firestore_v1_Value kMaxValue = {
.which_value_type = google_firestore_v1_Value_map_value_tag,
.map_value = kMaxValueMapValue};

} // namespace

using nanopb::Message;
Expand Down Expand Up @@ -684,16 +704,7 @@ bool IsMinValue(const google_firestore_v1_Value& value) {
}

google_firestore_v1_Value MaxValue() {
google_firestore_v1_Value max_value;
max_value.which_value_type = google_firestore_v1_Value_map_value_tag;
max_value.map_value.fields_count = 1;
max_value.map_value.fields =
nanopb::MakeArray<google_firestore_v1_MapValue_FieldsEntry>(1);
max_value.map_value.fields[0].key = kMaxValueFieldKey;
max_value.map_value.fields[0].value.which_value_type =
google_firestore_v1_Value_string_value_tag;
max_value.map_value.fields[0].value.string_value = kMaxValueFieldValue;
return max_value;
return kMaxValue;
}

bool IsMaxValue(const google_firestore_v1_Value& value) {
Expand Down
121 changes: 3 additions & 118 deletions Firestore/core/test/unit/local/local_serializer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,14 +324,13 @@ class LocalSerializerTest : public ::testing::Test {

void ExpectSerializationRoundTrip(const NamedQuery& named_query,
const ::firestore::NamedQuery& proto) {
ByteString bytes = EncodeNamedQuery(&serializer, named_query);
ByteString bytes = EncodeNamedQuery(named_query);
auto actual = ProtobufParse<::firestore::NamedQuery>(bytes);
EXPECT_TRUE(msg_diff.Compare(proto, actual)) << message_differences;
}

ByteString EncodeNamedQuery(local::LocalSerializer* serializer,
const NamedQuery& named_query) {
return MakeByteString(serializer->EncodeNamedQuery(named_query));
ByteString EncodeNamedQuery(const NamedQuery& named_query) {
return MakeByteString(serializer.EncodeNamedQuery(named_query));
}

void ExpectDeserializationRoundTrip(const NamedQuery& named_query,
Expand Down Expand Up @@ -370,120 +369,6 @@ class LocalSerializerTest : public ::testing::Test {
MessageDifferencer msg_diff;
};

// TODO(b/174608374): Remove these tests once we perform a schema migration.
TEST_F(LocalSerializerTest, SetMutationAndTransformMutationAreSquashed) {
::firestore::client::WriteBatch batch_proto{};
batch_proto.set_batch_id(42);
*batch_proto.add_writes() = SetProto();
*batch_proto.add_writes() = LegacyTransformProto();
*batch_proto.mutable_local_write_time() = WriteTimeProto();

ByteString bytes = ProtobufSerialize(batch_proto);
StringReader reader(bytes);
auto message = Message<firestore_client_WriteBatch>::TryParse(&reader);
MutationBatch decoded = serializer.DecodeMutationBatch(&reader, *message);
ASSERT_EQ(1, decoded.mutations().size());
ASSERT_EQ(Mutation::Type::Set, decoded.mutations()[0].type());

Message<google_firestore_v1_Write> encoded{
remote_serializer.EncodeMutation(decoded.mutations()[0])};
ExpectSet(*encoded);
ExpectUpdateTransform(*encoded);
}

// TODO(b/174608374): Remove these tests once we perform a schema migration.
TEST_F(LocalSerializerTest, PatchMutationAndTransformMutationAreSquashed) {
::firestore::client::WriteBatch batch_proto{};
batch_proto.set_batch_id(42);
*batch_proto.add_writes() = PatchProto();
*batch_proto.add_writes() = LegacyTransformProto();
*batch_proto.mutable_local_write_time() = WriteTimeProto();

ByteString bytes = ProtobufSerialize(batch_proto);
StringReader reader(bytes);
auto message = Message<firestore_client_WriteBatch>::TryParse(&reader);
MutationBatch decoded = serializer.DecodeMutationBatch(&reader, *message);
ASSERT_EQ(1, decoded.mutations().size());
ASSERT_EQ(Mutation::Type::Patch, decoded.mutations()[0].type());

Message<google_firestore_v1_Write> encoded{
remote_serializer.EncodeMutation(decoded.mutations()[0])};
ExpectPatch(*encoded);
ExpectUpdateTransform(*encoded);
}

// TODO(b/174608374): Remove these tests once we perform a schema migration.
TEST_F(LocalSerializerTest, TransformAndTransformThrowError) {
::firestore::client::WriteBatch batch_proto{};
batch_proto.set_batch_id(42);
*batch_proto.add_writes() = LegacyTransformProto();
*batch_proto.add_writes() = LegacyTransformProto();
*batch_proto.mutable_local_write_time() = WriteTimeProto();

ByteString bytes = ProtobufSerialize(batch_proto);
StringReader reader(bytes);
auto message = Message<firestore_client_WriteBatch>::TryParse(&reader);
EXPECT_ANY_THROW(serializer.DecodeMutationBatch(&reader, *message));
}

// TODO(b/174608374): Remove these tests once we perform a schema migration.
TEST_F(LocalSerializerTest, DeleteAndTransformThrowError) {
::firestore::client::WriteBatch batch_proto{};
batch_proto.set_batch_id(42);
*batch_proto.add_writes() = DeleteProto();
*batch_proto.add_writes() = LegacyTransformProto();
*batch_proto.mutable_local_write_time() = WriteTimeProto();

ByteString bytes = ProtobufSerialize(batch_proto);
StringReader reader(bytes);
auto message = Message<firestore_client_WriteBatch>::TryParse(&reader);
EXPECT_ANY_THROW(serializer.DecodeMutationBatch(&reader, *message));
}

// TODO(b/174608374): Remove these tests once we perform a schema migration.
TEST_F(LocalSerializerTest, MultipleMutationsAreSquashed) {
::firestore::client::WriteBatch batch_proto{};
batch_proto.set_batch_id(42);
*batch_proto.add_writes() = SetProto();
*batch_proto.add_writes() = SetProto();
*batch_proto.add_writes() = LegacyTransformProto();
*batch_proto.add_writes() = DeleteProto();
*batch_proto.add_writes() = PatchProto();
*batch_proto.add_writes() = LegacyTransformProto();
*batch_proto.add_writes() = PatchProto();
*batch_proto.mutable_local_write_time() = WriteTimeProto();

ByteString bytes = ProtobufSerialize(batch_proto);
StringReader reader(bytes);
auto message = Message<firestore_client_WriteBatch>::TryParse(&reader);
MutationBatch decoded = serializer.DecodeMutationBatch(&reader, *message);
ASSERT_EQ(5, decoded.mutations().size());

Message<google_firestore_v1_Write> encoded{
remote_serializer.EncodeMutation(decoded.mutations()[0])};
ExpectSet(*encoded);
ExpectNoUpdateTransform(*encoded);

encoded =
MakeMessage(remote_serializer.EncodeMutation(decoded.mutations()[1]));
ExpectSet(*encoded);
ExpectUpdateTransform(*encoded);

encoded =
MakeMessage(remote_serializer.EncodeMutation(decoded.mutations()[2]));
ExpectDelete(*encoded);

encoded =
MakeMessage(remote_serializer.EncodeMutation(decoded.mutations()[3]));
ExpectPatch(*encoded);
ExpectUpdateTransform(*encoded);

encoded =
MakeMessage(remote_serializer.EncodeMutation(decoded.mutations()[4]));
ExpectPatch(*encoded);
ExpectNoUpdateTransform(*encoded);
}

TEST_F(LocalSerializerTest, EncodesMutationBatch) {
Mutation base =
PatchMutation(Key("docs/1"), WrapObject("a", "b"), FieldMask{Field("a")},
Expand Down