handle scale overflows

[maddyblue]

The below code produces a panic. This is happening because the scale passed is math.MinInt32. When quoRem (dec.go:311) tries to call exp10(-shift), it panics. Here shift is math.MinInt32, and when it takes the negative value of that, it ends up still with math.MinInt32 because of two's complement (see https://play.golang.org/p/9H2UtAurI7).

package main

import (
	"fmt"
	"math"

	inf "gopkg.in/inf.v0"
)

func main() {
	z := new(inf.Dec)
	x := inf.NewDec(1, 0)
	y := inf.NewDec(1, 0)
	z.QuoRound(x, y, math.MinInt32, inf.RoundHalfUp)
	fmt.Println(z)
}

Panics with:

panic: runtime error: index out of range

goroutine 1 [running]:
panic(0x4a91a0, 0xc42000a150)
	/usr/local/go/src/runtime/panic.go:500 +0x1a1
gopkg.in/inf%2ev0.exp10(0x80000000, 0x0)
	/go/src/gopkg.in/inf.v0/dec.go:393 +0x52
gopkg.in/inf%2ev0.(*Dec).quoRem(0xc420013020, 0xc420012f60, 0xc420012fc0, 0x180000000, 0xc42000eb00, 0xc42000eb20, 0x482186, 0x4a1260, 0x1)
	/go/src/gopkg.in/inf.v0/dec.go:311 +0x595
gopkg.in/inf%2ev0.(*Dec).quo(0xc420012f30, 0xc420012f60, 0xc420012fc0, 0x51a1e0, 0xc42000a2f4, 0x51a560, 0xc42000a3c0, 0x1)
	/go/src/gopkg.in/inf.v0/dec.go:264 +0x12b
gopkg.in/inf%2ev0.(*Dec).QuoRound(0xc420012f30, 0xc420012f60, 0xc420012fc0, 0x80000000, 0x51a560, 0xc42000a3c0, 0x0)
	/go/src/gopkg.in/inf.v0/dec.go:257 +0xa1
main.main()
	/go/src/gopkg.in/m.go:14 +0xb1
exit status 2

[speter]

Thanks for the report. Note that inf.Dec in general doesn't handle scale overflows currently. This is because in most practical use cases the scale doesn't go anywhere near Max/MinInt32. (This is why it's just int32, and not int64 or big.Int.) So the question is, are you using MinInt32 in solving some practical problem, or were you just experimenting with boundaries?

Currently on scale overflow most computations just give the wrong result; This is arguably undesirable. I think the correct behavior with scale overflows would be to -- yes -- produce a panic. So if I were to fix this specific case, you would get instead of the current panic something like panic("scale overflow"). But I'm not convinced that doing it for just this case is worthwhile.

[maddyblue]

We use this library in @cockroachdb and do random testing each night. One of the tests generated some arguments that produced this panic (cockroachdb/cockroach#10311). So while we don't explicitly need this use case, we do need a library that doesn't have known panics since a user could at any time craft input and panic the server. Wrapping all call sites to the inf package with defer recover is also not what we'd like to do.

I understand that in order to communicate back to the caller, it's either panic or return an error, which would require an API change for some functions. I prefer errors over panics, even though API changes are unpleasant for users. However since you're already using gopkg.in, maybe it's not a huge leap.

The fix for us is certainly in our code, though. We do some scale calculation during pow, and it's generating some bogus numbers because the inputs are so high, which caused this case to be found. So I'm not convinced this package needs to change, but I thought I'd make you aware.

[speter]

I'm glad that you've managed to resolve this within your code. Thanks for the heads up anyway. It is useful to know about this quirk of the library, even though it is not straightforward to fix it without considering all cases when scale overflows (there is no clear "right result" for this case). Given that adding more panics for scale overflows is controversial, I prefer to leave it as it is for now. (Although I personally don't consider panics during calculations so evil; the [u]int* types and big.Int also panic, at least on division by zero.)

It seems to me that there is a fundamental gap though; your input is untrusted whereas in inf.Dec the input is trusted (although this is not explicitly stated, it is kind of implied by the name; infinite precision implies unbounded computational costs). I understand that there may be demand for a decimal library that works reasonably well with untrusted input, but I think this is best done as a separate package (perhaps as a sub-package, maybe even API compatible). The simplest way to achieve it would be by limiting the precision and scale, as is done in many database systems.

On a related note, while it appears that you're most concerned with panics, it seems to me that users being able to run arbitrarily complex computations could also be a security issue. What is your stance regarding this?

[maddyblue]

I have recently completed a series of PRs in cockroachdb to address the complexity problem. For now I have some hardcoded consts to prevent huge computations that take all the CPU. These have been placed at various loops that have produced these problems. This was in functions like sqrt, pow, log, exp, where the results are generally infinite and we have to make a decision about when to cutoff precision.

I'm not sure if I have anything to recommend about changing this package, though. It's really hard to know where these pain points are until you have some tests that exercise extreme arguments. One thing I did consider is plumbing a context.Context through the whole thing and add cancellation detection in any loop. Again, not sure it's a good idea, but for a library like this it could be.

See: https://github.com/cockroachdb/cockroach/commits/master/pkg/util/decimal (and as of this writing there's another out for review)