Return errors when signed or unsigned integer overflow is detected (#421

) Co-authored-by: Justin King <[email protected]>
google · Mar 25, 2021 · 066a51c · 066a51c
1 parent ac33e83
commit 066a51c
Show file tree

Hide file tree

Showing 5 changed files with 105 additions and 1 deletion.
diff --git a/common/types/int.go b/common/types/int.go
@@ -16,6 +16,7 @@ package types
 
 import (
 	"fmt"
+	"math"
 	"reflect"
 	"strconv"
 	"time"
@@ -63,6 +64,9 @@ func (i Int) Add(other ref.Val) ref.Val {
 	if !ok {
 		return ValOrErr(other, "no such overload")
 	}
+	if (otherInt > 0 && i > math.MaxInt64-otherInt) || (otherInt < 0 && i < math.MinInt64-otherInt) {
+		return NewErr("integer overflow")
+	}
 	return i + otherInt
 }
 
@@ -179,6 +183,10 @@ func (i Int) Divide(other ref.Val) ref.Val {
 	if otherInt == IntZero {
 		return NewErr("divide by zero")
 	}
+	// In twos complement, negating MinInt64 would result in a valid of MaxInt64+1.
+	if i == math.MinInt64 && otherInt == -1 {
+		return NewErr("integer overflow")
+	}
 	return i / otherInt
 }
 
@@ -200,6 +208,10 @@ func (i Int) Modulo(other ref.Val) ref.Val {
 	if otherInt == IntZero {
 		return NewErr("modulus by zero")
 	}
+	// In twos complement, negating MinInt64 would result in a valid of MaxInt64+1.
+	if i == math.MinInt64 && otherInt == -1 {
+		return NewErr("integer overflow")
+	}
 	return i % otherInt
 }
 
@@ -209,11 +221,23 @@ func (i Int) Multiply(other ref.Val) ref.Val {
 	if !ok {
 		return ValOrErr(other, "no such overload")
 	}
+	// Detecting multiplication overflow is more complicated than the others. The first two detect
+	// attempting to negate MinInt64, which would result in MaxInt64+1. The other four detect normal
+	// overflow conditions.
+	if (i == -1 && otherInt == math.MinInt64) || (otherInt == -1 && i == math.MinInt64) ||
+		(i > 0 && otherInt > 0 && i > math.MaxInt64/otherInt) || (i > 0 && otherInt < 0 && otherInt < math.MinInt64/i) ||
+		(i < 0 && otherInt > 0 && i < math.MinInt64/otherInt) || (i < 0 && otherInt < 0 && otherInt < math.MaxInt64/i) {
+		return NewErr("integer overflow")
+	}
 	return i * otherInt
 }
 
 // Negate implements traits.Negater.Negate.
 func (i Int) Negate() ref.Val {
+	// In twos complement, negating MinInt64 would result in a valid of MaxInt64+1.
+	if i == math.MinInt64 {
+		return NewErr("integer overflow")
+	}
 	return -i
 }
 
@@ -223,6 +247,9 @@ func (i Int) Subtract(subtrahend ref.Val) ref.Val {
 	if !ok {
 		return ValOrErr(subtrahend, "no such overload")
 	}
+	if (subtraInt < 0 && i > math.MaxInt64+subtraInt) || (subtraInt > 0 && i < math.MinInt64+subtraInt) {
+		return NewErr("integer overflow")
+	}
 	return i - subtraInt
 }
 

diff --git a/common/types/int_test.go b/common/types/int_test.go
@@ -34,6 +34,18 @@ func TestIntAdd(t *testing.T) {
 	if !IsError(Int(-1).Add(String("-1"))) {
 		t.Error("Adding non-int to int was not an error.")
 	}
+	if lhs, rhs := math.MaxInt64, 1; !IsError(Int(lhs).Add(Int(rhs))) {
+		t.Errorf("Expected adding %d and %d to result in overflow.", lhs, rhs)
+	}
+	if lhs, rhs := math.MinInt64, -1; !IsError(Int(lhs).Add(Int(rhs))) {
+		t.Errorf("Expected adding %d and %d to result in overflow.", lhs, rhs)
+	}
+	if lhs, rhs := math.MaxInt64-1, 1; !Int(lhs).Add(Int(rhs)).Equal(Int(math.MaxInt64)).(Bool) {
+		t.Errorf("Expected adding %d and %d to yield %d", lhs, rhs, math.MaxInt64)
+	}
+	if lhs, rhs := math.MinInt64+1, -1; !Int(lhs).Add(Int(rhs)).Equal(Int(math.MinInt64)).(Bool) {
+		t.Errorf("Expected adding %d and %d to yield %d", lhs, rhs, math.MaxInt64)
+	}
 }
 
 func TestIntCompare(t *testing.T) {
@@ -195,6 +207,9 @@ func TestIntDivide(t *testing.T) {
 	if !IsError(Int(1).Divide(Double(-1))) {
 		t.Error("Division permitted without express type-conversion.")
 	}
+	if lhs, rhs := math.MinInt64, -1; !IsError(Int(lhs).Divide(Int(rhs))) {
+		t.Errorf("Expected dividing %d and %d result in overflow.", lhs, rhs)
+	}
 }
 
 func TestIntEqual(t *testing.T) {
@@ -213,6 +228,9 @@ func TestIntModulo(t *testing.T) {
 	if !IsError(Int(21).Modulo(uintZero)) {
 		t.Error("Modulus permitted between different types without type conversion.")
 	}
+	if lhs, rhs := math.MinInt64, -1; !IsError(Int(lhs).Modulo(Int(rhs))) {
+		t.Errorf("Expected modulo %d and %d result in overflow.", lhs, rhs)
+	}
 }
 
 func TestIntMultiply(t *testing.T) {
@@ -222,12 +240,33 @@ func TestIntMultiply(t *testing.T) {
 	if !IsError(Int(1).Multiply(Double(-4.0))) {
 		t.Error("Multiplication permitted without express type-conversion.")
 	}
+	if lhs, rhs := math.MaxInt64/2, 3; !IsError(Int(lhs).Multiply(Int(rhs))) {
+		t.Errorf("Expected multiplying %d and %d to result in overflow.", lhs, rhs)
+	}
+	if lhs, rhs := math.MinInt64/2, 3; !IsError(Int(lhs).Multiply(Int(rhs))) {
+		t.Errorf("Expected multiplying %d and %d to result in overflow.", lhs, rhs)
+	}
+	if lhs, rhs := math.MaxInt64/2, 2; !Int(lhs).Multiply(Int(rhs)).Equal(Int(math.MaxInt64 - 1)).(Bool) {
+		t.Errorf("Expected multiplying %d and %d to yield %d", lhs, rhs, math.MaxInt64-1)
+	}
+	if lhs, rhs := math.MinInt64/2, 2; !Int(lhs).Multiply(Int(rhs)).Equal(Int(math.MinInt64)).(Bool) {
+		t.Errorf("Expected multiplying %d and %d to yield %d", lhs, rhs, math.MaxInt64)
+	}
+	if lhs, rhs := math.MinInt64, -1; !IsError(Int(lhs).Multiply(Int(rhs))) {
+		t.Errorf("Expected multiplying %d and %d result in overflow.", lhs, rhs)
+	}
 }
 
 func TestIntNegate(t *testing.T) {
 	if !Int(1).Negate().Equal(Int(-1)).(Bool) {
 		t.Error("Negating int value did not succeed")
 	}
+	if v := math.MinInt64; !IsError(Int(v).Negate()) {
+		t.Errorf("Expected negating %d to result in overflow.", v)
+	}
+	if v := math.MaxInt64; !Int(v).Negate().Equal(Int(math.MinInt64 + 1)).(Bool) {
+		t.Errorf("Expected negating %d to yeild %d", v, math.MinInt64+1)
+	}
 }
 
 func TestIntSubtract(t *testing.T) {
@@ -237,4 +276,16 @@ func TestIntSubtract(t *testing.T) {
 	if !IsError(Int(1).Subtract(Uint(1))) {
 		t.Error("Subtraction permitted without express type-conversion.")
 	}
+	if lhs, rhs := math.MaxInt64, -1; !IsError(Int(lhs).Subtract(Int(rhs))) {
+		t.Errorf("Expected subtracting %d and %d to result in overflow.", lhs, rhs)
+	}
+	if lhs, rhs := math.MinInt64, 1; !IsError(Int(lhs).Subtract(Int(rhs))) {
+		t.Errorf("Expected subtracting %d and %d to result in overflow.", lhs, rhs)
+	}
+	if lhs, rhs := math.MaxInt64-1, -1; !Int(lhs).Subtract(Int(rhs)).Equal(Int(math.MaxInt64)).(Bool) {
+		t.Errorf("Expected subtracting %d and %d to yield %d", lhs, rhs, math.MaxInt64)
+	}
+	if lhs, rhs := math.MinInt64+1, 1; !Int(lhs).Subtract(Int(rhs)).Equal(Int(math.MinInt64)).(Bool) {
+		t.Errorf("Expected subtracting %d and %d to yield %d", lhs, rhs, math.MaxInt64)
+	}
 }
diff --git a/common/types/uint.go b/common/types/uint.go
@@ -57,6 +57,9 @@ func (i Uint) Add(other ref.Val) ref.Val {
 	if !ok {
 		return ValOrErr(other, "no such overload")
 	}
+	if otherUint > 0 && i > math.MaxUint64-otherUint {
+		return NewErr("unsigned integer overflow")
+	}
 	return i + otherUint
 }
 
@@ -183,6 +186,9 @@ func (i Uint) Multiply(other ref.Val) ref.Val {
 	if !ok {
 		return ValOrErr(other, "no such overload")
 	}
+	if otherUint != 0 && i > math.MaxUint64/otherUint {
+		return NewErr("unsigned integer overflow")
+	}
 	return i * otherUint
 }
 
@@ -192,6 +198,9 @@ func (i Uint) Subtract(subtrahend ref.Val) ref.Val {
 	if !ok {
 		return ValOrErr(subtrahend, "no such overload")
 	}
+	if subtraUint > i {
+		return NewErr("unsigned integer overflow")
+	}
 	return i - subtraUint
 }
 

diff --git a/common/types/uint_test.go b/common/types/uint_test.go
@@ -33,6 +33,12 @@ func TestUint_Add(t *testing.T) {
 	if !IsError(Uint(1).Add(String("-1"))) {
 		t.Error("Adding non-uint to uint was not an error.")
 	}
+	if lhs, rhs := uint64(math.MaxUint64), 1; !IsError(Uint(lhs).Add(Uint(rhs))) {
+		t.Errorf("Expected adding %d and %d to result in overflow.", lhs, rhs)
+	}
+	if lhs, rhs := uint64(math.MaxUint64-1), 1; !Uint(lhs).Add(Uint(rhs)).Equal(Uint(math.MaxUint64)).(Bool) {
+		t.Errorf("Expected adding %d and %d to yield %d", lhs, rhs, uint64(math.MaxUint64))
+	}
 }
 
 func TestUint_Compare(t *testing.T) {
@@ -193,6 +199,12 @@ func TestUint_Multiply(t *testing.T) {
 	if !IsError(Uint(1).Multiply(Double(-4.0))) {
 		t.Error("Multiplication permitted without express type-conversion.")
 	}
+	if lhs, rhs := uint64(math.MaxUint64/2), 3; !IsError(Uint(lhs).Multiply(Uint(rhs))) {
+		t.Errorf("Expected multiplying %d and %d to result in overflow.", lhs, rhs)
+	}
+	if lhs, rhs := uint64(math.MaxUint64/2), 2; !Uint(lhs).Multiply(Uint(rhs)).Equal(Uint(uint64(math.MaxUint64 - 1))).(Bool) {
+		t.Errorf("Expected multiplying %d and %d to yield %d", lhs, rhs, uint64(math.MaxUint64-1))
+	}
 }
 
 func TestUint_Subtract(t *testing.T) {
@@ -202,4 +214,10 @@ func TestUint_Subtract(t *testing.T) {
 	if !IsError(Uint(1).Subtract(Int(1))) {
 		t.Error("Subtraction permitted without express type-conversion.")
 	}
+	if lhs, rhs := uint64(math.MaxUint64-1), uint64(math.MaxUint64); !IsError(Uint(lhs).Subtract(Uint(rhs))) {
+		t.Errorf("Expected subtracting %d and %d to result in overflow.", lhs, rhs)
+	}
+	if lhs, rhs := uint64(math.MaxUint64), uint64(math.MaxUint64); !Uint(lhs).Subtract(Uint(rhs)).Equal(Uint(0)).(Bool) {
+		t.Errorf("Expected subtracting %d and %d to yield %d", lhs, rhs, 0)
+	}
 }
diff --git a/conformance/BUILD.bazel b/conformance/BUILD.bazel
@@ -15,7 +15,6 @@ sh_test(
         "--skip_test=enums/strong_proto2",
         "--skip_test=enums/strong_proto3",
         "--skip_test=fields/qualified_identifier_resolution/map_key_float,map_key_null,map_value_repeat_key",
-        "--skip_test=integer_math/int64_math/int64_overflow_positive,int64_overflow_negative,uint64_overflow_positive,uint64_overflow_negative,int64_min_negate",
         "--skip_test=timestamps/timestamp_range/from_string_under,add_duration_under,add_duration_over",
         "$(location @com_google_cel_spec//tests/simple:testdata/plumbing.textproto)",
         "$(location @com_google_cel_spec//tests/simple:testdata/basic.textproto)",