Stop returning past CLEANUP, and validate the arguments every sibling validates
Closes internal-consistency items 16 and 17. Ten *_RETURN macros sat inside ATTEMPT blocks, which return past CLEANUP and skip every release in it. The one that mattered was the success path of akgl_get_json_tilemap_property: it leaked two of the string pool's 256 entries on every lookup that *found* what it was asked for, and a map load does that several times per layer. tests/tilemap.c now runs each of its three paths -- found, absent, wrong type -- twice the pool size and asserts the pool is where it started. Against the old code that test does not merely fail, it segfaults, which is Defects item 30 seen from the outside: pool exhaustion arriving as a NULL strncpy rather than as AKGL_ERR_HEAP. Two of the conversions needed more than swapping the macro. In akgl_get_json_tilemap_property a plain break would have fallen through to the "property not found" FAIL_RETURN after FINISH, reporting a miss for something found, so the success path sets a flag. In akgl_collide_rectangles the eight early exits are followed by `*collide = false;`, which would have overwritten the hit that broke out; each corner test writes the flag itself, so that line is gone rather than moved. The same function also released its scratch string once per loop iteration while continuing to use it, so the slot was free while still live -- one claim now covers the whole scan. akgl_controller_default is the other behavioural one: its SUCCEED_RETURN was the last statement in the ATTEMPT block, so the path that falls out of FINISH reached the closing brace of a non-void function. scripts/check_error_protocol.py keeps both rules enforced -- a *_RETURN inside ATTEMPT, and a return out of a HANDLE block -- as the error_protocol test. Neither produces a compiler diagnostic and neither fails a test run until the pool it drains is empty, which is why both have already shipped once. For item 17: the eight typed JSON accessors that validated their container and then wrote through dest unconditionally now check key and dest as the two string accessors always did; the null physics backend checks its actors like the arcade one; and akgl_render_2d_frame_start, _frame_end and _shutdown check self, which the first two read straight through. tests/renderer.c calls all three with NULL, which segfaulted before. 25/25 pass, memcheck clean, reindent --check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
145
scripts/check_error_protocol.py
Executable file
145
scripts/check_error_protocol.py
Executable file
@@ -0,0 +1,145 @@
|
||||
#!/usr/bin/env python3
|
||||
"""Enforce the two akerror control-flow rules whose failure mode is silent.
|
||||
|
||||
AGENTS.md states both, and both have already cost this library a leak:
|
||||
|
||||
1. Never use a ``*_RETURN`` macro inside an ``ATTEMPT`` block. It returns past
|
||||
``CLEANUP``, so every release, ``fclose`` and free in ``CLEANUP`` is
|
||||
skipped. ``akgl_get_json_tilemap_property`` leaked two pooled strings on
|
||||
every *successful* lookup this way.
|
||||
|
||||
2. Never ``return`` from inside a ``HANDLE`` block. ``FINISH`` is what carries
|
||||
``RELEASE_ERROR``, so leaving early never gives the handled context back to
|
||||
``AKERR_ARRAY_ERROR``. That cost ``akgl_path_relative`` one slot per call
|
||||
and killed the process on the 129th.
|
||||
|
||||
Neither produces a compiler diagnostic, and neither shows up in a passing test
|
||||
run until the pool it drains is empty. Hence a linter.
|
||||
|
||||
Usage: check_error_protocol.py <file-or-directory> [...]
|
||||
|
||||
Exits 0 when clean, 1 on a violation.
|
||||
"""
|
||||
|
||||
import os
|
||||
import re
|
||||
import sys
|
||||
|
||||
# A ``*_RETURN`` macro invocation: SUCCEED_RETURN, FAIL_RETURN, FAIL_ZERO_RETURN,
|
||||
# FAIL_NONZERO_RETURN, and anything later shaped like them.
|
||||
RETURN_MACRO = re.compile(r"\b[A-Z][A-Z0-9_]*_RETURN\s*\(")
|
||||
BARE_RETURN = re.compile(r"\breturn\b")
|
||||
ATTEMPT_OPEN = re.compile(r"\bATTEMPT\s*\{")
|
||||
CLEANUP_OPEN = re.compile(r"\}\s*CLEANUP\s*\{")
|
||||
HANDLE_OPEN = re.compile(r"\}\s*HANDLE(?:_GROUP)?\s*\(")
|
||||
FINISH_ANY = re.compile(r"\}\s*FINISH(?:_NORETURN)?\s*\(")
|
||||
|
||||
|
||||
def strip_comments(text):
|
||||
"""Blank out comments, keeping line numbering intact.
|
||||
|
||||
Comments are removed rather than skipped because several of these keywords
|
||||
appear in prose -- "which should never return AKERR_KEY" is a sentence, not
|
||||
a violation.
|
||||
"""
|
||||
out = []
|
||||
i = 0
|
||||
n = len(text)
|
||||
while i < n:
|
||||
ch = text[i]
|
||||
if ch == '"' or ch == "'":
|
||||
quote = ch
|
||||
out.append(ch)
|
||||
i += 1
|
||||
while i < n:
|
||||
if text[i] == "\\" and (i + 1) < n:
|
||||
out.append(" ")
|
||||
i += 2
|
||||
continue
|
||||
out.append(text[i])
|
||||
if text[i] == quote:
|
||||
i += 1
|
||||
break
|
||||
i += 1
|
||||
continue
|
||||
if text.startswith("/*", i):
|
||||
j = text.find("*/", i + 2)
|
||||
j = n if j == -1 else (j + 2)
|
||||
out.append("".join(c if c == "\n" else " " for c in text[i:j]))
|
||||
i = j
|
||||
continue
|
||||
if text.startswith("//", i):
|
||||
j = text.find("\n", i)
|
||||
j = n if j == -1 else j
|
||||
out.append(" " * (j - i))
|
||||
i = j
|
||||
continue
|
||||
out.append(ch)
|
||||
i += 1
|
||||
return "".join(out)
|
||||
|
||||
|
||||
def check(path):
|
||||
findings = []
|
||||
lines = strip_comments(open(path, encoding="utf-8", errors="replace").read()).split("\n")
|
||||
in_attempt = False
|
||||
in_handle = False
|
||||
|
||||
for number, line in enumerate(lines, 1):
|
||||
if ATTEMPT_OPEN.search(line):
|
||||
in_attempt = True
|
||||
continue
|
||||
if CLEANUP_OPEN.search(line):
|
||||
in_attempt = False
|
||||
if HANDLE_OPEN.search(line):
|
||||
in_handle = True
|
||||
continue
|
||||
if FINISH_ANY.search(line):
|
||||
in_handle = False
|
||||
continue
|
||||
|
||||
if in_attempt and RETURN_MACRO.search(line):
|
||||
findings.append(
|
||||
(number, "a *_RETURN macro inside an ATTEMPT block returns past CLEANUP; "
|
||||
"use the matching *_BREAK variant"))
|
||||
if in_handle and BARE_RETURN.search(line):
|
||||
findings.append(
|
||||
(number, "returning from inside a HANDLE block skips the RELEASE_ERROR that "
|
||||
"FINISH carries; set a flag and act on it after FINISH"))
|
||||
return findings
|
||||
|
||||
|
||||
def sources(targets):
|
||||
for target in targets:
|
||||
if os.path.isdir(target):
|
||||
for root, _dirs, files in os.walk(target):
|
||||
for name in sorted(files):
|
||||
if name.endswith(".c") and not name.endswith("~"):
|
||||
yield os.path.join(root, name)
|
||||
else:
|
||||
yield target
|
||||
|
||||
|
||||
def main(argv):
|
||||
if len(argv) < 2:
|
||||
print(__doc__, file=sys.stderr)
|
||||
return 1
|
||||
|
||||
total = 0
|
||||
for path in sources(argv[1:]):
|
||||
for number, message in check(path):
|
||||
print("%s:%d: %s" % (path, number, message), file=sys.stderr)
|
||||
total += 1
|
||||
|
||||
if total:
|
||||
print("", file=sys.stderr)
|
||||
print("check_error_protocol: %d violation(s). See the Error-Handling Protocol "
|
||||
"section of AGENTS.md." % total, file=sys.stderr)
|
||||
return 1
|
||||
|
||||
print("check_error_protocol: clean")
|
||||
return 0
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
sys.exit(main(sys.argv))
|
||||
Reference in New Issue
Block a user