From 42ac7cc92b6e75d930817ece02b525f1186189a8 Mon Sep 17 00:00:00 2001 From: Jonathan Tatum Date: Wed, 19 Aug 2026 15:29:33 -0700 Subject: [PATCH] Propagate unsafe bevhavior for arena owned messages. PiperOrigin-RevId: 967441637 --- common/value.cc | 14 ++++++ common/values/parsed_message_value.cc | 2 +- common/values/parsed_message_value.h | 11 +++-- eval/eval/select_step_test.cc | 63 ++++++++++++++------------- runtime/memory_safety_test.cc | 44 +++++++++++++++++++ 5 files changed, 99 insertions(+), 35 deletions(-) diff --git a/common/value.cc b/common/value.cc index fdb18d388..9ea8ec891 100644 --- a/common/value.cc +++ b/common/value.cc @@ -1512,6 +1512,13 @@ Value WrapFieldImpl( ABSL_DCHECK(!IsWellKnownMessageType(message->GetDescriptor())); const auto* reflection = message->GetReflection(); + if (ABSL_PREDICT_FALSE(reflection == nullptr)) { + // This only happens for special implementations of Message that + // should not normally be used with CEL. + return ErrorValue(absl::InvalidArgumentError( + absl::StrCat("failed to get reflection for message type: ", + message->GetDescriptor()->full_name()))); + } if (field->is_map()) { if (reflection->FieldSize(*message, field) == 0) { return MapValue(); @@ -1653,6 +1660,13 @@ Value WrapRepeatedFieldImpl( ABSL_DCHECK(arena != nullptr); const auto* reflection = message->GetReflection(); + if (ABSL_PREDICT_FALSE(reflection == nullptr)) { + // This only happens for special implementations of Message that + // should not normally be used with CEL. + return ErrorValue(absl::InvalidArgumentError( + absl::StrCat("failed to get reflection for message type: ", + message->GetDescriptor()->full_name()))); + } const int size = reflection->FieldSize(*message, field); if (ABSL_PREDICT_FALSE(index < 0 || index >= size)) { return ErrorValue(absl::InvalidArgumentError( diff --git a/common/values/parsed_message_value.cc b/common/values/parsed_message_value.cc index b0e881998..03d2d461d 100644 --- a/common/values/parsed_message_value.cc +++ b/common/values/parsed_message_value.cc @@ -382,7 +382,7 @@ absl::Status ParsedMessageValue::GetField( ABSL_DCHECK(arena != nullptr); ABSL_DCHECK(result != nullptr); - if (arena_ == nullptr) { + if (is_unsafe()) { *result = Value::WrapFieldUnsafe(unboxing_options, value_, field, descriptor_pool, message_factory, arena); } else { diff --git a/common/values/parsed_message_value.h b/common/values/parsed_message_value.h index 2e356d3e8..3cad912e7 100644 --- a/common/values/parsed_message_value.h +++ b/common/values/parsed_message_value.h @@ -192,7 +192,7 @@ class ParsedMessageValue final explicit ParsedMessageValue( const google::protobuf::Message* absl_nonnull value ABSL_ATTRIBUTE_LIFETIME_BOUND) - : value_(value), arena_(value->GetArena()) { + : value_(value), arena_(nullptr) { ABSL_DCHECK(value != nullptr); ABSL_DCHECK(!value_ || !IsWellKnownMessageType(value_->GetDescriptor())) << value_->GetTypeName() << " is a well known type"; @@ -210,9 +210,14 @@ class ParsedMessageValue final return absl::OkStatus(); } + bool is_unsafe() const { return arena_ == nullptr; } + const google::protobuf::Message* absl_nonnull value_; - // Arena that is attributed as owning the value. May be null to indicate that - // the value is managed externally. + + // The arena attributed as Owning this value. Null if the value is created by + // UnsafeParsedMessageValue() or derived from such a value. This is used to + // identify externally managed messages and propagating the unsafe field + // access behavior. google::protobuf::Arena* absl_nullable arena_; }; diff --git a/eval/eval/select_step_test.cc b/eval/eval/select_step_test.cc index 494def40f..f4dc3fcfb 100644 --- a/eval/eval/select_step_test.cc +++ b/eval/eval/select_step_test.cc @@ -92,7 +92,6 @@ class SelectStepTest : public testing::Test { // Helper method. Creates simple pipeline containing Select step and runs it. absl::StatusOr RunExpression(const CelValue target, absl::string_view field, bool test, - absl::string_view unknown_path, RunExpressionOptions options) { ExecutionPath path; @@ -118,6 +117,8 @@ class SelectStepTest : public testing::Test { runtime_options.unknown_processing = cel::UnknownProcessingOptions::kAttributeOnly; } + // Force the creation of a message factory at the env level. + static_cast(env_->MutableMessageFactory()); CelExpressionFlatImpl cel_expr( env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0, env_->type_registry.GetComposedTypeProvider(), @@ -132,35 +133,20 @@ class SelectStepTest : public testing::Test { absl::string_view field, bool test, RunExpressionOptions options) { return RunExpression(CelProtoWrapper::CreateMessage(message, &arena_), - field, test, "", options); + field, test, options); } absl::StatusOr RunExpression(const TestMessage* message, absl::string_view field, bool test, - absl::string_view unknown_path, RunExpressionOptions options) { return RunExpression(CelProtoWrapper::CreateMessage(message, &arena_), - field, test, unknown_path, options); - } - - absl::StatusOr RunExpression(const TestMessage* message, - absl::string_view field, bool test, - RunExpressionOptions options) { - return RunExpression(message, field, test, "", options); + field, test, options); } absl::StatusOr RunExpression(const CelMap* map_value, absl::string_view field, bool test, - absl::string_view unknown_path, RunExpressionOptions options) { - return RunExpression(CelValue::CreateMap(map_value), field, test, - unknown_path, options); - } - - absl::StatusOr RunExpression(const CelMap* map_value, - absl::string_view field, bool test, - RunExpressionOptions options) { - return RunExpression(map_value, field, test, "", options); + return RunExpression(CelValue::CreateMap(map_value), field, test, options); } protected: @@ -189,8 +175,7 @@ TEST_P(SelectStepConformanceTest, SelectTargetNotStructOrMap) { ASSERT_OK_AND_ASSIGN( CelValue result, RunExpression(CelValue::CreateStringView("some_value"), "some_field", - /*test=*/false, - /*unknown_path=*/"", options)); + /*test=*/false, options)); ASSERT_TRUE(result.IsError()); EXPECT_THAT(*result.ErrorOrDie(), @@ -547,17 +532,36 @@ TEST_P(SelectStepConformanceTest, GlobalExtensionsMessageTest) { } TEST_P(SelectStepConformanceTest, GlobalExtensionsMessageUnsetTest) { - TestExtensions exts; + // Implementation details: + // The test environment is a dynamic descriptor pool with the same definition + // as the linked proto. + // Use a dynamic message with the expected factory and pool so we can expect + // the identity of the default message. + const google::protobuf::Descriptor* descriptor = + env_->descriptor_pool->FindMessageTypeByName( + TestExtensions::descriptor()->full_name()); + ASSERT_NE(descriptor, nullptr); + const google::protobuf::FieldDescriptor* field = + env_->descriptor_pool->FindExtensionByPrintableName( + descriptor, "google.api.expr.runtime.nested_ext"); + ASSERT_NE(field, nullptr); + ASSERT_TRUE(field->containing_type() == descriptor); + const auto* prototype = + env_->MutableMessageFactory()->GetPrototype(descriptor); + ASSERT_NE(prototype, nullptr); + const auto* msg_default = &prototype->GetReflection()->GetMessage( + *prototype, field, env_->MutableMessageFactory()); + RunExpressionOptions options; options.enable_unknowns = GetParam(); ASSERT_OK_AND_ASSIGN( CelValue result, - RunExpression(&exts, "google.api.expr.runtime.nested_ext", false, - options)); + RunExpression(CelProtoWrapper::CreateMessage(prototype, &arena_), + "google.api.expr.runtime.nested_ext", false, options)); ASSERT_TRUE(result.IsMessage()); - EXPECT_THAT(result.MessageOrDie(), Eq(&TestExtensions::default_instance())); + EXPECT_THAT(result.MessageOrDie(), Eq(msg_default)); } TEST_P(SelectStepConformanceTest, GlobalExtensionsWrapperTest) { @@ -653,18 +657,15 @@ TEST_P(SelectStepConformanceTest, NullMessageAccessor) { CelValue value = CelValue::CreateMessageWrapper( CelValue::MessageWrapper(&message, TrivialTypeInfo::GetInstance())); - ASSERT_OK_AND_ASSIGN(CelValue result, - RunExpression(value, "message_value", - /*test=*/false, - /*unknown_path=*/"", options)); + ASSERT_OK_AND_ASSIGN(CelValue result, RunExpression(value, "message_value", + /*test=*/false, options)); ASSERT_TRUE(result.IsError()); EXPECT_THAT(*result.ErrorOrDie(), StatusIs(absl::StatusCode::kNotFound)); // same for has ASSERT_OK_AND_ASSIGN(result, RunExpression(value, "message_value", - /*test=*/true, - /*unknown_path=*/"", options)); + /*test=*/true, options)); ASSERT_TRUE(result.IsError()); EXPECT_THAT(*result.ErrorOrDie(), StatusIs(absl::StatusCode::kNotFound)); diff --git a/runtime/memory_safety_test.cc b/runtime/memory_safety_test.cc index a60b4ce60..f2903d820 100644 --- a/runtime/memory_safety_test.cc +++ b/runtime/memory_safety_test.cc @@ -782,6 +782,50 @@ TEST_P(ViewTypesMemorySafetyTest, UnsafeWrappedMessageDifferentArena) { EXPECT_THAT(result_msg, IsSameInstance(&proto)); } +TEST_P(ViewTypesMemorySafetyTest, UnsafeWrappedMessageDifferentExplicitArena) { + // Arrange: create the runtime and expression. + ASSERT_OK_AND_ASSIGN(std::unique_ptr runtime, + ConfigureRuntimeImpl(false, EvaluationOptions())); + constexpr absl::string_view kProtoValue = R"pb( + child { payload { repeated_int32: [ 1, 2, 3 ] } } + payload { repeated_string: [ "foo", "bar", "baz" ] } + )pb"; + + ASSERT_OK_AND_ASSIGN( + ValidationResult validation, + GetCompiler().Compile( + "condition ? nested_test_all_types : NestedTestAllTypes{}")); + ASSERT_TRUE(validation.IsValid()) << validation.FormatError(); + ASSERT_OK_AND_ASSIGN(auto ast, validation.ReleaseAst()); + ASSERT_OK_AND_ASSIGN(std::unique_ptr program, + runtime->CreateProgram(std::move(ast))); + + // Act: wrap the message and evaluate the expression. + // The unsafe version will alias the input message, so caller must ensure + // the input outlives the use of the `Value` rather than assuming it + // is managed by the evaluation arena. + google::protobuf::Arena arena; + google::protobuf::Arena other_arena; + auto* proto = google::protobuf::Arena::Create(&other_arena); + ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(kProtoValue, proto)); + Activation activation; + activation.InsertOrAssignValue("condition", BoolValue(true)); + activation.InsertOrAssignValue( + "nested_test_all_types", + Value::WrapMessageUnsafe(proto, google::protobuf::DescriptorPool::generated_pool(), + google::protobuf::MessageFactory::generated_factory(), + &arena)); + ASSERT_OK_AND_ASSIGN(Value result, program->Evaluate(&arena, activation)); + + // Assert: the result is an alias of the input message. + ASSERT_TRUE(result.IsParsedMessage()); + const ParsedMessageValue& result_msg = result.GetParsedMessage(); + EXPECT_THAT(result_msg, + test::StructValueIs(ParsedProtoStructEquals(kProtoValue))); + EXPECT_EQ(result_msg->GetArena(), &other_arena); + EXPECT_THAT(result_msg, IsSameInstance(proto)); +} + TEST_P(ViewTypesMemorySafetyTest, UnsafeWrappedMessageFields) { // Arrange: create the runtime and expression. ASSERT_OK_AND_ASSIGN(std::unique_ptr runtime,