Skip to content

Convert JSON parsing failures from assertions to exceptions #7531

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dschuff merged 7 commits into from
Apr 24, 2025
Merged

Convert JSON parsing failures from assertions to exceptions #7531

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
46eea97
Convert JSON parsing failures from assertions to exceptions
dschuff Apr 18, 2025
3c99c1b
add comment explaining generated lines
dschuff Apr 18, 2025
d88effd
add comment, format
dschuff Apr 18, 2025
e4c10dd
add gmock when using fuzztest
dschuff Apr 18, 2025
8947ac5
Exclude sourcemap test from fuzztests
dschuff Apr 21, 2025
a6376a6
Merge branch 'main' into parseexception
dschuff Apr 23, 2025
f8520e6
add comment explaining the exclusion
dschuff Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/source-map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};

Expand Down
30 changes: 21 additions & 9 deletions src/support/json.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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) {
Expand All @@ -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 == '{') {
Expand All @@ -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());
Expand All @@ -345,7 +357,7 @@ struct Value {
if (*curr == '}') {
break;
}
assert(*curr == ',');
THROW_IF(*curr != ',', "malformed value in JSON object");
curr++;
skip();
}
Expand Down
11 changes: 9 additions & 2 deletions src/wasm/source-map.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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");
}
Expand Down Expand Up @@ -135,6 +139,9 @@ SourceMapReader::readDebugLocationAt(size_t currLocation) {
col += readBase64VLQ();

next = peek();
if (next == ';') {
throw MapParseException("Unexpected mapping for 2nd generated line");
Copy link
Copy Markdown
Member

@kripken kripken Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "2nd" mean here?

Copy link
Copy Markdown
Member Author

@dschuff dschuff Apr 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A generated JS file can have multiple lines. Mappings for each line are separated by a ; character. But wasm files are treated as if they have only one line, and the column number is treated as the binary offset. I'll add a comment.

}
if (next == ',' || next == '\"') {
hasSymbol = false;
break;
Expand Down
1 change: 1 addition & 0 deletions test/gtest/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
50 changes: 47 additions & 3 deletions test/gtest/source-map.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

#include "source-map.h"
#include "print-test.h"
#include "gmock/gmock-matchers.h"
#include "gtest/gtest.h"

using namespace wasm;
Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand Down
Loading