Merge pull request #126 from josefblaha/issues/#125-overflow-2

Improved arithmetic overflow detection to fix #125

Author: FlorianRappl

src/Mages.Core.Tests/NumberHelperTests.cs

@@ -0,0 +1,68 @@
+using Mages.Core.Tokens;
+using NUnit.Framework;
+
+namespace Mages.Core.Tests
+{
+    [TestFixture]
+    public class NumberHelperTests
+    {
+        [TestCase(0u, 0u)]
+        [TestCase(1u, 0u)]
+        [TestCase(0u, 1u)]
+        [TestCase(1u, 1u)]
+        [TestCase(123u, 123u)]
+        [TestCase(20u, 123u)]
+        [TestCase(10u, ulong.MaxValue / 10)]
+        [TestCase(0u, ulong.MaxValue)]
+        public void TryMultiply_happy_case(ulong x, ulong y)
+        {
+            var result = NumberHelper.TryMultiply(x, y, out var actual);
+            Assert.AreEqual(checked(x * y), actual);
+            Assert.IsTrue(result);
+        }
+
+        [TestCase(ulong.MaxValue, 2u)]
+        [TestCase(ulong.MaxValue, ulong.MaxValue)]
+        [TestCase(ulong.MaxValue / 2 + 1, 3u)]
+        [TestCase(ulong.MaxValue / 3 + 1, 4u)]
+        [TestCase(ulong.MaxValue - 1, 2u)]
+        [TestCase(ulong.MaxValue / 10 * 9 + 1, 2u)]
+        [TestCase(0xFFFFFFFFFFFFFFFFu, 2u)]
+        [TestCase(8409014139716477191u, 10u)]
+        public void TryMultiply_overflow_case(ulong x, ulong y)
+        {
+            var result = NumberHelper.TryMultiply(x, y, out var actual);
+            Assert.AreEqual(0, actual);
+            Assert.IsFalse(result);
+        }
+
+        [TestCase(0u, 0u)]
+        [TestCase(1u, 0u)]
+        [TestCase(0u, 1u)]
+        [TestCase(1u, 1u)]
+        [TestCase(123u, 123u)]
+        [TestCase(20u, 123u)]
+        [TestCase(0u, ulong.MaxValue)]
+        [TestCase(ulong.MaxValue, 0u)]
+        [TestCase(ulong.MaxValue - 10, 10u)]
+        public void TryAdd_happy_case(ulong x, ulong y)
+        {
+            var result = NumberHelper.TryAdd(x, y, out var actual);
+            Assert.AreEqual(checked(x + y), actual);
+            Assert.IsTrue(result);
+        }
+
+        [TestCase(1u, ulong.MaxValue)]
+        [TestCase(ulong.MaxValue, 1u)]
+        [TestCase(ulong.MaxValue, ulong.MaxValue)]
+        [TestCase(ulong.MaxValue / 2 + 1, ulong.MaxValue / 2 + 2)]
+        [TestCase(ulong.MaxValue - 1, 2u)]
+        [TestCase(0xFFFFFFFFFFFFFFFFu, 1u)]
+        public void TryAdd_overflow_case(ulong x, ulong y)
+        {
+            var result = NumberHelper.TryAdd(x, y, out var actual);
+            Assert.AreEqual(0, actual);
+            Assert.IsFalse(result);
+        }
+    }
+}

src/Mages.Core.Tests/NumberTests.cs

@@ -67,6 +67,19 @@ public void NumberScannerLargeValue()
             Assert.AreEqual(9223372036854776000.0, ((NumberToken)result).Value);
         }
 
+        [TestCase("0.8409014139716477191")]
+        [TestCase("0.84090141397164771912")]
+        public void NumberScannerLargePrecisionValue(string source)
+        {
+            var scanner = new StringScanner(source);
+            Assert.IsTrue(scanner.MoveNext());
+            var tokenizer = new NumberTokenizer();
+            var result = tokenizer.Next(scanner);
+            Assert.IsInstanceOf<NumberToken>(result);
+            // double has a precision of ~15-17 digits
+            Assert.AreEqual(0.840901413971647, ((NumberToken)result).Value, 1e-15);
+        }
+
         [Test]
         public void NumberScannerScientificPlus()
         {

src/Mages.Core/Tokens/NumberHelper.cs

@@ -0,0 +1,49 @@
+namespace Mages.Core.Tokens;
+
+/// <summary>
+/// Helper methods for number arithmetics. 
+/// </summary>
+internal static class NumberHelper
+{
+    /// <summary>
+    /// Multiplies two unsigned numbers if their product does not exceed
+    /// <see cref="ulong.MaxValue"/>.
+    /// </summary>
+    /// <returns>A value indicating whether the numbers were multiplied.</returns>
+    public static bool TryMultiply(ulong x, ulong y, out ulong product)
+    {
+        if (x == 0 || y == 0)
+        {
+            product = 0;
+            return true;
+        }
+
+        // Check for overflow
+        if (x > ulong.MaxValue / y)
+        {
+            product = 0;
+            return false;
+        }
+
+        product = x * y;
+        return true;
+    }
+
+    /// <summary>
+    /// Adds two unsigned numbers if their sum does not exceed
+    /// <see cref="ulong.MaxValue"/>.
+    /// </summary>
+    /// <returns>A value indicating whether the numbers were added.</returns>
+    public static bool TryAdd(ulong x, ulong y, out ulong sum)
+    {
+        // Check for overflow
+        if (x > ulong.MaxValue - y)
+        {
+            sum = 0;
+            return false;
+        }
+
+        sum = x + y;
+        return true;
+    }
+}

src/Mages.Core/Tokens/NumberTokenizer.cs

@@ -258,18 +258,15 @@ private void AddValue(UInt64 scale, UInt64 diff)
             {
                 _shifts++;
             }
+            else if (NumberHelper.TryMultiply(_value, scale, out var product) && NumberHelper.TryAdd(product, diff, out var newValue))
+            {
+                // assign new value
+                _value = newValue;
+            }
             else
             {
-                var newValue = _value * scale + diff;
-
-                if (newValue < _value)
-                {
-                    _shifts++;
-                }
-                else
-                {
-                    _value = newValue;
-                }
+                // overflow!
+                _shifts++;
             }
         }
     }