Skip to content

Commit ec0f274

Browse files
l46kokcopybara-github
authored andcommitted
Fix unknown merging to be linear in space complexity when referenced in binds
PiperOrigin-RevId: 788995643
1 parent a1d76e4 commit ec0f274

4 files changed

Lines changed: 80 additions & 3 deletions

File tree

runtime/src/main/java/dev/cel/runtime/AccumulatedUnknowns.java

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
* an immutable CelUnknownSet.
2727
*/
2828
final class AccumulatedUnknowns {
29-
29+
private static final int MAX_UNKNOWN_ATTRIBUTE_SIZE = 500_000;
3030
private final List<Long> exprIds;
3131
private final List<CelAttribute> attributes;
3232

@@ -40,6 +40,7 @@ List<CelAttribute> attributes() {
4040

4141
@CanIgnoreReturnValue
4242
AccumulatedUnknowns merge(AccumulatedUnknowns arg) {
43+
enforceMaxAttributeSize(this.attributes, arg.attributes);
4344
this.exprIds.addAll(arg.exprIds);
4445
this.attributes.addAll(arg.attributes);
4546
return this;
@@ -57,6 +58,15 @@ static AccumulatedUnknowns create(Collection<Long> exprIds, Collection<CelAttrib
5758
return new AccumulatedUnknowns(new ArrayList<>(exprIds), new ArrayList<>(attributes));
5859
}
5960

61+
private static void enforceMaxAttributeSize(
62+
List<CelAttribute> lhsAttributes, List<CelAttribute> rhsAttributes) {
63+
int totalSize = lhsAttributes.size() + rhsAttributes.size();
64+
if (totalSize > MAX_UNKNOWN_ATTRIBUTE_SIZE) {
65+
throw new IllegalArgumentException(
66+
"Exceeded maximum allowed unknown attributes: " + totalSize);
67+
}
68+
}
69+
6070
private AccumulatedUnknowns(List<Long> exprIds, List<CelAttribute> attributes) {
6171
this.exprIds = exprIds;
6272
this.attributes = attributes;

runtime/src/main/java/dev/cel/runtime/RuntimeUnknownResolver.java

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ private ScopedResolver(
150150
DefaultInterpreter.IntermediateResult resolveSimpleName(String name, Long exprId) {
151151
DefaultInterpreter.IntermediateResult result = lazyEvalResultCache.get(name);
152152
if (result != null) {
153-
return result;
153+
return copyIfMutable(result);
154154
}
155155
result = shadowedVars.get(name);
156156
if (result != null) {
@@ -161,7 +161,27 @@ DefaultInterpreter.IntermediateResult resolveSimpleName(String name, Long exprId
161161

162162
@Override
163163
void cacheLazilyEvaluatedResult(String name, DefaultInterpreter.IntermediateResult result) {
164-
lazyEvalResultCache.put(name, result);
164+
lazyEvalResultCache.put(name, copyIfMutable(result));
165+
}
166+
167+
/**
168+
* Perform a defensive copy of the intermediate result if it is mutable.
169+
*
170+
* <p>Some internal types are mutable to optimize performance, but this can cause issues when
171+
* the result can be reused in multiple subexpressions due to caching.
172+
*
173+
* <p>Note: this is necessary on both the cache put and get path since the interpreter may use
174+
* the same instance that was cached as a return value.
175+
*/
176+
private static DefaultInterpreter.IntermediateResult copyIfMutable(
177+
DefaultInterpreter.IntermediateResult result) {
178+
if (result.value() instanceof AccumulatedUnknowns) {
179+
AccumulatedUnknowns unknowns = (AccumulatedUnknowns) result.value();
180+
return DefaultInterpreter.IntermediateResult.create(
181+
result.attribute(),
182+
AccumulatedUnknowns.create(unknowns.exprIds(), unknowns.attributes()));
183+
}
184+
return result;
165185
}
166186
}
167187

runtime/src/test/java/dev/cel/runtime/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ java_library(
5252
"//common/values:proto_message_lite_value_provider",
5353
"//compiler",
5454
"//compiler:compiler_builder",
55+
"//extensions",
5556
"//extensions:optional_library",
5657
"//parser:macro",
5758
"//parser:unparser",

runtime/src/test/java/dev/cel/runtime/CelRuntimeTest.java

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,13 @@
4242
import dev.cel.common.ast.CelExpr;
4343
import dev.cel.common.ast.CelExpr.ExprKind.Kind;
4444
import dev.cel.common.types.CelV1AlphaTypes;
45+
import dev.cel.common.types.ListType;
4546
import dev.cel.common.types.SimpleType;
4647
import dev.cel.common.types.StructTypeReference;
4748
import dev.cel.compiler.CelCompiler;
4849
import dev.cel.compiler.CelCompilerFactory;
4950
import dev.cel.expr.conformance.proto3.TestAllTypes;
51+
import dev.cel.extensions.CelExtensions;
5052
import dev.cel.parser.CelStandardMacro;
5153
import dev.cel.parser.CelUnparserFactory;
5254
import java.util.List;
@@ -117,6 +119,50 @@ public void evaluate_v1alpha1CheckedExpr() throws Exception {
117119
assertThat(evaluatedResult).isEqualTo("Hello world!");
118120
}
119121

122+
@Test
123+
// Lazy evaluation result cache doesn't allow references to mutate the cached instance.
124+
@TestParameters(
125+
"{expression: 'cel.bind(x, unknown_attr, (unknown_attr > 0) || [0, 1, 2, 3, 4, 5, 6, 7, 8, 9,"
126+
+ " 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20].exists(i, x + x > 0))'}")
127+
@TestParameters(
128+
"{expression: 'cel.bind(x, unknown_attr, x + x + x + x + x + x + x + x + x + x + x + x + x +"
129+
+ " x + x + x + x + x + x + x + x + x + x + x + x + x + x + x + x + x + x + x + x)'}")
130+
// A new unknown is created per 'x' reference.
131+
@TestParameters(
132+
"{expression: '(my_list.exists(x, (x + x + x + x + x + x + x + x + x + x + x + x + x + x + x"
133+
+ " + x + x + x + x + x + x + x + x + x + x + x + x + x + x + x + x + x + x) > 100) &&"
134+
+ " false) || unknown_attr > 0'}")
135+
public void advanceEvaluation_withUnknownTracking_noSelfReferenceInMerge(String expression)
136+
throws Exception {
137+
Cel cel =
138+
CelFactory.standardCelBuilder()
139+
.setStandardMacros(CelStandardMacro.STANDARD_MACROS)
140+
.addCompilerLibraries(CelExtensions.bindings())
141+
.setContainer(CelContainer.ofName("cel.expr.conformance.test"))
142+
.addVar("unknown_attr", SimpleType.INT)
143+
.addVar("my_list", ListType.create(SimpleType.INT))
144+
.setOptions(CelOptions.current().enableUnknownTracking(true).build())
145+
.build();
146+
147+
CelUnknownSet result =
148+
(CelUnknownSet)
149+
cel.createProgram(cel.compile(expression).getAst())
150+
.advanceEvaluation(
151+
UnknownContext.create(
152+
(String name) -> {
153+
if (name.equals("my_list")) {
154+
return Optional.of(ImmutableList.of(1));
155+
}
156+
return Optional.empty();
157+
},
158+
ImmutableList.of(
159+
CelAttributePattern.create("unknown_attr"),
160+
CelAttributePattern.create("my_list")
161+
.qualify(CelAttribute.Qualifier.ofInt(0)))));
162+
163+
assertThat(result.attributes()).containsExactly(CelAttribute.create("unknown_attr"));
164+
}
165+
120166
@Test
121167
public void newWellKnownTypeMessage_withDifferentDescriptorInstance() throws Exception {
122168
CelCompiler celCompiler =

0 commit comments

Comments
 (0)