From a3acb2ffad180887e6c7a72af353cbced7a0bad7 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Thu, 20 Aug 2026 10:40:16 -0700 Subject: [PATCH] Fix bug in ::Clone impl for repeated fields This would crash if the repeated field was not a message. This wasn't likely to be triggered but possible for interop where cel::Runtime API was used with some (adapted) legacy CelFunction implementations. Add additional coverage. PiperOrigin-RevId: 967916085 --- common/values/parsed_json_list_value_test.cc | 40 +++++++++- common/values/parsed_json_map_value_test.cc | 51 +++++++++++++ common/values/parsed_map_field_value.cc | 3 +- common/values/parsed_map_field_value_test.cc | 42 +++++++++++ common/values/parsed_repeated_field_value.cc | 72 ++++++++++++++++-- .../parsed_repeated_field_value_test.cc | 74 ++++++++++++++++++- 6 files changed, 270 insertions(+), 12 deletions(-) diff --git a/common/values/parsed_json_list_value_test.cc b/common/values/parsed_json_list_value_test.cc index a7ce17410..5fcff20bf 100644 --- a/common/values/parsed_json_list_value_test.cc +++ b/common/values/parsed_json_list_value_test.cc @@ -13,6 +13,7 @@ // limitations under the License. #include +#include #include #include @@ -21,14 +22,15 @@ #include "absl/status/status_matchers.h" #include "absl/status/statusor.h" #include "absl/strings/string_view.h" -#include "absl/types/optional.h" #include "common/memory.h" #include "common/type.h" #include "common/value.h" #include "common/value_kind.h" #include "common/value_testing.h" +#include "internal/parse_text_proto.h" #include "internal/testing.h" #include "cel/expr/conformance/proto3/test_all_types.pb.h" +#include "google/protobuf/arena.h" #include "google/protobuf/io/zero_copy_stream_impl_lite.h" namespace cel { @@ -285,5 +287,41 @@ TEST_F(ParsedJsonListValueTest, Contains_Dynamic) { IsOkAndHolds(BoolValueIs(true))); } +TEST_F(ParsedJsonListValueTest, CloneDefault) { + ParsedJsonListValue value; + EXPECT_FALSE(value.Clone(arena())); +} + +TEST_F(ParsedJsonListValueTest, CloneSameArena) { + ParsedJsonListValue value(DynamicParseTextProto( + R"pb(values { null_value: NULL_VALUE } + values { bool_value: true })pb"), + arena()); + auto cloned = value.Clone(arena()); + EXPECT_THAT( + cloned.Equal(value, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(BoolValueIs(true))); +} + +TEST_F(ParsedJsonListValueTest, CloneDifferentArena) { + google::protobuf::Arena other_arena; + ParsedJsonListValue value( + ::cel::internal::DynamicParseTextProto( + &other_arena, + R"pb(values { null_value: NULL_VALUE } + values { bool_value: true })pb", + descriptor_pool(), message_factory()), + &other_arena); + auto cloned = value.Clone(arena()); + EXPECT_THAT( + cloned.Equal(value, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(BoolValueIs(true))); + EXPECT_EQ(cloned.Size(), 2); + EXPECT_THAT(cloned.Get(0, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(IsNullValue())); + EXPECT_THAT(cloned.Get(1, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(BoolValueIs(true))); +} + } // namespace } // namespace cel diff --git a/common/values/parsed_json_map_value_test.cc b/common/values/parsed_json_map_value_test.cc index acc29deb2..67bddcf8e 100644 --- a/common/values/parsed_json_map_value_test.cc +++ b/common/values/parsed_json_map_value_test.cc @@ -336,5 +336,56 @@ TEST_F(ParsedJsonMapValueTest, NewIterator2) { IsOkAndHolds(Eq(std::nullopt))); } +TEST_F(ParsedJsonMapValueTest, CloneDefault) { + ParsedJsonMapValue value; + EXPECT_FALSE(value.Clone(arena())); +} + +TEST_F(ParsedJsonMapValueTest, CloneSameArena) { + ParsedJsonMapValue value(DynamicParseTextProto(R"pb( + fields { + key: "foo" + value: { null_value: NULL_VALUE } + } + fields { + key: "bar" + value: { bool_value: true } + })pb"), + arena()); + auto cloned = value.Clone(arena()); + EXPECT_THAT( + cloned.Equal(value, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(BoolValueIs(true))); +} + +TEST_F(ParsedJsonMapValueTest, CloneDifferentArena) { + google::protobuf::Arena other_arena; + ParsedJsonMapValue value( + ::cel::internal::DynamicParseTextProto( + &other_arena, + R"pb( + fields { + key: "foo" + value: { null_value: NULL_VALUE } + } + fields { + key: "bar" + value: { bool_value: true } + })pb", + descriptor_pool(), message_factory()), + &other_arena); + auto cloned = value.Clone(arena()); + EXPECT_THAT( + cloned.Equal(value, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(BoolValueIs(true))); + EXPECT_EQ(cloned.Size(), 2); + EXPECT_THAT(cloned.Get(StringValue("foo"), descriptor_pool(), + message_factory(), arena()), + IsOkAndHolds(IsNullValue())); + EXPECT_THAT(cloned.Get(StringValue("bar"), descriptor_pool(), + message_factory(), arena()), + IsOkAndHolds(BoolValueIs(true))); +} + } // namespace } // namespace cel diff --git a/common/values/parsed_map_field_value.cc b/common/values/parsed_map_field_value.cc index 92787a8af..0b2b49a8d 100644 --- a/common/values/parsed_map_field_value.cc +++ b/common/values/parsed_map_field_value.cc @@ -157,9 +157,8 @@ bool ParsedMapFieldValue::IsZeroValue() const { return IsEmpty(); } ParsedMapFieldValue ParsedMapFieldValue::Clone( google::protobuf::Arena* absl_nonnull arena) const { ABSL_DCHECK(arena != nullptr); - ABSL_DCHECK(*this); - if (ABSL_PREDICT_FALSE(field_ == nullptr)) { + if (ABSL_PREDICT_FALSE(!*this)) { return ParsedMapFieldValue(); } if (arena_ == arena) { diff --git a/common/values/parsed_map_field_value_test.cc b/common/values/parsed_map_field_value_test.cc index f55eb65ed..e5f79dc39 100644 --- a/common/values/parsed_map_field_value_test.cc +++ b/common/values/parsed_map_field_value_test.cc @@ -640,5 +640,47 @@ TEST_F(ParsedMapFieldValueTest, NewIterator2) { IsOkAndHolds(Eq(std::nullopt))); } +TEST_F(ParsedMapFieldValueTest, CloneDefault) { + ParsedMapFieldValue value; + EXPECT_FALSE(value.Clone(arena())); +} + +TEST_F(ParsedMapFieldValueTest, CloneSameArena) { + ParsedMapFieldValue value( + DynamicParseTextProto(R"pb( + map_string_string { key: "foo" value: "bar" } + map_string_string { key: "bar" value: "foo" } + )pb"), + DynamicGetField("map_string_string"), arena()); + auto cloned = value.Clone(arena()); + EXPECT_THAT( + cloned.Equal(value, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(BoolValueIs(true))); +} + +TEST_F(ParsedMapFieldValueTest, CloneDifferentArena) { + google::protobuf::Arena other_arena; + ParsedMapFieldValue value( + ::cel::internal::DynamicParseTextProto( + &other_arena, + R"pb( + map_string_string { key: "foo" value: "bar" } + map_string_string { key: "bar" value: "foo" } + )pb", + descriptor_pool(), message_factory()), + DynamicGetField("map_string_string"), &other_arena); + auto cloned = value.Clone(arena()); + EXPECT_THAT( + cloned.Equal(value, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(BoolValueIs(true))); + EXPECT_EQ(cloned.Size(), 2); + EXPECT_THAT(cloned.Get(StringValue("foo"), descriptor_pool(), + message_factory(), arena()), + IsOkAndHolds(StringValueIs("bar"))); + EXPECT_THAT(cloned.Get(StringValue("bar"), descriptor_pool(), + message_factory(), arena()), + IsOkAndHolds(StringValueIs("foo"))); +} + } // namespace } // namespace cel diff --git a/common/values/parsed_repeated_field_value.cc b/common/values/parsed_repeated_field_value.cc index b990d3965..db9a810ff 100644 --- a/common/values/parsed_repeated_field_value.cc +++ b/common/values/parsed_repeated_field_value.cc @@ -15,6 +15,7 @@ #include "common/values/parsed_repeated_field_value.h" #include +#include #include #include #include @@ -147,24 +148,79 @@ absl::Status ParsedRepeatedFieldValue::Equal( bool ParsedRepeatedFieldValue::IsZeroValue() const { return IsEmpty(); } +namespace { + +template +void CopyRepeatedFieldImpl(const google::protobuf::Reflection* absl_nonnull reflection, + const google::protobuf::Message& src_message, + google::protobuf::Message* absl_nonnull dst_message, + const google::protobuf::FieldDescriptor* absl_nonnull field) { + auto src_field = reflection->GetRepeatedFieldRef(src_message, field); + auto dst_field = + reflection->GetMutableRepeatedFieldRef(dst_message, field); + dst_field.CopyFrom(src_field); +} + +void CopyRepeatedField(const google::protobuf::Reflection* absl_nonnull reflection, + const google::protobuf::Message& src_message, + google::protobuf::Message* absl_nonnull dst_message, + const google::protobuf::FieldDescriptor* absl_nonnull field) { + switch (field->cpp_type()) { + case google::protobuf::FieldDescriptor::CPPTYPE_INT32: + CopyRepeatedFieldImpl(reflection, src_message, dst_message, + field); + break; + case google::protobuf::FieldDescriptor::CPPTYPE_INT64: + CopyRepeatedFieldImpl(reflection, src_message, dst_message, + field); + break; + case google::protobuf::FieldDescriptor::CPPTYPE_UINT32: + CopyRepeatedFieldImpl(reflection, src_message, dst_message, + field); + break; + case google::protobuf::FieldDescriptor::CPPTYPE_UINT64: + CopyRepeatedFieldImpl(reflection, src_message, dst_message, + field); + break; + case google::protobuf::FieldDescriptor::CPPTYPE_DOUBLE: + CopyRepeatedFieldImpl(reflection, src_message, dst_message, + field); + break; + case google::protobuf::FieldDescriptor::CPPTYPE_FLOAT: + CopyRepeatedFieldImpl(reflection, src_message, dst_message, field); + break; + case google::protobuf::FieldDescriptor::CPPTYPE_BOOL: + CopyRepeatedFieldImpl(reflection, src_message, dst_message, field); + break; + case google::protobuf::FieldDescriptor::CPPTYPE_ENUM: + CopyRepeatedFieldImpl(reflection, src_message, dst_message, + field); + break; + case google::protobuf::FieldDescriptor::CPPTYPE_STRING: + CopyRepeatedFieldImpl(reflection, src_message, dst_message, + field); + break; + case google::protobuf::FieldDescriptor::CPPTYPE_MESSAGE: + CopyRepeatedFieldImpl(reflection, src_message, + dst_message, field); + break; + } +} + +} // namespace + ParsedRepeatedFieldValue ParsedRepeatedFieldValue::Clone( google::protobuf::Arena* absl_nonnull arena) const { ABSL_DCHECK(arena != nullptr); - ABSL_DCHECK(*this); - if (ABSL_PREDICT_FALSE(field_ == nullptr)) { + if (ABSL_PREDICT_FALSE(!*this)) { return ParsedRepeatedFieldValue(); } if (arena_ == arena) { return *this; } - auto field = message_->GetReflection()->GetRepeatedFieldRef( - *message_, field_); auto* cloned_message = message_->New(arena); - auto cloned_field = - cloned_message->GetReflection() - ->GetMutableRepeatedFieldRef(cloned_message, field_); - cloned_field.CopyFrom(field); + CopyRepeatedField(GetReflection(), *message_, cloned_message, field_); return ParsedRepeatedFieldValue(cloned_message, field_, arena); } diff --git a/common/values/parsed_repeated_field_value_test.cc b/common/values/parsed_repeated_field_value_test.cc index 6941de0e9..a3a80456a 100644 --- a/common/values/parsed_repeated_field_value_test.cc +++ b/common/values/parsed_repeated_field_value_test.cc @@ -13,6 +13,7 @@ // limitations under the License. #include +#include #include #include @@ -22,14 +23,15 @@ #include "absl/status/statusor.h" #include "absl/strings/string_view.h" #include "absl/time/time.h" -#include "absl/types/optional.h" #include "common/memory.h" #include "common/type.h" #include "common/value.h" #include "common/value_kind.h" #include "common/value_testing.h" +#include "internal/parse_text_proto.h" #include "internal/testing.h" #include "cel/expr/conformance/proto3/test_all_types.pb.h" +#include "google/protobuf/arena.h" #include "google/protobuf/io/zero_copy_stream_impl_lite.h" namespace cel { @@ -45,6 +47,7 @@ using ::cel::test::DurationValueIs; using ::cel::test::ErrorValueIs; using ::cel::test::IntValueIs; using ::cel::test::IsNullValue; +using ::cel::test::StringValueIs; using ::cel::test::UintValueIs; using ::testing::_; using ::testing::ElementsAre; @@ -446,5 +449,74 @@ TEST_F(ParsedRepeatedFieldValueTest, Contains) { IsOkAndHolds(BoolValueIs(false))); } +TEST_F(ParsedRepeatedFieldValueTest, CloneDefault) { + ParsedRepeatedFieldValue value; + EXPECT_FALSE(value.Clone(arena())); +} + +TEST_F(ParsedRepeatedFieldValueTest, CloneSameArena) { + ParsedRepeatedFieldValue value( + DynamicParseTextProto(R"pb(repeated_int64: 1 + repeated_int64: 2)pb"), + DynamicGetField("repeated_int64"), arena()); + auto cloned = value.Clone(arena()); + EXPECT_THAT( + cloned.Equal(value, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(BoolValueIs(true))); +} + +TEST_F(ParsedRepeatedFieldValueTest, CloneDifferentArenaInt64) { + google::protobuf::Arena other_arena; + ParsedRepeatedFieldValue value( + ::cel::internal::DynamicParseTextProto( + &other_arena, R"pb(repeated_int64: 1 repeated_int64: 2)pb", + descriptor_pool(), message_factory()), + DynamicGetField("repeated_int64"), &other_arena); + auto cloned = value.Clone(arena()); + EXPECT_THAT( + cloned.Equal(value, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(BoolValueIs(true))); + EXPECT_EQ(cloned.Size(), 2); + EXPECT_THAT(cloned.Get(0, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(IntValueIs(1))); + EXPECT_THAT(cloned.Get(1, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(IntValueIs(2))); +} + +TEST_F(ParsedRepeatedFieldValueTest, CloneDifferentArenaString) { + google::protobuf::Arena other_arena; + ParsedRepeatedFieldValue value( + ::cel::internal::DynamicParseTextProto( + &other_arena, R"pb(repeated_string: "foo" repeated_string: "bar")pb", + descriptor_pool(), message_factory()), + DynamicGetField("repeated_string"), &other_arena); + auto cloned = value.Clone(arena()); + EXPECT_THAT( + cloned.Equal(value, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(BoolValueIs(true))); + EXPECT_EQ(cloned.Size(), 2); + EXPECT_THAT(cloned.Get(0, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(StringValueIs("foo"))); + EXPECT_THAT(cloned.Get(1, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(StringValueIs("bar"))); +} + +TEST_F(ParsedRepeatedFieldValueTest, CloneDifferentArenaMessage) { + google::protobuf::Arena other_arena; + ParsedRepeatedFieldValue value( + ::cel::internal::DynamicParseTextProto( + &other_arena, + R"pb(repeated_nested_message: { bb: 1 } + repeated_nested_message: { bb: 2 })pb", + descriptor_pool(), message_factory()), + DynamicGetField("repeated_nested_message"), + &other_arena); + auto cloned = value.Clone(arena()); + EXPECT_THAT( + cloned.Equal(value, descriptor_pool(), message_factory(), arena()), + IsOkAndHolds(BoolValueIs(true))); + EXPECT_EQ(cloned.Size(), 2); +} + } // namespace } // namespace cel