Report JSON that is not an Ink story instead of aborting - #166
Open
jerrytron wants to merge 1 commit into
Open
Conversation
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
json_compiler::compile()reached forinkVersionandrootwithoperator[]. On a constnlohmann::jsona missing key isassert(it != end()), so with assertions enabled the process aborts, and with them disabled it dereferences an end iterator.compile_container()then tookrbegin()andend() - 1without checking the container was a non-empty array.Anything at all can be handed to a compiler that accepts "a .json file" — a settings file, an API response, a
package.json. Today that is fatal rather than reportable.Measured
18 malformed inputs against the current
master:inkVersionor noroot,{}"root": [],"root": {}"root": "x","root": 42This change
Guarded lookups at the entry, plus one non-empty-array check in
compile_container()that covers the root and every nested container in a single place. Both raiseink_exception, which every other failure in this compiler already uses, so callers can report the problem instead of losing the process.All 18 now return a message. 77 real stories compile to byte-identical output, and
ctestpasses.Where it was found
An embedded target, where the abort was a panic and a reboot, and a WebAssembly build, where it killed the module for the rest of the page load.
I have not added a Catch2 case for this — happy to if you would like one; it would fit as a compiler test asserting
ink_exceptionon a handful of non-story documents.