Convert JSON parsing failures from assertions to exceptions (#7531)

This allows some useful error reporting with bad source maps.
The JSON exception is converted to the existing map parse exception by
the source map parser, allowing a unified interface. Also add several tests
of bogus source maps.

Author: dschuff

src/source-map.h

@@ -19,14 +19,15 @@
 
 #include <optional>
 
+#include "support/json.h"
 #include "wasm.h"
 
 namespace wasm {
 
 struct MapParseException {
-  std::string text;
-
-  MapParseException(std::string text) : text(text) {}
+  std::string errorText;
+  MapParseException(std::string errorText) : errorText(errorText){};
+  MapParseException(json::JsonParseException ex) : errorText(ex.errorText){};
   void dump(std::ostream& o) const;
 };
 

src/support/json.h

@@ -45,6 +45,18 @@ namespace json {
 
 using IString = wasm::IString;
 
+struct JsonParseException {
+  std::string errorText;
+
+  JsonParseException(std::string errorText) : errorText(errorText) {}
+  void dump(std::ostream& o) const { o << "JSON parse error: " << errorText; }
+};
+
+#define THROW_IF(expr, message)                                                \
+  if (expr) {                                                                  \
+    throw JsonParseException(message);                                         \
+  }
+
 // Main value type
 struct Value {
   struct Ref : public std::shared_ptr<Value> {
@@ -277,7 +289,7 @@ struct Value {
       do {
         close = strchr(close + 1, '"');
       } while (*(close - 1) == '\\');
-      assert(close);
+      THROW_IF(!close, "malformed JSON string");
       *close = 0; // end this string, and reuse it straight from the input
       char* raw = curr + 1;
       if (stringEncoding == ASCII) {
@@ -301,24 +313,24 @@ struct Value {
         if (*curr == ']') {
           break;
         }
-        assert(*curr == ',');
+        THROW_IF(*curr != ',', "malformed JSON array");
         curr++;
         skip();
       }
       curr++;
     } else if (*curr == 'n') {
       // Null
-      assert(strncmp(curr, "null", 4) == 0);
+      THROW_IF(strncmp(curr, "null", 4) != 0, "unexpected JSON literal");
       setNull();
       curr += 4;
     } else if (*curr == 't') {
       // Bool true
-      assert(strncmp(curr, "true", 4) == 0);
+      THROW_IF(strncmp(curr, "true", 4) != 0, "unexpected JSON literal");
       setBool(true);
       curr += 4;
     } else if (*curr == 'f') {
       // Bool false
-      assert(strncmp(curr, "false", 5) == 0);
+      THROW_IF(strncmp(curr, "false", 5) != 0, "unexpected JSON literal");
       setBool(false);
       curr += 5;
     } else if (*curr == '{') {
@@ -327,15 +339,15 @@ struct Value {
       skip();
       setObject();
       while (*curr != '}') {
-        assert(*curr == '"');
+        THROW_IF(*curr != '"', "malformed key in JSON object");
         curr++;
         char* close = strchr(curr, '"');
-        assert(close);
+        THROW_IF(!close, "malformed key in JSON object");
         *close = 0; // end this string, and reuse it straight from the input
         IString key(curr);
         curr = close + 1;
         skip();
-        assert(*curr == ':');
+        THROW_IF(*curr != ':', "missing ':', in JSON object");
         curr++;
         skip();
         Ref value = Ref(new Value());
@@ -345,7 +357,7 @@ struct Value {
         if (*curr == '}') {
           break;
         }
-        assert(*curr == ',');
+        THROW_IF(*curr != ',', "malformed value in JSON object");
         curr++;
         skip();
       }

src/wasm/source-map.cpp

@@ -28,7 +28,7 @@ void MapParseException::dump(std::ostream& o) const {
   Colors::red(o);
   o << "map parse exception: ";
   Colors::green(o);
-  o << text;
+  o << errorText;
   Colors::magenta(o);
   o << "]";
   Colors::normal(o);
@@ -39,7 +39,11 @@ void SourceMapReader::parse(Module& wasm) {
     return;
   }
   json::Value json;
-  json.parse(buffer.data(), json::Value::ASCII);
+  try {
+    json.parse(buffer.data(), json::Value::ASCII);
+  } catch (json::JsonParseException jx) {
+    throw MapParseException(jx);
+  }
   if (!json.isObject()) {
     throw MapParseException("Source map is not valid JSON");
   }
@@ -135,6 +139,12 @@ SourceMapReader::readDebugLocationAt(size_t currLocation) {
       col += readBase64VLQ();
 
       next = peek();
+      if (next == ';') {
+        // Generated JS files can have multiple lines, and mappings for each
+        // line are separated by ';'. Wasm files do not have lines, so there
+        // should be only one generated "line".
+        throw MapParseException("Unexpected mapping for 2nd generated line");
+      }
       if (next == ',' || next == '\"') {
         hasSymbol = false;
         break;

test/gtest/CMakeLists.txt

@@ -2,6 +2,7 @@ if(BUILD_FUZZTEST)
   include_directories(SYSTEM ${PROJECT_SOURCE_DIR}/third_party/fuzztest)
 else()
   include_directories(SYSTEM ${PROJECT_SOURCE_DIR}/third_party/googletest/googletest/include)
+  include_directories(SYSTEM ${PROJECT_SOURCE_DIR}/third_party/googletest/googlemock/include)
 endif()
 
 set(unittest_SOURCES
@@ -18,7 +19,6 @@ set(unittest_SOURCES
   possible-contents.cpp
   printing.cpp
   scc.cpp
-  source-map.cpp
   stringify.cpp
   suffix_tree.cpp
   topological-sort.cpp
@@ -29,6 +29,14 @@ set(unittest_SOURCES
 
 if(BUILD_FUZZTEST)
   set(unittest_SOURCES ${unittest_SOURCES} type-domains.cpp)
+else()
+  # source-map.cpp uses the gmock-matchers.h header, which is included with the
+  # "standard" upstream gtest library, but not with the one bundled into the
+  # fuzztest library (and the gmock included with upstream gtest seems
+  # incompatible with the one in fuzztest). For now we work around this by just
+  # excluding source-map.cpp from the fuzztest build, but if we start using
+  # gmock more we should figure out what the right way to hande this is.
+  set(unittest_SOURCES ${unittest_SOURCES} source-map.cpp)
 endif()
 
 # suffix_tree.cpp includes LLVM header using std::iterator (deprecated in C++17)

test/gtest/source-map.cpp

@@ -16,6 +16,7 @@
 
 #include "source-map.h"
 #include "print-test.h"
+#include "gmock/gmock-matchers.h"
 #include "gtest/gtest.h"
 
 using namespace wasm;
@@ -53,6 +54,16 @@ class SourceMapTest : public PrintTest {
     EXPECT_EQ(loc->columnNumber, col);
     EXPECT_EQ(loc->symbolNameIndex, sym);
   }
+
+  void ExpectParseError(std::string& mapString, const char* expectedError) {
+    SCOPED_TRACE(mapString);
+    EXPECT_THROW(parseMap(mapString), MapParseException);
+    try {
+      parseMap(mapString);
+    } catch (MapParseException ex) {
+      EXPECT_THAT(ex.errorText, ::testing::HasSubstr(expectedError));
+    }
+  }
 };
 
 // Check that debug location parsers can handle single-segment mappings.
@@ -73,16 +84,49 @@ TEST_F(SourceMapTest, SourceMappingSingleSegment) {
   EXPECT_FALSE(loc.has_value());
 }
 
-TEST_F(SourceMapTest, BadSourceMap) {
-  // This source map is missing the version field.
+TEST_F(SourceMapTest, BadSourceMaps) {
+  // Test that a malformed JSON string throws rather than asserting.
   std::string sourceMap = R"(
+    {
+      "version": 3,
+      "sources": ["foo.c"],
+      "mappings": ""
+    malformed
+    }
+  )";
+  ExpectParseError(sourceMap, "malformed value in JSON object");
+
+  // Valid JSON, but missing the version field.
+  sourceMap = R"(
     {
       "sources": [],
       "names": [],
       "mappings": "A"
     }
   )";
-  EXPECT_THROW(parseMap(sourceMap), MapParseException);
+  ExpectParseError(sourceMap, "Source map version missing");
+
+  // Valid JSON, but a bad "sources" field.
+  sourceMap = R"(
+    {
+      "version": 3,
+      "sources": 123,
+      "mappings": ""
+    }
+  )";
+  ExpectParseError(sourceMap, "Source map sources missing or not an array");
+
+  sourceMap = R"(
+    {
+      "version": 3,
+      "sources": ["foo.c"],
+      "mappings": "C;A"
+    }
+  )";
+  parseMap(sourceMap);
+  // Mapping strings are parsed incrementally, so errors don't show up until a
+  // sufficiently far-advanced location is requested to reach the problem.
+  EXPECT_THROW(reader->readDebugLocationAt(1), MapParseException);
 }
 
 TEST_F(SourceMapTest, SourcesAndNames) {