Skip to content

Propagate unsafe bevhavior for arena owned messages. #2218

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
copybara-service merged 1 commit into from
Aug 19, 2026
Merged

Propagate unsafe bevhavior for arena owned messages. #2218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 14 additions & 0 deletions common/value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion common/values/parsed_message_value.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
11 changes: 8 additions & 3 deletions common/values/parsed_message_value.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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_;
};

Expand Down
63 changes: 32 additions & 31 deletions eval/eval/select_step_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ class SelectStepTest : public testing::Test {
// Helper method. Creates simple pipeline containing Select step and runs it.
absl::StatusOr<CelValue> RunExpression(const CelValue target,
absl::string_view field, bool test,
absl::string_view unknown_path,
RunExpressionOptions options) {
ExecutionPath path;

Expand All @@ -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<void>(env_->MutableMessageFactory());
CelExpressionFlatImpl cel_expr(
env_, FlatExpression(std::move(path), /*comprehension_slot_count=*/0,
env_->type_registry.GetComposedTypeProvider(),
Expand All @@ -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<CelValue> 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<CelValue> RunExpression(const TestMessage* message,
absl::string_view field, bool test,
RunExpressionOptions options) {
return RunExpression(message, field, test, "", options);
field, test, options);
}

absl::StatusOr<CelValue> 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<CelValue> 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:
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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));
Expand Down
44 changes: 44 additions & 0 deletions runtime/memory_safety_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> 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> 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<NestedTestAllTypes>(&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> runtime,
Expand Down
Loading