Skip to content

Commit

Permalink
decimal: incorrect MP_DECIMAL decoding with scale < 0
Browse files Browse the repository at this point in the history
The `scale` value in `MP_DECIMAL` may be negative [1]. We need
to handle the case.

1. https://www.tarantool.io/en/doc/latest/dev_guide/internals/msgpack_extensions/#the-decimal-type

(cherry picked from 3aeb8c2)
  • Loading branch information
oleg-jukovec committed Aug 3, 2023
1 parent bf27cc9 commit 317d100
Show file tree
Hide file tree
Showing 6 changed files with 80 additions and 35 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ Versioning](http://semver.org/spec/v2.0.0.html) except to the first release.

- Flaky decimal/TestSelect (#300)
- Race condition at roundRobinStrategy.GetNextConnection() (#309)
- Incorrect decoding of an MP_DECIMAL when the `scale` value is negative (#314)

## [1.12.0] - 2023-06-07

Expand Down
44 changes: 17 additions & 27 deletions decimal/bcd.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,11 @@ package decimal
// * An implementation in C language https://github.com/tarantool/decNumber/blob/master/decPacked.c

import (
"bytes"
"fmt"
"strings"

"github.com/vmihailenco/msgpack/v5"
)

const (
Expand Down Expand Up @@ -183,13 +186,17 @@ func encodeStringToBCD(buf string) ([]byte, error) {
// ended by a 4-bit sign nibble in the least significant four bits of the final
// byte. The scale is used (negated) as the exponent of the decimal number.
// Note that zeroes may have a sign and/or a scale.
func decodeStringFromBCD(bcdBuf []byte) (string, error) {
// Index of a byte with scale.
const scaleIdx = 0
scale := int(bcdBuf[scaleIdx])
func decodeStringFromBCD(bcdBuf []byte) (string, int, error) {
// Read scale.
buf := bytes.NewBuffer(bcdBuf)
dec := msgpack.NewDecoder(buf)
scale, err := dec.DecodeInt()
if err != nil {
return "", 0, fmt.Errorf("unable to decode the decimal scale: %w", err)
}

// Get a BCD buffer without scale.
bcdBuf = bcdBuf[scaleIdx+1:]
// Get the data without the scale.
bcdBuf = buf.Bytes()
bufLen := len(bcdBuf)

// Every nibble contains a digit, and the last low nibble contains a
Expand All @@ -204,10 +211,6 @@ func decodeStringFromBCD(bcdBuf []byte) (string, error) {

// Reserve bytes for dot and sign.
numLen := ndigits + 2
// Reserve bytes for zeroes.
if scale >= ndigits {
numLen += scale - ndigits
}

var bld strings.Builder
bld.Grow(numLen)
Expand All @@ -219,26 +222,10 @@ func decodeStringFromBCD(bcdBuf []byte) (string, error) {
bld.WriteByte('-')
}

// Add missing zeroes to the left side when scale is bigger than a
// number of digits and a single missed zero to the right side when
// equal.
if scale > ndigits {
bld.WriteByte('0')
bld.WriteByte('.')
for diff := scale - ndigits; diff > 0; diff-- {
bld.WriteByte('0')
}
} else if scale == ndigits {
bld.WriteByte('0')
}

const MaxDigit = 0x09
// Builds a buffer with symbols of decimal number (digits, dot and sign).
processNibble := func(nibble byte) {
if nibble <= MaxDigit {
if ndigits == scale {
bld.WriteByte('.')
}
bld.WriteByte(nibble + '0')
ndigits--
}
Expand All @@ -254,5 +241,8 @@ func decodeStringFromBCD(bcdBuf []byte) (string, error) {
processNibble(lowNibble)
}

return bld.String(), nil
if bld.Len() == 0 || isNegative[sign] && bld.Len() == 1 {
bld.WriteByte('0')
}
return bld.String(), -1 * scale, nil
}
8 changes: 6 additions & 2 deletions decimal/decimal.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,15 +86,19 @@ func (decNum *Decimal) UnmarshalMsgpack(b []byte) error {
// +--------+-------------------+------------+===============+
// | MP_EXT | length (optional) | MP_DECIMAL | PackedDecimal |
// +--------+-------------------+------------+===============+
digits, err := decodeStringFromBCD(b)
digits, exp, err := decodeStringFromBCD(b)
if err != nil {
return fmt.Errorf("msgpack: can't decode string from BCD buffer (%x): %w", b, err)
}

dec, err := decimal.NewFromString(digits)
*decNum = *NewDecimal(dec)
if err != nil {
return fmt.Errorf("msgpack: can't encode string (%s) to a decimal number: %w", digits, err)
}

if exp != 0 {
dec = dec.Shift(int32(exp))
}
*decNum = *NewDecimal(dec)
return nil
}
49 changes: 48 additions & 1 deletion decimal/decimal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,18 @@ var correctnessSamples = []struct {
{"-1234567891234567890.0987654321987654321", "c7150113012345678912345678900987654321987654321d", false},
}

var correctnessDecodeSamples = []struct {
numString string
mpBuf string
fixExt bool
}{
{"1e2", "d501fe1c", true},
{"1e33", "c70301d0df1c", false},
{"1.1e31", "c70301e2011c", false},
{"13e-2", "c7030102013c", false},
{"-1e3", "d501fd1d", true},
}

// There is a difference between encoding result from a raw string and from
// decimal.Decimal. It's expected because decimal.Decimal simplifies decimals:
// 0.00010000 -> 0.0001
Expand Down Expand Up @@ -384,17 +396,21 @@ func TestEncodeStringToBCD(t *testing.T) {

func TestDecodeStringFromBCD(t *testing.T) {
samples := append(correctnessSamples, rawSamples...)
samples = append(samples, correctnessDecodeSamples...)
samples = append(samples, benchmarkSamples...)
for _, testcase := range samples {
t.Run(testcase.numString, func(t *testing.T) {
b, _ := hex.DecodeString(testcase.mpBuf)
bcdBuf := trimMPHeader(b, testcase.fixExt)
s, err := DecodeStringFromBCD(bcdBuf)
s, exp, err := DecodeStringFromBCD(bcdBuf)
if err != nil {
t.Fatalf("Failed to decode BCD '%x' to decimal: %s", bcdBuf, err)
}

decActual, err := decimal.NewFromString(s)
if exp != 0 {
decActual = decActual.Shift(int32(exp))
}
if err != nil {
t.Fatalf("Failed to encode string ('%s') to decimal", s)
}
Expand Down Expand Up @@ -525,6 +541,37 @@ func TestSelect(t *testing.T) {
tupleValueIsDecimal(t, resp.Data, number)
}

func TestUnmarshal_from_decimal_new(t *testing.T) {
skipIfDecimalUnsupported(t)

conn := test_helpers.ConnectWithValidation(t, server, opts)
defer conn.Close()

samples := correctnessSamples
samples = append(samples, correctnessDecodeSamples...)
samples = append(samples, benchmarkSamples...)
for _, testcase := range samples {
str := testcase.numString
t.Run(str, func(t *testing.T) {
number, err := decimal.NewFromString(str)
if err != nil {
t.Fatalf("Failed to prepare test decimal: %s", err)
}

call := NewEvalRequest("return require('decimal').new(...)").
Args([]interface{}{str})
resp, err := conn.Do(call).Get()
if err != nil {
t.Fatalf("Decimal create failed: %s", err)
}
if resp == nil {
t.Fatalf("Response is nil after Call")
}
tupleValueIsDecimal(t, []interface{}{resp.Data}, number)
})
}
}

func assertInsert(t *testing.T, conn *Connection, numString string) {
number, err := decimal.NewFromString(numString)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion decimal/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ func EncodeStringToBCD(buf string) ([]byte, error) {
return encodeStringToBCD(buf)
}

func DecodeStringFromBCD(bcdBuf []byte) (string, error) {
func DecodeStringFromBCD(bcdBuf []byte) (string, int, error) {
return decodeStringFromBCD(bcdBuf)
}

Expand Down
11 changes: 7 additions & 4 deletions decimal/fuzzing_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,14 @@ import (
. "github.com/tarantool/go-tarantool/decimal"
)

func strToDecimal(t *testing.T, buf string) decimal.Decimal {
func strToDecimal(t *testing.T, buf string, exp int) decimal.Decimal {
decNum, err := decimal.NewFromString(buf)
if err != nil {
t.Fatal(err)
}
if exp != 0 {
decNum = decNum.Shift(int32(exp))
}
return decNum
}

Expand All @@ -33,13 +36,13 @@ func FuzzEncodeDecodeBCD(f *testing.F) {
if err != nil {
t.Skip("Only correct requests are interesting: %w", err)
}
var dec string
dec, err = DecodeStringFromBCD(bcdBuf)

dec, exp, err := DecodeStringFromBCD(bcdBuf)
if err != nil {
t.Fatalf("Failed to decode encoded value ('%s')", orig)
}

if !strToDecimal(t, dec).Equal(strToDecimal(t, orig)) {
if !strToDecimal(t, dec, exp).Equal(strToDecimal(t, orig, 0)) {
t.Fatal("Decimal numbers are not equal")
}
})
Expand Down

0 comments on commit 317d100

Please sign in to comment.