From 5a7f38b43ed3c55094f8f77f66da9e947164ccd5 Mon Sep 17 00:00:00 2001 From: Jerry Belich Date: Wed, 9 Sep 2026 22:47:21 -0700 Subject: [PATCH 1/2] compiler: reject JSON that is not an Ink story instead of aborting Anything at all can be handed to a compiler that accepts "a .json file": a settings file, an API response, a package.json. Reaching for the top-level keys with operator[] made that fatal. On a const nlohmann::json a missing key is assert(it != end()), so with assertions enabled the process aborts, and with them disabled it dereferences an end iterator. compile_container() then took rbegin() and end() - 1 without checking the container was a non-empty array. Measured over 18 malformed inputs, before this change: 4 aborted (missing inkVersion or root, and {}), 2 segfaulted (a root of [] or {}), and 2 silently emitted a bogus 64-byte binary from a scalar root. Now they all raise ink_exception, which every other failure in this compiler already uses, so a caller can report the problem instead of losing the process. The container guard covers the root and every nested container in one place. Reported on an embedded target where the abort was a panic and a reboot, and in a WebAssembly build where it killed the module for the rest of the page. Co-Authored-By: Claude Opus 5 --- inkcpp_compiler/json_compiler.cpp | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) 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; From 9b2405cdabcda26bb9b75759fbdb6edcf6389cd6 Mon Sep 17 00:00:00 2001 From: Jerry Belich Date: Thu, 10 Sep 2026 09:53:52 -0700 Subject: [PATCH 2/2] test: cover JSON that is not an Ink story Catch2 coverage for the guards, so a future change cannot quietly restore the abort. Twelve documents across the three shapes that were measured to fail before them: a top level that is not an object, an object missing inkVersion or root, and a root that is not a non-empty array. Each is required to raise ink_exception rather than abort or segfault. A minimal valid story is compiled alongside them, so the guards cannot become too strict without the suite noticing. Built against master without the guards, this fails 9 assertions and takes a SIGSEGV on the empty-root case. Co-Authored-By: Claude Opus 5 --- inkcpp_test/CMakeLists.txt | 1 + inkcpp_test/NotAnInkStory.cpp | 88 +++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 inkcpp_test/NotAnInkStory.cpp 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); + } + } +}