Skip to content

Commit c616264

Browse files
jnthntatumcopybara-github
jnthntatum
authored and
copybara-github
committed
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: 967299294
1 parent 00f138c commit c616264

6 files changed

Lines changed: 270 additions & 12 deletions

‎common/values/parsed_json_list_value_test.cc‎

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
// limitations under the License.
1414

1515
#include <cstddef>
16+
#include <optional>
1617
#include <utility>
1718
#include <vector>
1819

@@ -21,14 +22,15 @@
2122
#include "absl/status/status_matchers.h"
2223
#include "absl/status/statusor.h"
2324
#include "absl/strings/string_view.h"
24-
#include "absl/types/optional.h"
2525
#include "common/memory.h"
2626
#include "common/type.h"
2727
#include "common/value.h"
2828
#include "common/value_kind.h"
2929
#include "common/value_testing.h"
30+
#include "internal/parse_text_proto.h"
3031
#include "internal/testing.h"
3132
#include "cel/expr/conformance/proto3/test_all_types.pb.h"
33+
#include "google/protobuf/arena.h"
3234
#include "google/protobuf/io/zero_copy_stream_impl_lite.h"
3335

3436
namespace cel {
@@ -285,5 +287,41 @@ TEST_F(ParsedJsonListValueTest, Contains_Dynamic) {
285287
IsOkAndHolds(BoolValueIs(true)));
286288
}
287289

290+
TEST_F(ParsedJsonListValueTest, CloneDefault) {
291+
ParsedJsonListValue value;
292+
EXPECT_FALSE(value.Clone(arena()));
293+
}
294+
295+
TEST_F(ParsedJsonListValueTest, CloneSameArena) {
296+
ParsedJsonListValue value(DynamicParseTextProto<google::protobuf::ListValue>(
297+
R"pb(values { null_value: NULL_VALUE }
298+
values { bool_value: true })pb"),
299+
arena());
300+
auto cloned = value.Clone(arena());
301+
EXPECT_THAT(
302+
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
303+
IsOkAndHolds(BoolValueIs(true)));
304+
}
305+
306+
TEST_F(ParsedJsonListValueTest, CloneDifferentArena) {
307+
google::protobuf::Arena other_arena;
308+
ParsedJsonListValue value(
309+
::cel::internal::DynamicParseTextProto<google::protobuf::ListValue>(
310+
&other_arena,
311+
R"pb(values { null_value: NULL_VALUE }
312+
values { bool_value: true })pb",
313+
descriptor_pool(), message_factory()),
314+
&other_arena);
315+
auto cloned = value.Clone(arena());
316+
EXPECT_THAT(
317+
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
318+
IsOkAndHolds(BoolValueIs(true)));
319+
EXPECT_EQ(cloned.Size(), 2);
320+
EXPECT_THAT(cloned.Get(0, descriptor_pool(), message_factory(), arena()),
321+
IsOkAndHolds(IsNullValue()));
322+
EXPECT_THAT(cloned.Get(1, descriptor_pool(), message_factory(), arena()),
323+
IsOkAndHolds(BoolValueIs(true)));
324+
}
325+
288326
} // namespace
289327
} // namespace cel

‎common/values/parsed_json_map_value_test.cc‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,5 +336,56 @@ TEST_F(ParsedJsonMapValueTest, NewIterator2) {
336336
IsOkAndHolds(Eq(std::nullopt)));
337337
}
338338

339+
TEST_F(ParsedJsonMapValueTest, CloneDefault) {
340+
ParsedJsonMapValue value;
341+
EXPECT_FALSE(value.Clone(arena()));
342+
}
343+
344+
TEST_F(ParsedJsonMapValueTest, CloneSameArena) {
345+
ParsedJsonMapValue value(DynamicParseTextProto<google::protobuf::Struct>(R"pb(
346+
fields {
347+
key: "foo"
348+
value: { null_value: NULL_VALUE }
349+
}
350+
fields {
351+
key: "bar"
352+
value: { bool_value: true }
353+
})pb"),
354+
arena());
355+
auto cloned = value.Clone(arena());
356+
EXPECT_THAT(
357+
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
358+
IsOkAndHolds(BoolValueIs(true)));
359+
}
360+
361+
TEST_F(ParsedJsonMapValueTest, CloneDifferentArena) {
362+
google::protobuf::Arena other_arena;
363+
ParsedJsonMapValue value(
364+
::cel::internal::DynamicParseTextProto<google::protobuf::Struct>(
365+
&other_arena,
366+
R"pb(
367+
fields {
368+
key: "foo"
369+
value: { null_value: NULL_VALUE }
370+
}
371+
fields {
372+
key: "bar"
373+
value: { bool_value: true }
374+
})pb",
375+
descriptor_pool(), message_factory()),
376+
&other_arena);
377+
auto cloned = value.Clone(arena());
378+
EXPECT_THAT(
379+
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
380+
IsOkAndHolds(BoolValueIs(true)));
381+
EXPECT_EQ(cloned.Size(), 2);
382+
EXPECT_THAT(cloned.Get(StringValue("foo"), descriptor_pool(),
383+
message_factory(), arena()),
384+
IsOkAndHolds(IsNullValue()));
385+
EXPECT_THAT(cloned.Get(StringValue("bar"), descriptor_pool(),
386+
message_factory(), arena()),
387+
IsOkAndHolds(BoolValueIs(true)));
388+
}
389+
339390
} // namespace
340391
} // namespace cel

‎common/values/parsed_map_field_value.cc‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,9 +157,8 @@ bool ParsedMapFieldValue::IsZeroValue() const { return IsEmpty(); }
157157
ParsedMapFieldValue ParsedMapFieldValue::Clone(
158158
google::protobuf::Arena* absl_nonnull arena) const {
159159
ABSL_DCHECK(arena != nullptr);
160-
ABSL_DCHECK(*this);
161160

162-
if (ABSL_PREDICT_FALSE(field_ == nullptr)) {
161+
if (ABSL_PREDICT_FALSE(!*this)) {
163162
return ParsedMapFieldValue();
164163
}
165164
if (arena_ == arena) {

‎common/values/parsed_map_field_value_test.cc‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -640,5 +640,47 @@ TEST_F(ParsedMapFieldValueTest, NewIterator2) {
640640
IsOkAndHolds(Eq(std::nullopt)));
641641
}
642642

643+
TEST_F(ParsedMapFieldValueTest, CloneDefault) {
644+
ParsedMapFieldValue value;
645+
EXPECT_FALSE(value.Clone(arena()));
646+
}
647+
648+
TEST_F(ParsedMapFieldValueTest, CloneSameArena) {
649+
ParsedMapFieldValue value(
650+
DynamicParseTextProto<TestAllTypesProto3>(R"pb(
651+
map_string_string { key: "foo" value: "bar" }
652+
map_string_string { key: "bar" value: "foo" }
653+
)pb"),
654+
DynamicGetField<TestAllTypesProto3>("map_string_string"), arena());
655+
auto cloned = value.Clone(arena());
656+
EXPECT_THAT(
657+
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
658+
IsOkAndHolds(BoolValueIs(true)));
659+
}
660+
661+
TEST_F(ParsedMapFieldValueTest, CloneDifferentArena) {
662+
google::protobuf::Arena other_arena;
663+
ParsedMapFieldValue value(
664+
::cel::internal::DynamicParseTextProto<TestAllTypesProto3>(
665+
&other_arena,
666+
R"pb(
667+
map_string_string { key: "foo" value: "bar" }
668+
map_string_string { key: "bar" value: "foo" }
669+
)pb",
670+
descriptor_pool(), message_factory()),
671+
DynamicGetField<TestAllTypesProto3>("map_string_string"), &other_arena);
672+
auto cloned = value.Clone(arena());
673+
EXPECT_THAT(
674+
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
675+
IsOkAndHolds(BoolValueIs(true)));
676+
EXPECT_EQ(cloned.Size(), 2);
677+
EXPECT_THAT(cloned.Get(StringValue("foo"), descriptor_pool(),
678+
message_factory(), arena()),
679+
IsOkAndHolds(StringValueIs("bar")));
680+
EXPECT_THAT(cloned.Get(StringValue("bar"), descriptor_pool(),
681+
message_factory(), arena()),
682+
IsOkAndHolds(StringValueIs("foo")));
683+
}
684+
643685
} // namespace
644686
} // namespace cel

‎common/values/parsed_repeated_field_value.cc‎

Lines changed: 64 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
#include "common/values/parsed_repeated_field_value.h"
1616

1717
#include <cstddef>
18+
#include <cstdint>
1819
#include <limits>
1920
#include <memory>
2021
#include <string>
@@ -147,24 +148,79 @@ absl::Status ParsedRepeatedFieldValue::Equal(
147148

148149
bool ParsedRepeatedFieldValue::IsZeroValue() const { return IsEmpty(); }
149150

151+
namespace {
152+
153+
template <typename T>
154+
void CopyRepeatedFieldImpl(const google::protobuf::Reflection* absl_nonnull reflection,
155+
const google::protobuf::Message& src_message,
156+
google::protobuf::Message* absl_nonnull dst_message,
157+
const google::protobuf::FieldDescriptor* absl_nonnull field) {
158+
auto src_field = reflection->GetRepeatedFieldRef<T>(src_message, field);
159+
auto dst_field =
160+
reflection->GetMutableRepeatedFieldRef<T>(dst_message, field);
161+
dst_field.CopyFrom(src_field);
162+
}
163+
164+
void CopyRepeatedField(const google::protobuf::Reflection* absl_nonnull reflection,
165+
const google::protobuf::Message& src_message,
166+
google::protobuf::Message* absl_nonnull dst_message,
167+
const google::protobuf::FieldDescriptor* absl_nonnull field) {
168+
switch (field->cpp_type()) {
169+
case google::protobuf::FieldDescriptor::CPPTYPE_INT32:
170+
CopyRepeatedFieldImpl<int32_t>(reflection, src_message, dst_message,
171+
field);
172+
break;
173+
case google::protobuf::FieldDescriptor::CPPTYPE_INT64:
174+
CopyRepeatedFieldImpl<int64_t>(reflection, src_message, dst_message,
175+
field);
176+
break;
177+
case google::protobuf::FieldDescriptor::CPPTYPE_UINT32:
178+
CopyRepeatedFieldImpl<uint32_t>(reflection, src_message, dst_message,
179+
field);
180+
break;
181+
case google::protobuf::FieldDescriptor::CPPTYPE_UINT64:
182+
CopyRepeatedFieldImpl<uint64_t>(reflection, src_message, dst_message,
183+
field);
184+
break;
185+
case google::protobuf::FieldDescriptor::CPPTYPE_DOUBLE:
186+
CopyRepeatedFieldImpl<double>(reflection, src_message, dst_message,
187+
field);
188+
break;
189+
case google::protobuf::FieldDescriptor::CPPTYPE_FLOAT:
190+
CopyRepeatedFieldImpl<float>(reflection, src_message, dst_message, field);
191+
break;
192+
case google::protobuf::FieldDescriptor::CPPTYPE_BOOL:
193+
CopyRepeatedFieldImpl<bool>(reflection, src_message, dst_message, field);
194+
break;
195+
case google::protobuf::FieldDescriptor::CPPTYPE_ENUM:
196+
CopyRepeatedFieldImpl<int32_t>(reflection, src_message, dst_message,
197+
field);
198+
break;
199+
case google::protobuf::FieldDescriptor::CPPTYPE_STRING:
200+
CopyRepeatedFieldImpl<std::string>(reflection, src_message, dst_message,
201+
field);
202+
break;
203+
case google::protobuf::FieldDescriptor::CPPTYPE_MESSAGE:
204+
CopyRepeatedFieldImpl<google::protobuf::Message>(reflection, src_message,
205+
dst_message, field);
206+
break;
207+
}
208+
}
209+
210+
} // namespace
211+
150212
ParsedRepeatedFieldValue ParsedRepeatedFieldValue::Clone(
151213
google::protobuf::Arena* absl_nonnull arena) const {
152214
ABSL_DCHECK(arena != nullptr);
153-
ABSL_DCHECK(*this);
154215

155-
if (ABSL_PREDICT_FALSE(field_ == nullptr)) {
216+
if (ABSL_PREDICT_FALSE(!*this)) {
156217
return ParsedRepeatedFieldValue();
157218
}
158219
if (arena_ == arena) {
159220
return *this;
160221
}
161-
auto field = message_->GetReflection()->GetRepeatedFieldRef<google::protobuf::Message>(
162-
*message_, field_);
163222
auto* cloned_message = message_->New(arena);
164-
auto cloned_field =
165-
cloned_message->GetReflection()
166-
->GetMutableRepeatedFieldRef<google::protobuf::Message>(cloned_message, field_);
167-
cloned_field.CopyFrom(field);
223+
CopyRepeatedField(GetReflection(), *message_, cloned_message, field_);
168224
return ParsedRepeatedFieldValue(cloned_message, field_, arena);
169225
}
170226

‎common/values/parsed_repeated_field_value_test.cc‎

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
// limitations under the License.
1414

1515
#include <cstddef>
16+
#include <optional>
1617
#include <utility>
1718
#include <vector>
1819

@@ -22,14 +23,15 @@
2223
#include "absl/status/statusor.h"
2324
#include "absl/strings/string_view.h"
2425
#include "absl/time/time.h"
25-
#include "absl/types/optional.h"
2626
#include "common/memory.h"
2727
#include "common/type.h"
2828
#include "common/value.h"
2929
#include "common/value_kind.h"
3030
#include "common/value_testing.h"
31+
#include "internal/parse_text_proto.h"
3132
#include "internal/testing.h"
3233
#include "cel/expr/conformance/proto3/test_all_types.pb.h"
34+
#include "google/protobuf/arena.h"
3335
#include "google/protobuf/io/zero_copy_stream_impl_lite.h"
3436

3537
namespace cel {
@@ -45,6 +47,7 @@ using ::cel::test::DurationValueIs;
4547
using ::cel::test::ErrorValueIs;
4648
using ::cel::test::IntValueIs;
4749
using ::cel::test::IsNullValue;
50+
using ::cel::test::StringValueIs;
4851
using ::cel::test::UintValueIs;
4952
using ::testing::_;
5053
using ::testing::ElementsAre;
@@ -446,5 +449,74 @@ TEST_F(ParsedRepeatedFieldValueTest, Contains) {
446449
IsOkAndHolds(BoolValueIs(false)));
447450
}
448451

452+
TEST_F(ParsedRepeatedFieldValueTest, CloneDefault) {
453+
ParsedRepeatedFieldValue value;
454+
EXPECT_FALSE(value.Clone(arena()));
455+
}
456+
457+
TEST_F(ParsedRepeatedFieldValueTest, CloneSameArena) {
458+
ParsedRepeatedFieldValue value(
459+
DynamicParseTextProto<TestAllTypesProto3>(R"pb(repeated_int64: 1
460+
repeated_int64: 2)pb"),
461+
DynamicGetField<TestAllTypesProto3>("repeated_int64"), arena());
462+
auto cloned = value.Clone(arena());
463+
EXPECT_THAT(
464+
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
465+
IsOkAndHolds(BoolValueIs(true)));
466+
}
467+
468+
TEST_F(ParsedRepeatedFieldValueTest, CloneDifferentArenaInt64) {
469+
google::protobuf::Arena other_arena;
470+
ParsedRepeatedFieldValue value(
471+
::cel::internal::DynamicParseTextProto<TestAllTypesProto3>(
472+
&other_arena, R"pb(repeated_int64: 1 repeated_int64: 2)pb",
473+
descriptor_pool(), message_factory()),
474+
DynamicGetField<TestAllTypesProto3>("repeated_int64"), &other_arena);
475+
auto cloned = value.Clone(arena());
476+
EXPECT_THAT(
477+
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
478+
IsOkAndHolds(BoolValueIs(true)));
479+
EXPECT_EQ(cloned.Size(), 2);
480+
EXPECT_THAT(cloned.Get(0, descriptor_pool(), message_factory(), arena()),
481+
IsOkAndHolds(IntValueIs(1)));
482+
EXPECT_THAT(cloned.Get(1, descriptor_pool(), message_factory(), arena()),
483+
IsOkAndHolds(IntValueIs(2)));
484+
}
485+
486+
TEST_F(ParsedRepeatedFieldValueTest, CloneDifferentArenaString) {
487+
google::protobuf::Arena other_arena;
488+
ParsedRepeatedFieldValue value(
489+
::cel::internal::DynamicParseTextProto<TestAllTypesProto3>(
490+
&other_arena, R"pb(repeated_string: "foo" repeated_string: "bar")pb",
491+
descriptor_pool(), message_factory()),
492+
DynamicGetField<TestAllTypesProto3>("repeated_string"), &other_arena);
493+
auto cloned = value.Clone(arena());
494+
EXPECT_THAT(
495+
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
496+
IsOkAndHolds(BoolValueIs(true)));
497+
EXPECT_EQ(cloned.Size(), 2);
498+
EXPECT_THAT(cloned.Get(0, descriptor_pool(), message_factory(), arena()),
499+
IsOkAndHolds(StringValueIs("foo")));
500+
EXPECT_THAT(cloned.Get(1, descriptor_pool(), message_factory(), arena()),
501+
IsOkAndHolds(StringValueIs("bar")));
502+
}
503+
504+
TEST_F(ParsedRepeatedFieldValueTest, CloneDifferentArenaMessage) {
505+
google::protobuf::Arena other_arena;
506+
ParsedRepeatedFieldValue value(
507+
::cel::internal::DynamicParseTextProto<TestAllTypesProto3>(
508+
&other_arena,
509+
R"pb(repeated_nested_message: { bb: 1 }
510+
repeated_nested_message: { bb: 2 })pb",
511+
descriptor_pool(), message_factory()),
512+
DynamicGetField<TestAllTypesProto3>("repeated_nested_message"),
513+
&other_arena);
514+
auto cloned = value.Clone(arena());
515+
EXPECT_THAT(
516+
cloned.Equal(value, descriptor_pool(), message_factory(), arena()),
517+
IsOkAndHolds(BoolValueIs(true)));
518+
EXPECT_EQ(cloned.Size(), 2);
519+
}
520+
449521
} // namespace
450522
} // namespace cel

0 commit comments

Comments
 (0)