Skip to content

Commit

Permalink
fix(NODE-5546): decimal 128 fromString performs inexact rounding (#613)
Browse files Browse the repository at this point in the history
  • Loading branch information
@W-A-James
W-A-James committed Sep 8, 2023
1 parent 4c1db9a commit 316c30e
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 72 deletions.
102 changes: 35 additions & 67 deletions src/decimal128.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ export class Decimal128 extends BSONValue {
static fromString(representation: string): Decimal128 {
// Parse state tracking
let isNegative = false;
let sawSign = false;
let sawRadix = false;
let foundNonZero = false;

Expand All @@ -180,15 +181,11 @@ export class Decimal128 extends BSONValue {
let nDigitsStored = 0;
// Insertion pointer for digits
let digitsInsert = 0;
// The index of the first non-zero digit
let firstDigit = 0;
// The index of the last digit
let lastDigit = 0;

// Exponent
let exponent = 0;
// loop index over array
let i = 0;
// The high 17 digits of the significand
let significandHigh = new Long(0, 0);
// The low 17 digits of the significand
Expand Down Expand Up @@ -241,6 +238,7 @@ export class Decimal128 extends BSONValue {

// Get the negative or positive sign
if (representation[index] === '+' || representation[index] === '-') {
sawSign = true;
isNegative = representation[index++] === '-';
}

Expand All @@ -263,7 +261,7 @@ export class Decimal128 extends BSONValue {
continue;
}

if (nDigitsStored < 34) {
if (nDigitsStored < MAX_DIGITS) {
if (representation[index] !== '0' || foundNonZero) {
if (!foundNonZero) {
firstNonZero = nDigitsRead;
Expand Down Expand Up @@ -307,11 +305,7 @@ export class Decimal128 extends BSONValue {

// Done reading input
// Find first non-zero digit in digits
firstDigit = 0;

if (!nDigitsStored) {
firstDigit = 0;
lastDigit = 0;
digits[0] = 0;
nDigits = 1;
nDigitsStored = 1;
Expand All @@ -320,7 +314,11 @@ export class Decimal128 extends BSONValue {
lastDigit = nDigitsStored - 1;
significantDigits = nDigits;
if (significantDigits !== 1) {
while (digits[firstNonZero + significantDigits - 1] === 0) {
while (
representation[
firstNonZero + significantDigits - 1 + Number(sawSign) + Number(sawRadix)
] === '0'
) {
significantDigits = significantDigits - 1;
}
}
Expand All @@ -331,7 +329,7 @@ export class Decimal128 extends BSONValue {
// to represent user input

// Overflow prevention
if (exponent <= radixPosition && radixPosition - exponent > 1 << 14) {
if (exponent <= radixPosition && radixPosition > exponent + (1 << 14)) {
exponent = EXPONENT_MIN;
} else {
exponent = exponent - radixPosition;
Expand All @@ -341,11 +339,9 @@ export class Decimal128 extends BSONValue {
while (exponent > EXPONENT_MAX) {
// Shift exponent to significand and decrease
lastDigit = lastDigit + 1;

if (lastDigit - firstDigit > MAX_DIGITS) {
if (lastDigit >= MAX_DIGITS) {
// Check if we have a zero then just hard clamp, otherwise fail
const digitsString = digits.join('');
if (digitsString.match(/^0+$/)) {
if (significantDigits === 0) {
exponent = EXPONENT_MAX;
break;
}
Expand All @@ -357,85 +353,57 @@ export class Decimal128 extends BSONValue {

while (exponent < EXPONENT_MIN || nDigitsStored < nDigits) {
// Shift last digit. can only do this if < significant digits than # stored.
if (lastDigit === 0 && significantDigits < nDigitsStored) {
exponent = EXPONENT_MIN;
significantDigits = 0;
break;
if (lastDigit === 0) {
if (significantDigits === 0) {
exponent = EXPONENT_MIN;
break;
}

invalidErr(representation, 'exponent underflow');
}

if (nDigitsStored < nDigits) {
if (
representation[nDigits - 1 + Number(sawSign) + Number(sawRadix)] !== '0' &&
significantDigits !== 0
) {
invalidErr(representation, 'inexact rounding');
}
// adjust to match digits not stored
nDigits = nDigits - 1;
} else {
if (digits[lastDigit] !== 0) {
invalidErr(representation, 'inexact rounding');
}
// adjust to round
lastDigit = lastDigit - 1;
}

if (exponent < EXPONENT_MAX) {
exponent = exponent + 1;
} else {
// Check if we have a zero then just hard clamp, otherwise fail
const digitsString = digits.join('');
if (digitsString.match(/^0+$/)) {
exponent = EXPONENT_MAX;
break;
}
invalidErr(representation, 'overflow');
}
}

// Round
// We've normalized the exponent, but might still need to round.
if (lastDigit - firstDigit + 1 < significantDigits) {
let endOfString = nDigitsRead;

if (lastDigit + 1 < significantDigits) {
// If we have seen a radix point, 'string' is 1 longer than we have
// documented with ndigits_read, so inc the position of the first nonzero
// digit and the position that digits are read to.
if (sawRadix) {
firstNonZero = firstNonZero + 1;
endOfString = endOfString + 1;
}
// if negative, we need to increment again to account for - sign at start.
if (isNegative) {
// if saw sign, we need to increment again to account for - or + sign at start.
if (sawSign) {
firstNonZero = firstNonZero + 1;
endOfString = endOfString + 1;
}

const roundDigit = parseInt(representation[firstNonZero + lastDigit + 1], 10);
let roundBit = 0;

if (roundDigit >= 5) {
roundBit = 1;
if (roundDigit === 5) {
roundBit = digits[lastDigit] % 2 === 1 ? 1 : 0;
for (i = firstNonZero + lastDigit + 2; i < endOfString; i++) {
if (parseInt(representation[i], 10)) {
roundBit = 1;
break;
}
}
}
}

if (roundBit) {
let dIdx = lastDigit;

for (; dIdx >= 0; dIdx--) {
if (++digits[dIdx] > 9) {
digits[dIdx] = 0;

// overflowed most significant digit
if (dIdx === 0) {
if (exponent < EXPONENT_MAX) {
exponent = exponent + 1;
digits[dIdx] = 1;
} else {
return new Decimal128(isNegative ? INF_NEGATIVE_BUFFER : INF_POSITIVE_BUFFER);
}
}
}
}
if (roundDigit !== 0) {
invalidErr(representation, 'inexact rounding');
}
}

Expand All @@ -449,8 +417,8 @@ export class Decimal128 extends BSONValue {
if (significantDigits === 0) {
significandHigh = Long.fromNumber(0);
significandLow = Long.fromNumber(0);
} else if (lastDigit - firstDigit < 17) {
let dIdx = firstDigit;
} else if (lastDigit < 17) {
let dIdx = 0;
significandLow = Long.fromNumber(digits[dIdx++]);
significandHigh = new Long(0, 0);

Expand All @@ -459,7 +427,7 @@ export class Decimal128 extends BSONValue {
significandLow = significandLow.add(Long.fromNumber(digits[dIdx]));
}
} else {
let dIdx = firstDigit;
let dIdx = 0;
significandHigh = Long.fromNumber(digits[dIdx++]);

for (; dIdx <= lastDigit - 17; dIdx++) {
Expand Down
5 changes: 0 additions & 5 deletions test/node/bson_corpus.spec.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,8 @@ function normalize(cEJ) {

const parseErrorForDecimal128 = scenario => {
// TODO(NODE-3637): remove regex of skipped tests and and add errors to d128 parsing
const skipRegex = /dqbsr|Inexact/;
for (const parseError of scenario.parseErrors) {
it(parseError.description, function () {
if (skipRegex.test(parseError.description)) {
this.skip();
}

expect(
() => BSON.Decimal128.fromString(parseError.string),
`Decimal.fromString('${parseError.string}') should throw`
Expand Down
22 changes: 22 additions & 0 deletions test/node/decimal128_tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1211,4 +1211,26 @@ describe('Decimal128', function () {
expect(() => new Decimal128(Buffer.alloc(16))).to.not.throw();
expect(() => new Decimal128(new Uint8Array(16))).to.not.throw();
});

context('fromString()', function () {
context("when input has leading '+' and has more than 34 significant digits", function () {
it('throws BSON error on inexact rounding', function () {
expect(() =>
Decimal128.fromString('+100000000000000000000000000000000000000000000001')
).to.throw(BSON.BSONError, /inexact rounding/);
});
});

context(
'when input has 1 significant digits, 34 total digits and an exponent greater than exponent_max',
function () {
it('throws BSON error reporting overflow', function () {
expect(() => Decimal128.fromString('1000000000000000000000000000000000e6112')).to.throw(
BSON.BSONError,
/overflow/
);
});
}
);
});
});
24 changes: 24 additions & 0 deletions test/node/specs/bson-corpus/decimal128-1.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,30 @@
"canonical_bson": "18000000136400000000000a5bc138938d44c64d31cc3700",
"degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"1000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\"}}",
"canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"1.000000000000000000000000000000000E+999\"}}"
},
{
"description": "Clamped zeros with a large positive exponent",
"canonical_bson": "180000001364000000000000000000000000000000FE5F00",
"degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E+2147483647\"}}",
"canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E+6111\"}}"
},
{
"description": "Clamped zeros with a large negative exponent",
"canonical_bson": "180000001364000000000000000000000000000000000000",
"degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E-2147483647\"}}",
"canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E-6176\"}}"
},
{
"description": "Clamped negative zeros with a large positive exponent",
"canonical_bson": "180000001364000000000000000000000000000000FEDF00",
"degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E+2147483647\"}}",
"canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E+6111\"}}"
},
{
"description": "Clamped negative zeros with a large negative exponent",
"canonical_bson": "180000001364000000000000000000000000000000008000",
"degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E-2147483647\"}}",
"canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E-6176\"}}"
}
]
}

0 comments on commit 316c30e

Please sign in to comment.