diff --git a/common/src/main/java/dev/cel/common/CelOptions.java b/common/src/main/java/dev/cel/common/CelOptions.java index 3525e45d7..417d4dc9d 100644 --- a/common/src/main/java/dev/cel/common/CelOptions.java +++ b/common/src/main/java/dev/cel/common/CelOptions.java @@ -120,6 +120,8 @@ public enum ProtoUnsetFieldOptions { public abstract boolean enableComprehension(); + public abstract boolean enableTimestampOverflowCheck(); + public abstract int maxRegexProgramSize(); public abstract Builder toBuilder(); @@ -166,6 +168,7 @@ public static Builder newBuilder() { .unwrapWellKnownTypesOnFunctionDispatch(true) .fromProtoUnsetFieldOption(ProtoUnsetFieldOptions.BIND_DEFAULT) .enableComprehension(true) + .enableTimestampOverflowCheck(true) .maxRegexProgramSize(-1); } @@ -529,6 +532,15 @@ public abstract static class Builder { */ public abstract Builder enableJsonFieldNames(boolean value); + /** + * Enable or disable validating that duration values resulting from timestamp arithmetic do not + * overflow 64-bit nanoseconds. Defaults to enabled. + * + *

Disabling this option is an out-of-conformance behavior that suppresses nanosecond + * overflow validation when subtracting timestamps. + */ + public abstract Builder enableTimestampOverflowCheck(boolean value); + public abstract CelOptions build(); } } diff --git a/common/src/main/java/dev/cel/common/internal/ProtoAdapter.java b/common/src/main/java/dev/cel/common/internal/ProtoAdapter.java index 7e3910433..b1b56afe1 100644 --- a/common/src/main/java/dev/cel/common/internal/ProtoAdapter.java +++ b/common/src/main/java/dev/cel/common/internal/ProtoAdapter.java @@ -325,13 +325,6 @@ private BidiConverter fieldToValueConverter(FieldDescriptor fieldDescriptor) { value -> BidiConverter.IDENTITY.backwardConverter().convert(maybeUnwrap(value))); case FLOAT: return unwrapAndConvert(DOUBLE_CONVERTER); - case DOUBLE: - case SFIXED64: - case SINT64: - case INT64: - return BidiConverter.of( - BidiConverter.IDENTITY.forwardConverter(), - value -> BidiConverter.IDENTITY.backwardConverter().convert(maybeUnwrap(value))); case BYTES: if (celOptions.evaluateCanonicalTypesToNativeValues()) { return BidiConverter.of( @@ -342,10 +335,11 @@ private BidiConverter fieldToValueConverter(FieldDescriptor fieldDescriptor) { return BidiConverter.of( BidiConverter.IDENTITY.forwardConverter(), value -> BidiConverter.IDENTITY.backwardConverter().convert(maybeUnwrap(value))); + case DOUBLE: + case SFIXED64: + case SINT64: + case INT64: case STRING: - return BidiConverter.of( - BidiConverter.IDENTITY.forwardConverter(), - value -> BidiConverter.IDENTITY.backwardConverter().convert(maybeUnwrap(value))); case BOOL: return BidiConverter.of( BidiConverter.IDENTITY.forwardConverter(), @@ -353,10 +347,14 @@ private BidiConverter fieldToValueConverter(FieldDescriptor fieldDescriptor) { case ENUM: return BidiConverter.of( value -> (long) ((EnumValueDescriptor) value).getNumber(), - number -> - fieldDescriptor - .getEnumType() - .findValueByNumberCreatingIfUnknown(number.intValue())); + number -> { + if (number > Integer.MAX_VALUE || number < Integer.MIN_VALUE) { + throw new IllegalArgumentException("Enum value out of int32 range: " + number); + } + return fieldDescriptor + .getEnumType() + .findValueByNumberCreatingIfUnknown(number.intValue()); + }); case MESSAGE: return BidiConverter.of( this::adaptProtoToValue, diff --git a/common/src/main/java/dev/cel/common/internal/ProtoTimeUtils.java b/common/src/main/java/dev/cel/common/internal/ProtoTimeUtils.java index 36671842d..a6e98c571 100644 --- a/common/src/main/java/dev/cel/common/internal/ProtoTimeUtils.java +++ b/common/src/main/java/dev/cel/common/internal/ProtoTimeUtils.java @@ -402,10 +402,21 @@ public static Timestamp subtract(Timestamp ts, Duration dur) { /** Calculate the difference between two timestamps. */ public static Duration between(Timestamp from, Timestamp to) { + return between(from, to, /* validateOverflow= */ true); + } + + /** Calculate the difference between two timestamps. */ + public static Duration between(Timestamp from, Timestamp to, boolean validateOverflow) { Instant javaFrom = ProtoTimeUtils.toJavaInstant(checkValid(from)); Instant javaTo = ProtoTimeUtils.toJavaInstant(checkValid(to)); java.time.Duration between = java.time.Duration.between(javaFrom, javaTo); + if (validateOverflow) { + // Call toNanos() to validate 64-bit nanosecond overflow (throws ArithmeticException). + // Suppress unused variable warning as the duration object itself is returned. + @SuppressWarnings("unused") + long unused = between.toNanos(); + } return ProtoTimeUtils.toProtoDuration(between); } diff --git a/conformance/src/test/java/dev/cel/conformance/BUILD.bazel b/conformance/src/test/java/dev/cel/conformance/BUILD.bazel index e9ed58642..e6f67fe66 100644 --- a/conformance/src/test/java/dev/cel/conformance/BUILD.bazel +++ b/conformance/src/test/java/dev/cel/conformance/BUILD.bazel @@ -104,16 +104,12 @@ _ALL_TESTS = [ ] _TESTS_TO_SKIP_LEGACY = [ + # Broken test cases which should be supported. # TODO: Support setting / getting enum values out of the defined enum value range. "enums/legacy_proto2/select_big,select_neg", "enums/legacy_proto2/assign_standalone_int_big,assign_standalone_int_neg", - # TODO: Generate errors on enum value assignment overflows for proto3. - "enums/legacy_proto3/assign_standalone_int_too_big,assign_standalone_int_too_neg", - # TODO: Ensure overflow occurs on conversions of double values which might not work properly on all platforms. - "conversions/int/double_int_min_range", - # TODO: Duration and timestamp operations should error on overflow. - "timestamps/timestamp_range/sub_time_duration_over,sub_time_duration_under", + # TODO: Ensure adding negative duration values is appropriately supported. "timestamps/timestamp_arithmetic/add_time_to_duration_nanos_negative", @@ -154,15 +150,6 @@ _TESTS_TO_SKIP_PLANNER = [ # TODO: Check behavior for go/cpp "basic/functions/unbound_is_runtime_error", - # TODO: Ensure overflow occurs on conversions of double values which might not work properly on all platforms. - "conversions/int/double_int_min_range", - "enums/legacy_proto3/assign_standalone_int_too_big", - "enums/legacy_proto3/assign_standalone_int_too_neg", - - # TODO: Duration and timestamp operations should error on overflow. - "timestamps/timestamp_range/sub_time_duration_over", - "timestamps/timestamp_range/sub_time_duration_under", - # Skip until fixed. "parse/receiver_function_names", diff --git a/runtime/src/main/java/dev/cel/runtime/RuntimeHelpers.java b/runtime/src/main/java/dev/cel/runtime/RuntimeHelpers.java index 0ee7824b7..fb512541f 100644 --- a/runtime/src/main/java/dev/cel/runtime/RuntimeHelpers.java +++ b/runtime/src/main/java/dev/cel/runtime/RuntimeHelpers.java @@ -391,7 +391,7 @@ public static Optional doubleToUnsignedChecked(double v) { public static Optional doubleToLongChecked(double v) { // getExponent of NaN or Infinite values will return a Double.MAX_EXPONENT + 1 (or 128) int exp = Math.getExponent(v); - if (exp >= 63 && v != Math.scalb(-1.0, 63)) { + if (exp >= 63) { return Optional.empty(); } return Optional.of((long) v); diff --git a/runtime/src/main/java/dev/cel/runtime/standard/IntFunction.java b/runtime/src/main/java/dev/cel/runtime/standard/IntFunction.java index 63959af87..77924d873 100644 --- a/runtime/src/main/java/dev/cel/runtime/standard/IntFunction.java +++ b/runtime/src/main/java/dev/cel/runtime/standard/IntFunction.java @@ -82,7 +82,7 @@ public enum IntOverload implements CelStandardOverload { return RuntimeHelpers.doubleToLongChecked(arg) .orElseThrow( () -> - new CelNumericOverflowException("double is out of range for int")); + new CelNumericOverflowException("double is out of range for int")); } return arg.longValue(); })), diff --git a/runtime/src/main/java/dev/cel/runtime/standard/SubtractOperator.java b/runtime/src/main/java/dev/cel/runtime/standard/SubtractOperator.java index 784c46825..822c3066a 100644 --- a/runtime/src/main/java/dev/cel/runtime/standard/SubtractOperator.java +++ b/runtime/src/main/java/dev/cel/runtime/standard/SubtractOperator.java @@ -68,13 +68,33 @@ public enum SubtractOverload implements CelStandardOverload { "subtract_timestamp_timestamp", Instant.class, Instant.class, - (Instant i1, Instant i2) -> java.time.Duration.between(i2, i1)); + (Instant i1, Instant i2) -> { + java.time.Duration between = java.time.Duration.between(i2, i1); + if (celOptions.enableTimestampOverflowCheck()) { + try { + // Call toNanos() to validate 64-bit nanosecond overflow (throws + // ArithmeticException). + @SuppressWarnings("unused") + long unused = between.toNanos(); + } catch (ArithmeticException e) { + throw new CelNumericOverflowException(e); + } + } + return between; + }); } else { return CelFunctionBinding.from( "subtract_timestamp_timestamp", Timestamp.class, Timestamp.class, - (Timestamp t1, Timestamp t2) -> ProtoTimeUtils.between(t2, t1)); + (Timestamp t1, Timestamp t2) -> { + try { + return ProtoTimeUtils.between( + t2, t1, celOptions.enableTimestampOverflowCheck()); + } catch (ArithmeticException e) { + throw new CelNumericOverflowException(e); + } + }); } }), SUBTRACT_TIMESTAMP_DURATION(