improved errorhandling related to deserializing `ErrorMessage` (#4604)

cli/processexecutor.cpp

@@ -162,7 +162,10 @@ int ProcessExecutor::handleRead(int rpipe, unsigned int &result)
     } else if (type == PipeWriter::REPORT_ERROR || type == PipeWriter::REPORT_INFO) {
         ErrorMessage msg;
         try {
-            msg.deserialize(buf);
+            if (!msg.deserialize(buf)) {
+                std::cerr << "#### ThreadExecutor::handleRead error" << std::endl;
+                std::exit(EXIT_FAILURE);
+            }
         } catch (const InternalError& e) {
             std::cerr << "#### ThreadExecutor::handleRead error, internal error:" << e.errorMessage << std::endl;
             std::exit(EXIT_FAILURE);

lib/errorlogger.cpp

@@ -258,8 +258,10 @@ std::string ErrorMessage::serialize() const
 
 bool ErrorMessage::deserialize(const std::string &data)
 {
+    // TODO: clear all fields
     certainty = Certainty::normal;
     callStack.clear();
+
     std::istringstream iss(data);
     std::array<std::string, 7> results;
     std::size_t elem = 0;
@@ -268,13 +270,21 @@ bool ErrorMessage::deserialize(const std::string &data)
         if (!(iss >> len))
             return false;
 
-        iss.get();
+        if (iss.get() != ' ')
+            return false;
+
+        if (!iss.good())
+            return false;
+
         std::string temp;
         for (unsigned int i = 0; i < len && iss.good(); ++i) {
             const char c = static_cast<char>(iss.get());
             temp.append(1, c);
         }
 
+        if (!iss.good())
+            return false;
+
         if (temp == "inconclusive") {
             certainty = Certainty::inconclusive;
             continue;
@@ -283,14 +293,19 @@ bool ErrorMessage::deserialize(const std::string &data)
         results[elem++] = temp;
     }
 
+    if (!iss.good())
+        return false;
+
     if (elem != 7)
-        throw InternalError(nullptr, "Internal Error: Deserialization of error message failed");
+        throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - insufficient elements");
 
     id = results[0];
     severity = Severity::fromString(results[1]);
-    std::istringstream(results[2]) >> cwe.id;
+    if (!(std::istringstream(results[2]) >> cwe.id))
-    std::istringstream(results[3]) >> hash;
+        throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - invalid CWE ID");
-    std::istringstream(results[4]) >> file0;
+    if (!(std::istringstream(results[3]) >> hash))
+        throw InternalError(nullptr, "Internal Error: Deserialization of error message failed - invalid hash");
+    file0 = results[4];
     mShortMessage = results[5];
     mVerboseMessage = results[6];
 
@@ -298,6 +313,12 @@ bool ErrorMessage::deserialize(const std::string &data)
     if (!(iss >> stackSize))
         return false;
 
+    if (iss.get() != ' ')
+        return false;
+
+    if (stackSize == 0)
+        return true;
+
     while (iss.good()) {
         unsigned int len = 0;
         if (!(iss >> len))

test/testerrorlogger.cpp

@@ -297,7 +297,7 @@ private:
                       "0 ", msg.serialize());
 
         ErrorMessage msg2;
-        msg2.deserialize(msg.serialize());
+        ASSERT(msg2.deserialize(msg.serialize()));
         ASSERT_EQUALS("errorId", msg2.id);
         ASSERT_EQUALS(Severity::error, msg2.severity);
         ASSERT_EQUALS("test.cpp", msg2.file0);
@@ -307,8 +307,53 @@ private:
     }
 
     void DeserializeInvalidInput() const {
+        {
+            // missing/invalid length
+            // missing separator
             ErrorMessage msg;
-        ASSERT_THROW(msg.deserialize("500foobar"), InternalError);
+            ASSERT(!msg.deserialize("500foobar"));
+        }
+        {
+            // invalid length
+            ErrorMessage msg;
+            ASSERT(!msg.deserialize("foo foobar"));
+        }
+        {
+            // mismatching length
+            ErrorMessage msg;
+            ASSERT(!msg.deserialize("8 errorId"));
+        }
+        {
+            // incomplete message
+            ErrorMessage msg;
+            ASSERT(!msg.deserialize("7 errorId"));
+        }
+        {
+            // invalid CWE ID
+            const char str[] = "7 errorId"
+                               "5 error"
+                               "7 invalid"
+                               "1 0"
+                               "8 test.cpp"
+                               "17 Programming error"
+                               "17 Programming error"
+                               "0 ";
+            ErrorMessage msg;
+            ASSERT_THROW(msg.deserialize(str), InternalError);
+        }
+        {
+            // invalid hash
+            const char str[] = "7 errorId"
+                               "5 error"
+                               "1 0"
+                               "7 invalid"
+                               "8 test.cpp"
+                               "17 Programming error"
+                               "17 Programming error"
+                               "0 ";
+            ErrorMessage msg;
+            ASSERT_THROW(msg.deserialize(str), InternalError);
+        }
     }
 
     void SerializeSanitize() const {
@@ -326,7 +371,7 @@ private:
                       "0 ", msg.serialize());
 
         ErrorMessage msg2;
-        msg2.deserialize(msg.serialize());
+        ASSERT(msg2.deserialize(msg.serialize()));
         ASSERT_EQUALS("errorId", msg2.id);
         ASSERT_EQUALS(Severity::error, msg2.severity);
         ASSERT_EQUALS("1.c", msg2.file0);
@@ -344,7 +389,7 @@ private:
         ErrorMessage msg(locs, emptyString, Severity::error, "Programming error", "errorId", Certainty::inconclusive);
 
         ErrorMessage msg2;
-        msg2.deserialize(msg.serialize());
+        ASSERT(msg2.deserialize(msg.serialize()));
         ASSERT_EQUALS("[]:;,()", msg2.callStack.front().getfile(false));
         ASSERT_EQUALS(":/,;", msg2.callStack.front().getOrigFile(false));
         ASSERT_EQUALS(654, msg2.callStack.front().line);