Skip to content

Commit e97e327

Browse files
l46kokcopybara-github
authored andcommitted
Fix conformance issues around type conversion overflows and duration subtractions
PiperOrigin-RevId: 955074851
1 parent 0bd9173 commit e97e327

7 files changed

Lines changed: 61 additions & 33 deletions

File tree

common/src/main/java/dev/cel/common/CelOptions.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,8 @@ public enum ProtoUnsetFieldOptions {
118118

119119
public abstract boolean enableComprehension();
120120

121+
public abstract boolean enableTimestampOverflowCheck();
122+
121123
public abstract int maxRegexProgramSize();
122124

123125
public abstract Builder toBuilder();
@@ -163,6 +165,7 @@ public static Builder newBuilder() {
163165
.unwrapWellKnownTypesOnFunctionDispatch(true)
164166
.fromProtoUnsetFieldOption(ProtoUnsetFieldOptions.BIND_DEFAULT)
165167
.enableComprehension(true)
168+
.enableTimestampOverflowCheck(true)
166169
.maxRegexProgramSize(-1);
167170
}
168171

@@ -518,6 +521,15 @@ public abstract static class Builder {
518521
*/
519522
public abstract Builder enableJsonFieldNames(boolean value);
520523

524+
/**
525+
* Enable or disable validating that duration values resulting from timestamp arithmetic do not
526+
* overflow 64-bit nanoseconds. Defaults to enabled.
527+
*
528+
* <p>Disabling this option is an out-of-conformance behavior that suppresses nanosecond overflow
529+
* validation when subtracting timestamps.
530+
*/
531+
public abstract Builder enableTimestampOverflowCheck(boolean value);
532+
521533
public abstract CelOptions build();
522534
}
523535
}

common/src/main/java/dev/cel/common/internal/ProtoAdapter.java

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -325,13 +325,6 @@ private BidiConverter fieldToValueConverter(FieldDescriptor fieldDescriptor) {
325325
value -> BidiConverter.IDENTITY.backwardConverter().convert(maybeUnwrap(value)));
326326
case FLOAT:
327327
return unwrapAndConvert(DOUBLE_CONVERTER);
328-
case DOUBLE:
329-
case SFIXED64:
330-
case SINT64:
331-
case INT64:
332-
return BidiConverter.of(
333-
BidiConverter.IDENTITY.forwardConverter(),
334-
value -> BidiConverter.IDENTITY.backwardConverter().convert(maybeUnwrap(value)));
335328
case BYTES:
336329
if (celOptions.evaluateCanonicalTypesToNativeValues()) {
337330
return BidiConverter.<Object, Object>of(
@@ -342,21 +335,26 @@ private BidiConverter fieldToValueConverter(FieldDescriptor fieldDescriptor) {
342335
return BidiConverter.of(
343336
BidiConverter.IDENTITY.forwardConverter(),
344337
value -> BidiConverter.IDENTITY.backwardConverter().convert(maybeUnwrap(value)));
338+
case DOUBLE:
339+
case SFIXED64:
340+
case SINT64:
341+
case INT64:
345342
case STRING:
346-
return BidiConverter.of(
347-
BidiConverter.IDENTITY.forwardConverter(),
348-
value -> BidiConverter.IDENTITY.backwardConverter().convert(maybeUnwrap(value)));
349343
case BOOL:
350344
return BidiConverter.of(
351345
BidiConverter.IDENTITY.forwardConverter(),
352346
value -> BidiConverter.IDENTITY.backwardConverter().convert(maybeUnwrap(value)));
353347
case ENUM:
354348
return BidiConverter.<Object, Long>of(
355349
value -> (long) ((EnumValueDescriptor) value).getNumber(),
356-
number ->
357-
fieldDescriptor
358-
.getEnumType()
359-
.findValueByNumberCreatingIfUnknown(number.intValue()));
350+
number -> {
351+
if (number > Integer.MAX_VALUE || number < Integer.MIN_VALUE) {
352+
throw new IllegalArgumentException("Enum value out of int32 range: " + number);
353+
}
354+
return fieldDescriptor
355+
.getEnumType()
356+
.findValueByNumberCreatingIfUnknown(number.intValue());
357+
});
360358
case MESSAGE:
361359
return BidiConverter.<MessageOrBuilder, Object>of(
362360
this::adaptProtoToValue,

common/src/main/java/dev/cel/common/internal/ProtoTimeUtils.java

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -402,10 +402,21 @@ public static Timestamp subtract(Timestamp ts, Duration dur) {
402402

403403
/** Calculate the difference between two timestamps. */
404404
public static Duration between(Timestamp from, Timestamp to) {
405+
return between(from, to, /* validateOverflow= */ true);
406+
}
407+
408+
/** Calculate the difference between two timestamps. */
409+
public static Duration between(Timestamp from, Timestamp to, boolean validateOverflow) {
405410
Instant javaFrom = ProtoTimeUtils.toJavaInstant(checkValid(from));
406411
Instant javaTo = ProtoTimeUtils.toJavaInstant(checkValid(to));
407412

408413
java.time.Duration between = java.time.Duration.between(javaFrom, javaTo);
414+
if (validateOverflow) {
415+
// Call toNanos() to validate 64-bit nanosecond overflow (throws ArithmeticException).
416+
// Suppress unused variable warning as the duration object itself is returned.
417+
@SuppressWarnings("unused")
418+
long unused = between.toNanos();
419+
}
409420

410421
return ProtoTimeUtils.toProtoDuration(between);
411422
}

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

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -104,16 +104,12 @@ _ALL_TESTS = [
104104
]
105105

106106
_TESTS_TO_SKIP_LEGACY = [
107+
107108
# Broken test cases which should be supported.
108109
# TODO: Support setting / getting enum values out of the defined enum value range.
109110
"enums/legacy_proto2/select_big,select_neg",
110111
"enums/legacy_proto2/assign_standalone_int_big,assign_standalone_int_neg",
111-
# TODO: Generate errors on enum value assignment overflows for proto3.
112-
"enums/legacy_proto3/assign_standalone_int_too_big,assign_standalone_int_too_neg",
113-
# TODO: Ensure overflow occurs on conversions of double values which might not work properly on all platforms.
114-
"conversions/int/double_int_min_range",
115-
# TODO: Duration and timestamp operations should error on overflow.
116-
"timestamps/timestamp_range/sub_time_duration_over,sub_time_duration_under",
112+
117113
# TODO: Ensure adding negative duration values is appropriately supported.
118114
"timestamps/timestamp_arithmetic/add_time_to_duration_nanos_negative",
119115

@@ -154,15 +150,6 @@ _TESTS_TO_SKIP_PLANNER = [
154150
# TODO: Check behavior for go/cpp
155151
"basic/functions/unbound_is_runtime_error",
156152

157-
# TODO: Ensure overflow occurs on conversions of double values which might not work properly on all platforms.
158-
"conversions/int/double_int_min_range",
159-
"enums/legacy_proto3/assign_standalone_int_too_big",
160-
"enums/legacy_proto3/assign_standalone_int_too_neg",
161-
162-
# TODO: Duration and timestamp operations should error on overflow.
163-
"timestamps/timestamp_range/sub_time_duration_over",
164-
"timestamps/timestamp_range/sub_time_duration_under",
165-
166153
# Skip until fixed.
167154
"parse/receiver_function_names",
168155

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -391,7 +391,7 @@ public static Optional<UnsignedLong> doubleToUnsignedChecked(double v) {
391391
public static Optional<Long> doubleToLongChecked(double v) {
392392
// getExponent of NaN or Infinite values will return a Double.MAX_EXPONENT + 1 (or 128)
393393
int exp = Math.getExponent(v);
394-
if (exp >= 63 && v != Math.scalb(-1.0, 63)) {
394+
if (exp >= 63) {
395395
return Optional.empty();
396396
}
397397
return Optional.of((long) v);

runtime/src/main/java/dev/cel/runtime/standard/IntFunction.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ public enum IntOverload implements CelStandardOverload {
8282
return RuntimeHelpers.doubleToLongChecked(arg)
8383
.orElseThrow(
8484
() ->
85-
new CelNumericOverflowException("double is out of range for int"));
85+
new CelNumericOverflowException("double is out of range for int"));
8686
}
8787
return arg.longValue();
8888
})),

runtime/src/main/java/dev/cel/runtime/standard/SubtractOperator.java

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,13 +68,33 @@ public enum SubtractOverload implements CelStandardOverload {
6868
"subtract_timestamp_timestamp",
6969
Instant.class,
7070
Instant.class,
71-
(Instant i1, Instant i2) -> java.time.Duration.between(i2, i1));
71+
(Instant i1, Instant i2) -> {
72+
java.time.Duration between = java.time.Duration.between(i2, i1);
73+
if (celOptions.enableTimestampOverflowCheck()) {
74+
try {
75+
// Call toNanos() to validate 64-bit nanosecond overflow (throws
76+
// ArithmeticException).
77+
@SuppressWarnings("unused")
78+
long unused = between.toNanos();
79+
} catch (ArithmeticException e) {
80+
throw new CelNumericOverflowException(e);
81+
}
82+
}
83+
return between;
84+
});
7285
} else {
7386
return CelFunctionBinding.from(
7487
"subtract_timestamp_timestamp",
7588
Timestamp.class,
7689
Timestamp.class,
77-
(Timestamp t1, Timestamp t2) -> ProtoTimeUtils.between(t2, t1));
90+
(Timestamp t1, Timestamp t2) -> {
91+
try {
92+
return ProtoTimeUtils.between(
93+
t2, t1, celOptions.enableTimestampOverflowCheck());
94+
} catch (ArithmeticException e) {
95+
throw new CelNumericOverflowException(e);
96+
}
97+
});
7898
}
7999
}),
80100
SUBTRACT_TIMESTAMP_DURATION(

0 commit comments

Comments
 (0)