mirror of
https://github.com/open-policy-agent/opa.git
synced 2026-08-12 19:32:48 -06:00
topdown: fix sum overflow when integer elements fit int64 but the sum does not (#8987)
## Description
`sum` has an integer fast path that accumulates elements into a plain Go
`int`. Any element that fits a machine int takes this path, so a running
total that exceeds int64 wraps silently:
```rego
sum([9223372036854775807, 1]) # -9223372036854775808 (should be 9223372036854775808)
sum({9223372036854775807, 1, 2}) # -9223372036854775806 (should be 9223372036854775810)
```
`plus` is correct for the same values (`9223372036854775807 + 1` is
`9223372036854775808`), so `sum` and `+` disagree.
#8887 (fixes #6281) added exact `big.Int` accumulation for elements that
are individually larger than 64 bits, but that fallback only runs when
an element does not fit a machine int (`n.Int()` fails). When every
element fits int64 and only the running total overflows, the fast path
is still taken and wraps.
## Fix
Guard the fast-path addition and fall back to the existing
`exactIntAccumulate` big.Int path on overflow, the accumulator `product`
already uses. Small-int, float, mixed, and >64-bit-element inputs are
unchanged.
## Test
Extended
`v1/test/cases/testdata/v1/aggregates/test-aggregates-bignum.yaml` with
a case where each element fits int64 but the sum does not: array and set
overflow, negative overflow, an at-limit value that must stay on the
fast path, and a `+` control. Results are rendered with `sprintf`
because the golden-case loader parses expected numbers as float64. The
case fails on `main` and passes with this change. Added the matching
WASM exception (#3711), as #8887 did, since the result exceeds 64 bits.
`go test ./v1/topdown/` passes.
Signed-off-by: Sueun Cho <sueun.dev@gmail.com>
This commit is contained in:
@@ -5,3 +5,4 @@
|
||||
"arithmetic/bignum exact (>64-bit integers through plus, minus, multiply)": "WASM cannot represent integers larger than 64 bits (see https://github.com/open-policy-agent/opa/issues/3711); this change fixes the Go topdown builtins."
|
||||
"aggregates/bignum exact (>64-bit integers through sum and product)": "WASM cannot represent integers larger than 64 bits (see https://github.com/open-policy-agent/opa/issues/3711); this change fixes the Go topdown builtins."
|
||||
"arithmetic/bignum modulo (divisor a nonzero multiple of 2^64)": "WASM cannot represent integers larger than 64 bits (see https://github.com/open-policy-agent/opa/issues/3711); this change fixes the Go topdown builtin."
|
||||
"aggregates/sum integer fast-path overflow (elements fit int64, sum does not)": "WASM cannot represent integers larger than 64 bits (see https://github.com/open-policy-agent/opa/issues/3711); this change fixes the Go topdown builtin."
|
||||
|
||||
@@ -32,3 +32,29 @@ cases:
|
||||
sum_float: "4"
|
||||
sum_mixed: "3.5"
|
||||
product_float: "3"
|
||||
# Every element fits in a machine int, but the running sum does not. The sum
|
||||
# fast path accumulated into a plain int and wrapped silently; it now falls
|
||||
# back to exact big.Int accumulation, matching the plus builtin.
|
||||
- note: "aggregates/sum integer fast-path overflow (elements fit int64, sum does not)"
|
||||
query: data.generated.p = x
|
||||
modules:
|
||||
- |
|
||||
package generated
|
||||
|
||||
p := result if {
|
||||
result := {
|
||||
"sum_array": sprintf("%v", [sum([9223372036854775807, 1])]),
|
||||
"sum_set": sprintf("%v", [sum({9223372036854775807, 1, 2})]),
|
||||
"sum_negative": sprintf("%v", [sum([-9223372036854775808, -1])]),
|
||||
"sum_at_limit": sprintf("%v", [sum([9223372036854775806, 1])]),
|
||||
"plus_control": sprintf("%v", [9223372036854775807 + 1]),
|
||||
}
|
||||
}
|
||||
data: {}
|
||||
want_result:
|
||||
- x:
|
||||
sum_array: "9223372036854775808"
|
||||
sum_set: "9223372036854775810"
|
||||
sum_negative: "-9223372036854775809"
|
||||
sum_at_limit: "9223372036854775807"
|
||||
plus_control: "9223372036854775808"
|
||||
|
||||
@@ -5,6 +5,7 @@
|
||||
package topdown
|
||||
|
||||
import (
|
||||
"math"
|
||||
"math/big"
|
||||
|
||||
"github.com/open-policy-agent/opa/v1/ast"
|
||||
@@ -63,25 +64,38 @@ func exactIntAccumulate(a termIterable, init int64, op func(z, x, y *big.Int) *b
|
||||
return builtins.IntToNumber(acc), true
|
||||
}
|
||||
|
||||
// addInt returns x+y, reporting false if the sum overflows an int so the caller
|
||||
// can fall back to exact big.Int accumulation instead of wrapping silently.
|
||||
func addInt(x, y int) (int, bool) {
|
||||
if (y > 0 && x > math.MaxInt-y) || (y < 0 && x < math.MinInt-y) {
|
||||
return 0, false
|
||||
}
|
||||
return x + y, true
|
||||
}
|
||||
|
||||
func builtinSum(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error {
|
||||
switch a := operands[0].Value.(type) {
|
||||
case *ast.Array:
|
||||
// Fast path for arrays of integers
|
||||
is := 0
|
||||
nonInts := a.Until(func(x *ast.Term) bool {
|
||||
bail := a.Until(func(x *ast.Term) bool {
|
||||
if n, ok := x.Value.(ast.Number); ok {
|
||||
if i, ok := n.Int(); ok {
|
||||
is += i
|
||||
if s, ok := addInt(is, i); ok {
|
||||
is = s
|
||||
return false
|
||||
}
|
||||
}
|
||||
}
|
||||
return true
|
||||
})
|
||||
if !nonInts {
|
||||
if !bail {
|
||||
return iter(ast.InternedTerm(is))
|
||||
}
|
||||
|
||||
// Non-integer values found, so we need to sum as floats.
|
||||
// A non-integer element, or an integer sum that would overflow the
|
||||
// machine int: accumulate on exact big.Ints, falling back to floats for
|
||||
// genuinely non-integer input.
|
||||
if n, ok := exactIntAccumulate(a, 0, (*big.Int).Add); ok {
|
||||
return iter(ast.NewTerm(n))
|
||||
}
|
||||
@@ -103,16 +117,18 @@ func builtinSum(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) err
|
||||
case ast.Set:
|
||||
// Fast path for sets of integers
|
||||
is := 0
|
||||
nonInts := a.Until(func(x *ast.Term) bool {
|
||||
bail := a.Until(func(x *ast.Term) bool {
|
||||
if n, ok := x.Value.(ast.Number); ok {
|
||||
if i, ok := n.Int(); ok {
|
||||
is += i
|
||||
if s, ok := addInt(is, i); ok {
|
||||
is = s
|
||||
return false
|
||||
}
|
||||
}
|
||||
}
|
||||
return true
|
||||
})
|
||||
if !nonInts {
|
||||
if !bail {
|
||||
return iter(ast.InternedTerm(is))
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user