fix(c): check alloc/snprintf results on the checkpoint-load path (#798) - #1101
Merged
Conversation
…-load path Reported by ZacharyZcR in issue JustVugg#798 (the unchecked reallocs and the ignored snprintf return); all credit for finding those goes to ZacharyZcR. The strdup pair and the json.h initial-malloc guards are our own additions on the same path. Scope: the sites enumerated below only -- NOT every allocation site on the checkpoint-load path; known unguarded siblings elsewhere in st.h and json.h are deliberately left to their own change. Paths that ignored a fallible call's result: st.h the colibri.fmt stamp table growth (fmt_name/fmt_val) assigned realloc's result straight back over the original pointer, and the two stamp strings' strdup results were stored unchecked -- a dropped fmt_val is worse than a crash: st_fmt_stamp() would return NULL for a tensor that IS stamped, silently disabling qt_verify_fmt_stamp's format check for that tensor. json.h j_parse_val's object (keys/kids) and array (kids) growth assigned realloc's result straight back; on failure the very next statement stores through the NULL. The same function's initial cap=8 keys/kids mallocs had the same defect and get the same guard. route_trace.h rt_save ignored snprintf's return building "<path>.tmp": an overlong path would silently write-then-rename to a TRUNCATED path instead of the caller's, and a negative (encoding-error) return leaves the buffer indeterminate, so proceeding would fopen whatever bytes happen to be there. Both refuse through the same path. tests/test_798_guards.c exercises every guard above with real failure injection (malloc/realloc/strdup/snprintf shadowed to fail on a specific, documented call ordinal, every other call passed through), plus controls, and asserts each refusal's own diagnostic in a forked child so the refusal is provably the guard's and not a downstream failure on the bad value. Registered as a Makefile test gate.
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
2 participants
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.
Authored by Fable 5 in Claude Code, analysis in partnership with @monotophic
This implements the checkpoint-load-path defect set surfaced in #798 —
credit for the find belongs to @ZacharyZcR (per our comment there: we'd
be glad to see the broader #798 work land with their authorship; this PR
covers only the load-path correctness subset, independently re-derived).
Unchecked
realloc/strdupresults inc/st.handc/json.hNULL-storeon allocation failure mid-load (SIGSEGV or corrupted state), and
c/route_trace.h's stats-save builds its temp path with an uncheckedsnprintf— a too-long or encoding-failed path would silently write to aDIFFERENT file than asked. All sites on the load path now fail cleanly
with named diagnostics; a truncated or failed path refuses instead of
landing elsewhere.
Scope, stated plainly: the #798-named sites on the checkpoint-load
path. Other unchecked allocation sites exist in these files and are
deliberately out of scope here.
Behavioral contract
failure, never a NULL-store/SIGSEGV, never partial silent state.
return, before any filesystem touch; nothing is written to an
unintended path.
Capstone matrix
make test-cexit 0 (ALL PASS);make check+ METAL suites green on final headFuller matrix and review record
Two-reviewer roster (blind validator + deep auditor). Review changes, landed with produce-first evidence: (1) `j_parse_val`'s initial keys/kids allocations were still unguarded while later ones were — reproduced by injection (SIGSEGV), then guarded per file convention; (2) negative `snprintf` return now takes the same refusal path (seam-injected -1 test); (3) commit prose rescoped from all-sites to the #798 sites. Sibling unchecked sites recorded out-of-scope: st.h:147,468,565,584; json.h:41,49 (line refs at ad79236). Windows lane compiles out the fork-based injection tests (existing repo idiom); guards themselves are platform-independent.Durable vs current-state: guards are durable; sibling line references
are current-state (base ad79236, 2026-08-18).