Detect integer overflow when processing sizes, indices, integer values.

Author: LTLA

include/eminem/Parser.hpp

@@ -10,6 +10,7 @@
 #include <thread>
 #include <mutex>
 #include <condition_variable>
+#include <limits>
 
 #include "byteme/RawBufferReader.hpp"
 #include "byteme/PerByte.hpp"
@@ -569,13 +570,24 @@ class Parser {
             }
         };
 
+        constexpr Index max_limit = std::numeric_limits<Index>::max();
+        constexpr Index max_limit_before_mult = max_limit / 10; 
+        constexpr Index max_limit_mod = max_limit % 10; 
+
         while (1) {
             char x = input.get();
             switch(x) {
                 case '0': case '1': case '2': case '3': case '4': case '5': case '6': case '7': case '8': case '9':
+                    { 
+                        Index delta = x - '0';
+                        // Structuring the conditionals so that it's most likely to short-circuit after only testing the first one.
+                        if (output.index >= max_limit_before_mult && !(output.index == max_limit_before_mult && delta <= max_limit_mod)) {
+                            throw std::runtime_error("integer overflow in " + what() + " field on line " + std::to_string(overall_line_count + 1));
+                        }
+                        output.index *= 10;
+                        output.index += delta;
+                    }
                     found = true;
-                    output.index *= 10;
-                    output.index += x - '0';
                     break;
                 case '\n':
                     // This check only needs to be put here, as all blanks should be chomped before calling
@@ -745,6 +757,8 @@ class Parser {
 private:
     template<typename Type_>
     struct ParseInfo {
+        ParseInfo() = default;
+        ParseInfo(Type_ value, bool remaining) : value(value), remaining(remaining) {}
         Type_ value;
         bool remaining;
     };
@@ -1358,41 +1372,55 @@ class Parser {
     public:
         template<class Input2_>
         ParseInfo<Type_> operator()(Input2_& input, Index overall_line_count) {
+            Type_ val = 0;
+            bool found = false;
+
             bool negative = (input.get() == '-');
             if (negative) {
                 if (!(input.advance())) {
                     throw std::runtime_error("premature termination of an integer on line " + std::to_string(overall_line_count + 1));
                 }
             }
 
-            Type_ val = 0;
-            auto finish = [&](bool valid) -> ParseInfo<Type_> {
-                ParseInfo<Type_> output;
-                output.remaining = valid;
-                if (negative) {
-                    val *= -1;
-                }
-                output.value = val;
-                return output;
-            };
+            constexpr Type_ upper_limit = std::numeric_limits<Type_>::max();
+            constexpr Type_ upper_limit_before_mult = upper_limit / 10;
+            constexpr Type_ upper_limit_mod = upper_limit % 10;
+            constexpr Type_ lower_limit = std::numeric_limits<Type_>::lowest();
+            constexpr Type_ lower_limit_before_mult = lower_limit / 10;
+            constexpr Type_ lower_limit_mod = -(lower_limit % 10);
 
-            bool found = false;
             while (1) {
                 char x = input.get();
                 switch (x) {
                     case '0': case '1': case '2': case '3': case '4': case '5': case '6': case '7': case '8': case '9':
-                        val *= 10;
-                        val += x - '0';
+                        {
+                            Type_ delta = x - '0';
+                            // We have to handle negative and positive cases separately as they overflow at different thresholds.
+                            if (negative) {
+                                // Structuring the conditionals so that it's most likely to short-circuit after only testing the first one.
+                                if (val <= lower_limit_before_mult && !(val == lower_limit_before_mult && delta <= lower_limit_mod)) {
+                                    throw std::runtime_error("integer underflow on line " + std::to_string(overall_line_count + 1));
+                                }
+                                val *= 10;
+                                val -= delta;
+                            } else {
+                                if (val >= upper_limit_before_mult && !(val == upper_limit_before_mult && delta <= upper_limit_mod)) {
+                                    throw std::runtime_error("integer overflow on line " + std::to_string(overall_line_count + 1));
+                                }
+                                val *= 10;
+                                val += delta;
+                            }
+                        }
                         found = true;
                         break;
                     case ' ': case '\t':
                         if (!advance_and_chomp(input)) { // skipping past the current position before chomping.
-                            return finish(false);
+                            return ParseInfo<Type_>(val, false);
                         }
                         if (input.get() != '\n') {
                             throw std::runtime_error("more fields than expected on line " + std::to_string(overall_line_count + 1));
                         }
-                        return finish(input.advance()); // move past the newline.
+                        return ParseInfo<Type_>(val, input.advance()); // move past the newline.
                     case '\n':
                         // This check only needs to be put here, as all blanks should be chomped before calling
                         // this function; so we must start on a non-blank character. This starting character is either:
@@ -1402,7 +1430,7 @@ class Parser {
                         if (!found) {
                             throw std::runtime_error("empty integer field on line " + std::to_string(overall_line_count + 1));
                         }
-                        return finish(input.advance()); // move past the newline.
+                        return ParseInfo<Type_>(val, input.advance()); // move past the newline.
                     default:
                         throw std::runtime_error("expected an integer value on line " + std::to_string(overall_line_count + 1));
                 }
@@ -1412,7 +1440,7 @@ class Parser {
                 }
             }
 
-            return finish(false);
+            return ParseInfo<Type_>(val, false);
         }
     };
 

tests/src/integer.cpp

@@ -98,14 +98,84 @@ TEST(ParserInteger, OtherType) {
     eminem::Parser parser(std::make_unique<byteme::PerByteSerial<char> >(std::move(reader)), {});
     parser.scan_preamble();
 
-    std::vector<uint16_t> observed;
-    EXPECT_TRUE(parser.template scan_integer<uint16_t>([&](eminem::Index, eminem::Index, uint16_t val) {
+    std::vector<std::uint16_t> observed;
+    EXPECT_TRUE(parser.template scan_integer<std::uint16_t>([&](eminem::Index, eminem::Index, std::uint16_t val) {
         observed.push_back(val);
     }));
-    std::vector<uint16_t> expected { 33, 666, 9 };
+    std::vector<std::uint16_t> expected { 33, 666, 9 };
     EXPECT_EQ(observed, expected);
 }
 
+TEST(ParserInteger, Overflow) {
+    // Unsigned.
+    {
+        // First we check that it works as a positive control.
+        std::string input = "%%MatrixMarket matrix coordinate integer general\n2 2 3\n1 1 255\n2 2 0\n2 1 -0\n";
+        auto reader = std::make_unique<byteme::RawBufferReader>(reinterpret_cast<const unsigned char*>(input.data()), input.size()); 
+        eminem::Parser parser(std::make_unique<byteme::PerByteSerial<char> >(std::move(reader)), {});
+        parser.scan_preamble();
+
+        std::vector<std::uint8_t> observed;
+        parser.scan_integer<std::uint8_t>([&](eminem::Index, eminem::Index, std::uint8_t val){
+            observed.push_back(val);
+        });
+        std::vector<std::uint8_t> expected { 255, 0, 0 };
+        EXPECT_EQ(observed, expected);
+
+        // Now testing for the failures.
+        auto test_overflow = [&](std::string input) -> void {
+            auto reader = std::make_unique<byteme::RawBufferReader>(reinterpret_cast<const unsigned char*>(input.data()), input.size()); 
+            eminem::Parser parser(std::make_unique<byteme::PerByteSerial<char> >(std::move(reader)), {});
+            parser.scan_preamble();
+            EXPECT_ANY_THROW({
+                try {
+                    parser.scan_integer<std::uint8_t>([&](eminem::Index, eminem::Index, std::uint8_t){});
+                } catch (std::exception& e) {
+                    EXPECT_THAT(e.what(), ::testing::HasSubstr("flow"));
+                    throw;
+                }
+            });
+        };
+        test_overflow("%%MatrixMarket matrix coordinate integer general\n2 2 1\n1 1 256");
+        test_overflow("%%MatrixMarket matrix coordinate integer general\n2 2 1\n1 1 1000");
+        test_overflow("%%MatrixMarket matrix coordinate integer general\n2 2 1\n1 1 -1");
+    }
+
+    // Signed.
+    {
+        // First we check that it works as a positive control.
+        std::string input = "%%MatrixMarket matrix coordinate integer general\n2 2 3\n1 1 0\n2 2 127\n2 1 -128\n";
+        auto reader = std::make_unique<byteme::RawBufferReader>(reinterpret_cast<const unsigned char*>(input.data()), input.size()); 
+        eminem::Parser parser(std::make_unique<byteme::PerByteSerial<char> >(std::move(reader)), {});
+        parser.scan_preamble();
+        std::vector<std::int8_t> observed;
+        parser.scan_integer<std::int8_t>([&](eminem::Index, eminem::Index, std::int8_t val){
+            observed.push_back(val);
+        });
+        std::vector<std::int8_t> expected { 0, 127, -128 };
+        EXPECT_EQ(observed, expected);
+
+        // Now testing for the failures.
+        auto test_overflow = [&](std::string input) -> void {
+            auto reader = std::make_unique<byteme::RawBufferReader>(reinterpret_cast<const unsigned char*>(input.data()), input.size()); 
+            eminem::Parser parser(std::make_unique<byteme::PerByteSerial<char> >(std::move(reader)), {});
+            parser.scan_preamble();
+            EXPECT_ANY_THROW({
+                try {
+                    parser.scan_integer<std::int8_t>([&](eminem::Index, eminem::Index, std::int8_t){});
+                } catch (std::exception& e) {
+                    EXPECT_THAT(e.what(), ::testing::HasSubstr("flow"));
+                    throw;
+                }
+            });
+        };
+        test_overflow("%%MatrixMarket matrix coordinate integer general\n2 2 1\n1 1 128");
+        test_overflow("%%MatrixMarket matrix coordinate integer general\n2 2 1\n1 1 1000");
+        test_overflow("%%MatrixMarket matrix coordinate integer general\n2 2 1\n1 1 -129");
+        test_overflow("%%MatrixMarket matrix coordinate integer general\n2 2 1\n1 1 -1000");
+    }
+}
+
 TEST(ParserInteger, QuitEarly) {
     std::string input = "%%MatrixMarket matrix coordinate integer general\n10 10 3\n1 2 33\n4 5 666\n7 8 9\n";
     auto reader = std::make_unique<byteme::RawBufferReader>(reinterpret_cast<const unsigned char*>(input.data()), input.size()); 

tests/src/preamble.cpp

@@ -236,6 +236,35 @@ TEST(ParserPreamble, SizeErrors) {
     test_error("%%MatrixMarket matrix coordinate integer general\n2 -5 1\n", "unexpected character");
 }
 
+TEST(ParserPreamble, SizeOverflowErrors) {
+    auto limit = std::numeric_limits<eminem::Index>::max();
+
+    // No overflow at this point, as a negative control.
+    auto limitstring = std::to_string(limit);
+    {
+        std::string payload = std::string("%%MatrixMarket matrix coordinate integer general\n2 ") + limitstring + std::string(" 0\n");
+        auto reader = std::make_unique<byteme::RawBufferReader>(reinterpret_cast<const unsigned char*>(payload.data()), payload.size()); 
+        eminem::Parser parser(std::make_unique<byteme::PerByteSerial<char> >(std::move(reader)), {});
+        parser.scan_preamble();
+        EXPECT_EQ(parser.get_ncols(), limit);
+    }
+
+    // Obvious overflow if we increase it by 10-fold with an extra digit on the end.
+    test_error("%%MatrixMarket matrix coordinate integer general\n1 " + limitstring + "0 0\n", "overflow");
+
+    // Manually adding one to the limit.
+    auto extrastring = limitstring;
+    for (auto it = extrastring.rbegin(); it != extrastring.rend(); ++it) {
+        int digit = *it - '0';
+        ++digit;
+        if (digit < 10) {
+            *it = digit + '0';
+            break;
+        }
+    }
+    test_error("%%MatrixMarket matrix coordinate integer general\n1 " + extrastring + " 0\n", "overflow");
+}
+
 /**********************************************
  **********************************************/