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 6 commits
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
14 changes: 12 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,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");
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
4 changes: 3 additions & 1 deletion 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 All @@ -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
Expand All @@ -29,6 +29,8 @@ set(unittest_SOURCES

if(BUILD_FUZZTEST)
set(unittest_SOURCES ${unittest_SOURCES} type-domains.cpp)
else()
set(unittest_SOURCES ${unittest_SOURCES} source-map.cpp)
Copy link
Copy Markdown
Member Author

@dschuff dschuff Apr 21, 2025

Choose a reason for hiding this comment

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

@tlively I've had to exclude this file from fuzztest builds, because it uses the gmock-matchers.h header (gmock is included with "standard" gtest but not with the one bundled with fuzztest). I tried including the gmock header along with the fuzztest gtest, but it seems to depend on some kind of internal gtest bit that the fuzztest gtest doesn't have. Do you know if fuzztest's bundled gtest is some sort of subset, or if they have any recommendations for use with gtest features they don't support themselves? Or maybe this is just WAI, and the intention is that fuzztests are logically separate enough from regular unit tests that it just makes sense to build them separately.

Copy link
Copy Markdown
Member

@tlively tlively Apr 23, 2025

Choose a reason for hiding this comment

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

No, I don't know the details of why fuzztest's bundled gtest is different from normal gtest, nor have I seen any recommendations for this. It's definitely not WAI, though. fuzztests are supposed to coexist peacefully with normal gtests AFAICT.

It's probably fine excluding the file to work around the problem for now. The fuzztest build isn't important enough (yet) to spend time on. It would be good to add a comment explaining the problem, though.

endif()

# suffix_tree.cpp includes LLVM header using std::iterator (deprecated in C++17)
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