Fix shared state in Env extension (#334)
* Fix shared state in Env extension
* Tests for variable and type isolation in the environment
diff --git a/cel/BUILD.bazel b/cel/BUILD.bazel
index adafc93..73ee4ef 100644
--- a/cel/BUILD.bazel
+++ b/cel/BUILD.bazel
@@ -52,6 +52,8 @@
"//common/types/ref:go_default_library",
"//common/types/traits:go_default_library",
"//interpreter/functions:go_default_library",
+ "//test/proto2pb:go_default_library",
+ "//test/proto3pb:go_default_library",
"@io_bazel_rules_go//proto/wkt:descriptor_go_proto",
"@org_golang_google_genproto//googleapis/api/expr/v1alpha1:go_default_library",
],
diff --git a/cel/cel_test.go b/cel/cel_test.go
index 0b614a1..bc17c9b 100644
--- a/cel/cel_test.go
+++ b/cel/cel_test.go
@@ -33,6 +33,8 @@
"github.com/google/cel-go/parser"
descpb "github.com/golang/protobuf/protoc-gen-go/descriptor"
+ proto2pb "github.com/google/cel-go/test/proto2pb"
+ proto3pb "github.com/google/cel-go/test/proto3pb"
exprpb "google.golang.org/genproto/googleapis/api/expr/v1alpha1"
)
@@ -703,10 +705,79 @@
decls.NewObjectType("google.api.expr.v1alpha1.Expr"), nil),
),
)
- e2, _ := e.Extend()
+ e2, _ := e.Extend(
+ CustomTypeAdapter(types.DefaultTypeAdapter),
+ Types(&proto3pb.TestAllTypes{}),
+ )
if e == e2 {
t.Error("got object equality, wanted separate objects")
}
+ if e.TypeAdapter() == e2.TypeAdapter() {
+ t.Error("got the same type adapter, wanted isolated instances.")
+ }
+ if e.TypeProvider() == e2.TypeProvider() {
+ t.Error("got the same type provider, wanted isolated instances.")
+ }
+ e3, _ := e2.Extend()
+ if e2.TypeAdapter() != e3.TypeAdapter() {
+ t.Error("got different type adapters, wanted immutable adapter reference")
+ }
+ if e2.TypeProvider() == e3.TypeProvider() {
+ t.Error("got the same type provider, wanted isolated instances.")
+ }
+}
+
+func Test_EnvExtensionIsolation(t *testing.T) {
+ baseEnv, err := NewEnv(
+ Container("google.expr"),
+ Declarations(
+ decls.NewIdent("age", decls.Int, nil),
+ decls.NewIdent("gender", decls.String, nil),
+ decls.NewIdent("country", decls.String, nil),
+ ),
+ )
+ if err != nil {
+ t.Fatal(err)
+ }
+ env1, err := baseEnv.Extend(
+ Types(&proto2pb.TestAllTypes{}),
+ Declarations(decls.NewIdent("name", decls.String, nil)))
+ if err != nil {
+ t.Fatal(err)
+ }
+ env2, err := baseEnv.Extend(
+ Types(&proto3pb.TestAllTypes{}),
+ Declarations(decls.NewIdent("group", decls.String, nil)))
+ if err != nil {
+ t.Fatal(err)
+ }
+ _, issues := env2.Compile(`size(group) > 10
+ && !has(proto3.test.TestAllTypes{}.single_int32)`)
+ if issues.Err() != nil {
+ t.Fatal(issues.Err())
+ }
+ _, issues = env2.Compile(`size(name) > 10`)
+ if issues.Err() == nil {
+ t.Fatal("env2 contains 'name', but should not")
+ }
+ _, issues = env2.Compile(`!has(proto2.test.TestAllTypes{}.single_int32)`)
+ if issues.Err() == nil {
+ t.Fatal("env2 contains 'proto2.test.TestAllTypes', but should not")
+ }
+
+ _, issues = env1.Compile(`size(name) > 10
+ && !has(proto2.test.TestAllTypes{}.single_int32)`)
+ if issues.Err() != nil {
+ t.Fatal(issues.Err())
+ }
+ _, issues = env1.Compile("size(group) > 10")
+ if issues.Err() == nil {
+ t.Fatal("env1 contains 'group', but should not")
+ }
+ _, issues = env1.Compile(`!has(proto3.test.TestAllTypes{}.single_int32)`)
+ if issues.Err() == nil {
+ t.Fatal("env1 contains 'proto3.test.TestAllTypes', but should not")
+ }
}
func Test_ParseAndCheckConcurrently(t *testing.T) {
diff --git a/cel/env.go b/cel/env.go
index 2136b26..7d3756a 100644
--- a/cel/env.go
+++ b/cel/env.go
@@ -205,18 +205,52 @@
}
// Extend the current environment with additional options to produce a new Env.
+//
+// Note, the extended Env value should not share memory with the original. It is possible, however,
+// that a CustomTypeAdapter or CustomTypeProvider options could provide values which are mutable.
+// To ensure separation of state between extended environments either make sure the TypeAdapter and
+// TypeProvider are immutable, or that their underlying implementations are based on the
+// ref.TypeRegistry which provides a Copy method which will be invoked by this method.
func (e *Env) Extend(opts ...EnvOption) (*Env, error) {
if e.chkErr != nil {
return nil, e.chkErr
}
+ // Copy slices.
+ decsCopy := make([]*exprpb.Decl, len(e.declarations))
+ macsCopy := make([]parser.Macro, len(e.macros))
+ progOptsCopy := make([]ProgramOption, len(e.progOpts))
+ copy(decsCopy, e.declarations)
+ copy(macsCopy, e.macros)
+ copy(progOptsCopy, e.progOpts)
+
+ // Copy the adapter / provider if they appear to be mutable.
+ adapter := e.adapter
+ provider := e.provider
+ adapterReg, isAdapterReg := e.adapter.(ref.TypeRegistry)
+ providerReg, isProviderReg := e.provider.(ref.TypeRegistry)
+ // In most cases the provider and adapter will be a ref.TypeRegistry;
+ // however, in the rare cases where they are not, they are assumed to
+ // be immutable. Since it is possible to set the TypeProvider separately
+ // from the TypeAdapter, the possible configurations which could use a
+ // TypeRegistry as the base implementation are captured below.
+ if isAdapterReg && isProviderReg && adapterReg == providerReg {
+ reg := providerReg.Copy()
+ adapter = reg
+ provider = reg
+ } else if isProviderReg {
+ provider = providerReg.Copy()
+ } else if isAdapterReg {
+ adapter = adapterReg.Copy()
+ }
+
ext := &Env{
- declarations: e.declarations,
- adapter: e.adapter,
+ declarations: decsCopy,
+ macros: macsCopy,
+ progOpts: progOptsCopy,
+ adapter: adapter,
enableDynamicAggregateLiterals: e.enableDynamicAggregateLiterals,
- macros: e.macros,
pkg: e.pkg,
- progOpts: e.progOpts,
- provider: e.provider,
+ provider: provider,
}
return ext.configure(opts)
}
diff --git a/common/types/pb/pb.go b/common/types/pb/pb.go
index 629f9be..656d767 100644
--- a/common/types/pb/pb.go
+++ b/common/types/pb/pb.go
@@ -61,6 +61,15 @@
return pbdb
}
+// Copy creates a copy of the current database with its own internal descriptor mapping.
+func (pbdb *Db) Copy() *Db {
+ copy := NewDb()
+ for k, v := range pbdb.revFileDescriptorMap {
+ copy.revFileDescriptorMap[k] = v
+ }
+ return copy
+}
+
// RegisterDescriptor produces a `FileDescription` from a `FileDescriptorProto` and registers the
// message and enum types into the `pb.Db`.
func (pbdb *Db) RegisterDescriptor(fileDesc *descpb.FileDescriptorProto) (*FileDescription, error) {
diff --git a/common/types/provider.go b/common/types/provider.go
index 885f60a..2b14e10 100644
--- a/common/types/provider.go
+++ b/common/types/provider.go
@@ -77,6 +77,19 @@
}
}
+// Copy implements the ref.TypeRegistry interface method which copies the current state of the
+// registry into its own memory space.
+func (p *protoTypeRegistry) Copy() ref.TypeRegistry {
+ copy := &protoTypeRegistry{
+ revTypeMap: make(map[string]ref.Type),
+ pbdb: p.pbdb.Copy(),
+ }
+ for k, v := range p.revTypeMap {
+ copy.revTypeMap[k] = v
+ }
+ return copy
+}
+
func (p *protoTypeRegistry) EnumValue(enumName string) ref.Val {
enumVal, err := p.pbdb.DescribeEnum(enumName)
if err != nil {
diff --git a/common/types/ref/provider.go b/common/types/ref/provider.go
index 825f91b..541dbdb 100644
--- a/common/types/ref/provider.go
+++ b/common/types/ref/provider.go
@@ -78,6 +78,9 @@
// If a type is provided more than once with an alternative definition, the
// call will result in an error.
RegisterType(types ...Type) error
+
+ // Copy the TypeRegistry and return a new registry whose mutable state is isolated.
+ Copy() TypeRegistry
}
// FieldType represents a field's type value and whether that field supports
diff --git a/test/proto2pb/BUILD.bazel b/test/proto2pb/BUILD.bazel
index 57a0620..b6e46a4 100644
--- a/test/proto2pb/BUILD.bazel
+++ b/test/proto2pb/BUILD.bazel
@@ -3,6 +3,7 @@
package(
default_visibility = [
+ "//cel:__subpackages__",
"//checker:__subpackages__",
"//common:__subpackages__",
"//interpreter:__subpackages__",
diff --git a/test/proto3pb/BUILD.bazel b/test/proto3pb/BUILD.bazel
index 9b9035f..210254c 100644
--- a/test/proto3pb/BUILD.bazel
+++ b/test/proto3pb/BUILD.bazel
@@ -3,6 +3,7 @@
package(
default_visibility = [
+ "//cel:__subpackages__",
"//checker:__subpackages__",
"//common:__subpackages__",
"//interpreter:__subpackages__",
