fix(NODE-5546): decimal 128 fromString performs inexact rounding (#613)

Author: W-A-James

src/decimal128.ts

@@ -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;
 
@@ -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
@@ -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++] === '-';
     }
 
@@ -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;
@@ -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;
@@ -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;
         }
       }
@@ -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;
@@ -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;
         }
@@ -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');
       }
     }
 
@@ -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);
 
@@ -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++) {

test/node/bson_corpus.spec.test.js

@@ -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`

test/node/decimal128_tests.js

@@ -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/
+          );
+        });
+      }
+    );
+  });
 });

test/node/specs/bson-corpus/decimal128-1.json

@@ -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\"}}"
         }
     ]
 }