diff --git a/inkcpp_compiler/json_compiler.cpp b/inkcpp_compiler/json_compiler.cpp index e6550f0e..ce564366 100644 --- a/inkcpp_compiler/json_compiler.cpp +++ b/inkcpp_compiler/json_compiler.cpp @@ -30,8 +30,25 @@ void json_compiler::compile( const nlohmann::json& input, emitter* output, compilation_results* results ) { + /* Look the top-level keys up rather than reaching for them with operator[]. + On a const json, operator[] with a missing key is an assert(), which + aborts the process instead of reporting an error, and any JSON at all can + be handed to a compiler that accepts "a .json file". Every other failure + here is an ink_exception the caller can catch. */ + if (! input.is_object()) { + throw ink_exception("this JSON is not an Ink story: its top level is not an object"); + } + const auto version_itr = input.find("inkVersion"); + if (version_itr == input.end()) { + throw ink_exception("this JSON is not an Ink story: it has no \"inkVersion\""); + } + const auto root_itr = input.find("root"); + if (root_itr == input.end()) { + throw ink_exception("this JSON is not an Ink story: it has no \"root\""); + } + // Get the runtime version - _ink_version = input["inkVersion"]; + _ink_version = *version_itr; // Start the output set_results(results); @@ -45,7 +62,7 @@ void json_compiler::compile( _emitter->set_list_meta(_list_meta); } // Compile the root container - compile_container(input["root"], 0, 0); + compile_container(*root_itr, 0, 0); // finalize _emitter->finish(_next_container_index); @@ -133,6 +150,14 @@ void json_compiler::compile_container( const std::string& name_override ) { + /* Every container is an array whose last element is its metadata object, so + rbegin() and the end() - 1 below are only meaningful for a non-empty + array. Reaching them with anything else dereferences an end iterator. + This guard covers the root and every nested container in one place. */ + if (! container.is_array() || container.empty()) { + throw ink_exception("malformed Ink story: a container must be a non-empty array"); + } + // Grab metadata from the last object in this container container_meta meta; bool is_knot = name_override != "" && index_in_parent == -1; diff --git a/inkcpp_test/CMakeLists.txt b/inkcpp_test/CMakeLists.txt index 6e8cbd5b..c6cd1397 100644 --- a/inkcpp_test/CMakeLists.txt +++ b/inkcpp_test/CMakeLists.txt @@ -33,6 +33,7 @@ add_executable( Fixes.cpp Migration.cpp MultiRunner.cpp + NotAnInkStory.cpp ) target_link_libraries(inkcpp_test PUBLIC inkcpp inkcpp_compiler inkcpp_shared) diff --git a/inkcpp_test/NotAnInkStory.cpp b/inkcpp_test/NotAnInkStory.cpp new file mode 100644 index 00000000..63df6699 --- /dev/null +++ b/inkcpp_test/NotAnInkStory.cpp @@ -0,0 +1,88 @@ +#include "catch.hpp" + +#include +#include + +#include +#include + +/* Anything at all can be handed to a compiler that accepts "a .json file": a + * settings file, an API response, a package.json. Before the guards in + * json_compiler::compile() and compile_container(), reaching for the top-level + * keys with operator[] made that fatal rather than reportable - on a const + * nlohmann::json a missing key is an assert(), so the process aborted with + * assertions on and dereferenced an end iterator with them off. + * + * These cases are the ones that were measured to abort, segfault, or silently + * emit a bogus binary. They are all cheap to state, and the failure they guard + * against is the kind that only shows up in someone else's crash report. */ + +using namespace ink::compiler; + +namespace +{ + std::string compile_or_throw(const char* json) + { + std::istringstream in(json); + std::ostringstream out; + run(in, out); + return out.str(); + } +} // namespace + +SCENARIO("JSON that is not an Ink story is reported, not fatal", "[compiler]") +{ + GIVEN("a document whose top level is not an object") + { + auto source = GENERATE("[1, 2, 3]", "\"just a string\"", "42", "null"); + THEN("compiling it raises rather than aborting") + { + REQUIRE_THROWS_AS(compile_or_throw(source), ink::ink_exception); + } + } + + GIVEN("an object without the keys a story must have") + { + // Previously an assert() on the missing key, so an abort() with + // assertions enabled. + auto source = GENERATE( + "{}", // neither key + "{\"inkVersion\": 21}", // no root + "{\"root\": [[\"done\"], null]}", // no inkVersion + "{\"name\": \"not a story\", \"value\": 42}" // something else entirely + ); + THEN("compiling it raises rather than aborting") + { + REQUIRE_THROWS_AS(compile_or_throw(source), ink::ink_exception); + } + } + + GIVEN("a root that is not a non-empty array") + { + // compile_container() takes rbegin() and end() - 1, which are only + // meaningful for a non-empty array. An empty array or an object used to + // dereference an end iterator; a scalar root silently produced a small + // binary that was not a story. + auto source = GENERATE( + "{\"inkVersion\": 21, \"root\": []}", "{\"inkVersion\": 21, \"root\": {}}", + "{\"inkVersion\": 21, \"root\": \"nope\"}", "{\"inkVersion\": 21, \"root\": 42}" + ); + THEN("compiling it raises rather than aborting") + { + REQUIRE_THROWS_AS(compile_or_throw(source), ink::ink_exception); + } + } + + GIVEN("a minimal but valid story") + { + THEN("it still compiles, so the guards have not become too strict") + { + std::string binary; + REQUIRE_NOTHROW( + binary = compile_or_throw("{\"inkVersion\": 21, \"root\": [[\"done\"], null], " + "\"listDefs\": {}}") + ); + REQUIRE(binary.size() > 0); + } + } +}