setString accepts sign characters after decimal point, producing corrupt Decimal values

[jms-bc]

It has previously been raised in #120 that SetString will panic on ".-400000000000000000000000000000000000000". During fuzzing we have discovered that similar inputs via NewFromString can produce corrupt decimal values that result in invalid arithmetic operations.

This is triggered by a value like ".-1" that has (1) no digits before the decimal point, and (2) a sign character after the decimal point. This is due to Decimal.setString's reliance on the downstream parser in BigInt.SetString to handle validation of the mantissa, but they have different sets of valid values.

Cause

setString removes the decimal point by concatenating the characters before and after it. If a sign character (- or +) appears immediately after the dot, it becomes the leading character of the string passed to BigInt.SetString which accepts it. The sign check in setString only inspects the start of the string, so a sign after the dot is never rejected.

If the sign is negative, this produces a Decimal with a negative Coeff, which should never happen and results in a corrupted Decimal with Sign() and Cmp() reporting the decimal's positive sign from Decimal.Negative whilst arithmetic operations use the negative coefficient.

If the sign is positive, BigInt.SetString absorbs the + silently and produces the correct coefficient. However the + character is counted as a fractional digit when computing the exponent which is correspondingly off-by-one. This produces a value 10x smaller than intended, i.e. .+5 parses as 0.05 rather than 0.5.

Proposed Fix

The mantissa should be validated before being passed to BigInt.SetString - after the removal of the decimal point and exponent, the remaining string should contain only ASCII digits:

// Accept only ASCII characters in the range 0-9
for _, c := range s {
    if c < '0' || c > '9' {
        return 0, fmt.Errorf("could not parse mantissa: %s", s)
    }
}

We will file a PR shortly with this fix.

Reproduction

Some sample code to reproduce this behaviour is:

package main

import (
        "fmt"
        "log"

        apd "github.com/cockroachdb/apd/v3"
)

func main() {
        negativeCoeff()
        offByOneExponent()
}

// negativeCoeff demonstrates that ".-1" produces a Decimal whose Coeff is
// negative, causing Sign() and Cmp() to disagree with arithmetic operations
// that use the raw coefficient.
func negativeCoeff() {
        fmt.Println("=== Case 1: negative coefficient (input \".-1\") ===")

        d, _, err := apd.NewFromString(".-1")
        if err != nil {
                log.Fatalf("creating decimal: %v", err)
        }

        fmt.Printf("  String()   = %s\n", d.String())
        fmt.Printf("  Negative   = %v\n", d.Negative)
        fmt.Printf("  Coeff      = %s\n", d.Coeff.String())
        fmt.Printf("  Sign()     = %d\n", d.Sign())

        // Sign() reports +1 (Negative is false), but the coefficient is -1.
        // Adding this "positive" value to 10 should give a value greater than
        // 10, but the negative coefficient makes it 9.99.
        ten := apd.New(10, 0)
        var sum apd.Decimal
        ctx := apd.BaseContext.WithPrecision(16)
        ctx.Add(&sum, ten, d)
        fmt.Printf("  10 + d     = %s\n", sum.String())

        // d.Cmp(0) says d > 0
        zero := apd.New(0, 0)
        fmt.Printf("  Cmp(d, 0)  = %d\n", d.Cmp(zero))
}

// offByOneExponent demonstrates that ".+5" parses as 0.05.
// The "+" is silently absorbed by BigInt.SetString but is counted as a
// fractional digit, making the exponent off by one.
func offByOneExponent() {
        fmt.Println("=== Case 2: off-by-one exponent (input \".+5\") ===")

        d, _, err := apd.NewFromString(".+5")
        if err != nil {
                log.Fatalf("creating decimal: %v", err)
        }

        fmt.Printf("  String()   = %s\n", d.String())
        fmt.Printf("  Exponent   = %d\n", d.Exponent)
        fmt.Printf("  Coeff      = %s\n", d.Coeff.String())

        // .+5 behaves like .05
        fmt.Printf("  Cmp(d, 0.5)  = %d\n", d.Cmp(apd.New(5, -1)))
        fmt.Printf("  Cmp(d, 0.05) = %d\n", d.Cmp(apd.New(5, -2)))
}

With output

=== Case 1: negative coefficient (input ".-1") ===
  String()   = 0.-1
  Negative   = false
  Coeff      = -1
  Sign()     = 1
  10 + d     = 9.99
  Cmp(d, 0)  = 1
=== Case 2: off-by-one exponent (input ".+5") ===
  String()   = 0.05
  Exponent   = -2
  Coeff      = 5
  Cmp(d, 0.5)  = -1
  Cmp(d, 0.05) = 0

[yuzefovich]

fixed via #145