diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 86083e7..92a8357 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -19,6 +19,9 @@ jobs: name: ${{ matrix.name }} steps: - uses: actions/checkout@v4 + - name: Lint -- forbid assert(side-effect) + if: runner.os == 'Linux' + run: python3 tests/scripts/check_assert_side_effects.py - name: Configure run: cmake -B build -DCMAKE_BUILD_TYPE=Release - name: Build diff --git a/.gitignore b/.gitignore index 1ade37a..7c7c52a 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,13 @@ build/ build-*/ cmake-build-*/ +# CTest run outputs (when ctest is invoked outside of build/) +Testing/ + +# Python bytecode +__pycache__/ +*.pyc + # Docs build docs/_build/ diff --git a/tests/scripts/check_assert_side_effects.py b/tests/scripts/check_assert_side_effects.py new file mode 100755 index 0000000..42f1b7e --- /dev/null +++ b/tests/scripts/check_assert_side_effects.py @@ -0,0 +1,86 @@ +#!/usr/bin/env python3 +# Fails when assert(...) wraps a function call with side effects. +# +# Background: under -DNDEBUG (CMake's default for Release) the assert macro +# expands to ((void)0) and the wrapped expression is not evaluated. Any work +# done inside assert() is silently dropped. This has cost the project two +# CI rounds: +# - dae8a50: int-truncation in test_merkle / test_dict Debug builds +# - 6d8087f: test_delta double-free under Release / Windows MSVC +# +# Rule: tests must capture the call result first, then assert on it: +# int rc = call(...); +# assert(rc == EXPECTED); +# +# This script detects the dangerous form by matching assert() that wraps a +# call to a function whose name contains a side-effect verb. Pure queries +# (_equal, _match, _verify, _has_, _is_, _root, _id, _hash, _attest_name, +# memcmp, strcmp, ...) are allowed. + +import re +import sys +from pathlib import Path + +SIDE_EFFECT_VERBS = ( + "encode", "decode", "parse", "serialize", "deserialize", + "build", "init", "write", "read_file", "attach", "extract", + "compress", "decompress", "create", "destroy", "open", "close", + "flush", "push", "pop", "append", "insert", "remove", "update", + "store", "load", "put", "finalize", "process", "run", "step", + "alloc", "free", "register", "submit", "commit", "rollback", +) + +SCAN_DIRS = ("tests/src", "lib/src", "cli/src", "src") + +assert_call_re = re.compile( + r"assert\s*\(\s*([A-Za-z_][A-Za-z0-9_]*)\s*\(" +) + +verb_re = re.compile( + r"(?:^|_)(" + "|".join(SIDE_EFFECT_VERBS) + r")(?:_|$)" +) + + +def scan(root: Path) -> list[tuple[Path, int, str, str]]: + findings = [] + for d in SCAN_DIRS: + base = root / d + if not base.is_dir(): + continue + for path in sorted(base.rglob("*.c")): + for lineno, line in enumerate(path.read_text(encoding="utf-8", + errors="replace").splitlines(), + start=1): + # Skip comments quickly. Not perfect but adequate here. + stripped = line.lstrip() + if stripped.startswith("//") or stripped.startswith("*"): + continue + m = assert_call_re.search(line) + if not m: + continue + ident = m.group(1) + if verb_re.search(ident): + findings.append((path, lineno, ident, line.rstrip())) + return findings + + +def main() -> int: + repo_root = Path(__file__).resolve().parents[2] + findings = scan(repo_root) + if not findings: + print("OK: no side-effecting asserts found.") + return 0 + print("ERROR: assert() must not wrap calls with side effects.", file=sys.stderr) + print("Under -DNDEBUG (Release builds) the call is dropped, leaving", file=sys.stderr) + print("output parameters uninitialised and the test silently no-op.", file=sys.stderr) + print("Convert to: int rc = call(...); assert(rc == EXPECTED);", file=sys.stderr) + print(file=sys.stderr) + for path, lineno, ident, line in findings: + rel = path.relative_to(repo_root) + print(f"{rel}:{lineno}: {ident}", file=sys.stderr) + print(f" {line.strip()}", file=sys.stderr) + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/src/test_ots.c b/tests/src/test_ots.c index ab5a862..1ec325f 100644 --- a/tests/src/test_ots.c +++ b/tests/src/test_ots.c @@ -19,7 +19,8 @@ static void test_varint_roundtrip(void) size_t n = uc2_ots_varint_encode(cases[i], buf); uint64_t got; size_t consumed; - assert(uc2_ots_varint_decode(buf, n, &got, &consumed) == UC2_OTS_OK); + int rc = uc2_ots_varint_decode(buf, n, &got, &consumed); + assert(rc == UC2_OTS_OK); assert(got == cases[i]); assert(consumed == n); } @@ -30,7 +31,8 @@ static void test_varint_truncated(void) uint8_t buf[] = { 0x80, 0x80 }; /* unterminated */ uint64_t v; size_t c; - assert(uc2_ots_varint_decode(buf, 2, &v, &c) == UC2_OTS_ERR_TRUNCATED); + int rc = uc2_ots_varint_decode(buf, 2, &v, &c); + assert(rc == UC2_OTS_ERR_TRUNCATED); } static void test_varint_noncanonical(void) @@ -39,7 +41,8 @@ static void test_varint_noncanonical(void) uint8_t buf[] = { 0x80, 0x00 }; uint64_t v; size_t c; - assert(uc2_ots_varint_decode(buf, 2, &v, &c) == UC2_OTS_ERR_NONCANONICAL); + int rc = uc2_ots_varint_decode(buf, 2, &v, &c); + assert(rc == UC2_OTS_ERR_NONCANONICAL); } static void test_varint_overflow_64bit(void) @@ -52,7 +55,8 @@ static void test_varint_overflow_64bit(void) }; uint64_t v; size_t c; - assert(uc2_ots_varint_decode(buf, sizeof buf, &v, &c) == UC2_OTS_ERR_OVERFLOW); + int rc = uc2_ots_varint_decode(buf, sizeof buf, &v, &c); + assert(rc == UC2_OTS_ERR_OVERFLOW); } static void test_varint_max_64bit(void) @@ -62,7 +66,8 @@ static void test_varint_max_64bit(void) size_t n = uc2_ots_varint_encode((uint64_t)-1, buf); uint64_t v; size_t c; - assert(uc2_ots_varint_decode(buf, n, &v, &c) == UC2_OTS_OK); + int rc = uc2_ots_varint_decode(buf, n, &v, &c); + assert(rc == UC2_OTS_OK); assert(v == (uint64_t)-1); } @@ -99,8 +104,8 @@ static void test_file_bad_magic(void) uint8_t hash_op; const uint8_t *l, *b; size_t ll, bl; - assert(uc2_ots_parse_file(buf, sizeof buf, &hash_op, &l, &ll, &b, &bl) - == UC2_OTS_ERR_BAD_MAGIC); + int rc = uc2_ots_parse_file(buf, sizeof buf, &hash_op, &l, &ll, &b, &bl); + assert(rc == UC2_OTS_ERR_BAD_MAGIC); } struct cb_ctx {