Gate every push on cppcheck: 6 findings, all in src/error.c #33

New Issue

tachikoma · 2026-08-05T15:53:48-04:00

tachikoma commented

2026-08-05 15:53:48 -04:00

Add a cppcheck static-analysis gate to this repository: run on every push, fail the
pipeline on any finding, and run again in the pre-push hook so it fails on the workstation
first. This is one of four -- akbasic, libakerror, libakstdlib and libakgl all get the same
gate, built to the same plan, and this issue is the libakerror half.

libakerror goes first because it is the smallest tree in the set: one source file, six
findings, no submodules, and a CI job with nothing to check out. Whatever scripts/cppcheck.sh
and the static_analysis job end up looking like here is what the other three copy.

Measured, before any change

cppcheck 2.19.0, --std=c99 --platform=unix64 --enable=style, over src include:

severity	count
error	0
warning	0
style	6

All six are in src/error.c, and none of them is a defect a caller could reach:

src/error.c:644:15: style: Condition 'refused!=0' is always true [knownConditionTrueFalse]
src/error.c:393:9:  style: The scope of the variable 'oldid' can be reduced. [variableScope]
src/error.c:393:15: style: Variable 'oldid' is assigned a value that is never used. [unreadVariable]
src/error.c:145:51: style: Parameter 'ptr' can be declared as pointer to const [constParameterPointer]
src/error.c:622:47: style: Parameter 'name' can be declared as pointer to const [constParameterPointer]
src/error.c:354:64: style: Parameter 'errctx' can be declared as pointer to const [constParameterCallback]

Three of them are worth reading before reaching for a fix.

refused != 0 at 644 is reported always-true because refused = 1 at 641 sits inside an
ATTEMPT block that FINISH_NORETURN closes at 643. cppcheck is not following the error
macros -- the same blind spot gcov has, and for the same reason: they expand at the call
site. The condition is not always true; expect this to be an inline suppression with that
reason written down.

oldid at 393 in akerr_release_error is assigned and never read. That is either dead
code or a check somebody meant to write, and it should be read before it is deleted.

The constParameterCallback at 354 is akerr_default_handler_unhandled_error, and
cppcheck says so itself: const-qualifying it means casting the function pointer at 247.
Another suppression candidate rather than a change.

The remaining three are mechanical, but two of them touch published declarations --
akerr_valid_error_address's ptr and akerr_name_for_status's name. Const-qualifying
a pointer parameter is source-compatible for callers and does not move the soname, but the
header and the definition move together, and the C guide requires their parameter names
match.

Scope for this repository

SCOPE=( src include )

No examples/, tools/ or util/ in this tree. tests/ (38 files) is excluded per the
plan below.

Work

Fix or inline-suppress all six findings. Every suppression carries its reason on the
line above it.
Add scripts/cppcheck.sh with the scope above.
Add the static_analysis job to .gitea/workflows/ci.yaml.
Prove the gate red once on a throwaway branch, then delete the branch.
Wire it into the pre-push hook. This repository has no pre-push hook and no
.githooks/ at all -- that is #32, and this box waits on it.
Record the tests/ exclusion in TODO.md with its consequence.

The plan (identical in all four repositories)

The gate

cppcheck <scope> \
    --std=c99 \
    --platform=unix64 \
    --enable=style \
    --inline-suppr \
    --error-exitcode=1 \
    --quiet

Four deviations from the sketch, each with a measurement behind it:

An explicit scope, not .. cppcheck . descends into deps/ and into whatever
build*/ happens to be lying around. Measured in libakstdlib: cppcheck . -i tests
reports 30 findings, 24 of them inside deps/libakerror -- the same six defects this
plan already gates in libakerror's own pipeline, reported a second time from a
repository that cannot fix them. akbasic and libakgl vendor SDL, SDL_image, SDL_mixer,
SDL_ttf, jansson, libccd and clay, which is thousands of findings and minutes of
runtime for code nobody here owns. Build directories are the other half: the mutation
harness copies the whole tree into one, and the pre-push hook runs on a workstation
where two or three of them exist.
--error-exitcode=1. cppcheck exits 0 when it has findings. Verified on 2.19.0:
exit 0 with six findings reported, exit 1 with the flag, exit 0 on a clean tree. Without
this flag the job is green no matter what it prints.
--inline-suppr, and no baseline file. Every finding is either fixed or carries a
// cppcheck-suppress <id> on the line above it with the reason in a comment. A
suppression list in a separate file is a list nobody reads; a suppression at the site is
a documented, tracked defect, which is what the engineering rules ask for. There is no
"current findings" baseline: the tree goes clean before the gate turns on.
-i tests, as sketched -- deferred, not decided against. Tests are where a leak is
most likely to hide, and this excludes them. That is a named deferral for TODO.md, not a
judgement that test code does not matter. (-i 'tests/*' and -i tests both work on
2.19.0; an explicit scope makes the question moot.)

Where it lives

scripts/cppcheck.sh -- one definition that CI and the pre-push hook both run, so a push
that would fail CI fails on the workstation first. Same shape as the existing
scripts/coverage.py, scripts/memcheck.sh and scripts/mutation_test.py.

#!/usr/bin/env bash
#
# Static analysis gate. Run by .gitea/workflows/ci.yaml and by the pre-push hook,
# so both check the same thing.

set -u

if ! command -v cppcheck > /dev/null 2>&1; then
    echo "cppcheck: not installed" >&2
    exit 127
fi

root=$(git rev-parse --show-toplevel) || exit 1
cd "$root" || exit 1

exec cppcheck "${SCOPE[@]}" \
    --std=c99 \
    --platform=unix64 \
    --enable=style \
    --inline-suppr \
    --error-exitcode=1 \
    --quiet \
    "$@"

SCOPE is the repository's own C and nothing else -- named per repository below.

CI

A new static_analysis job:

  static_analysis:
    runs-on: ubuntu-latest
    steps:
      - name: Check out repository code
        uses: actions/checkout@v4
        # No submodules on purpose: the scope is this repository's own C, and the
        # dependencies gate themselves in their own pipelines.
      - name: dependencies
        run: |
          sudo apt-get update -y
          sudo apt-get install -y cppcheck
      # Findings move with the version. Record which cppcheck gated this run, so a
      # red job that nobody's commit caused can be identified as such in one look.
      - run: cppcheck --version
      - name: static analysis
        run: scripts/cppcheck.sh
      - run: echo "🍏 This job's status is ${{ job.status }}."

The check itself is 2 s (libakgl) to 6 s (akbasic) measured on this workstation; the apt
install dominates the job.

Version drift is a real cost and is accepted rather than engineered around: a newer
cppcheck finds new things, and the gate goes red on a push that did not cause it. That is
fix-forward. Printing the version is what makes it diagnosable. Do not pin an old cppcheck
to keep the tree quiet.

pre-push

First step in the hook -- it is the cheapest gate there by an order of magnitude. Skipped
with a warning when cppcheck is not installed, matching the doxygen precedent in
libakstdlib's hook: a hook that fails closed on a missing optional tool only teaches
everyone to pass --no-verify. CI is the hard gate.

Order

libakerror (6 findings) -- smallest tree, one source file. Proves the script and the
job shape; the other three copy it.
libakstdlib (15, one of them a real error) -- already has .githooks/pre-push.
akbasic (102) and libakgl (103) in parallel -- both need a pre-push hook first.

Acceptance, per repository

scripts/cppcheck.sh exits 0 on a clean checkout with no suppressions beyond those whose
reason is written at the site.
The static_analysis job exists and has been proven red once, by pushing a deliberate
finding on a throwaway branch and watching the pipeline fail.
The pre-push hook runs it -- or the hook bug is filed, linked here, and this repository's
wiring waits on it.
Every remaining finding is either fixed or inline-suppressed with a reason.
The -i tests deferral is recorded in TODO.md with its consequence.

Measured with cppcheck 2.19.0 on 2026-08-05.

Add a `cppcheck` static-analysis gate to this repository: run on every push, fail the pipeline on any finding, and run again in the pre-push hook so it fails on the workstation first. This is one of four -- akbasic, libakerror, libakstdlib and libakgl all get the same gate, built to the same plan, and this issue is the libakerror half. libakerror goes first because it is the smallest tree in the set: one source file, six findings, no submodules, and a CI job with nothing to check out. Whatever `scripts/cppcheck.sh` and the `static_analysis` job end up looking like here is what the other three copy. ## Measured, before any change cppcheck 2.19.0, `--std=c99 --platform=unix64 --enable=style`, over `src include`: | severity | count | | --- | --- | | error | 0 | | warning | 0 | | style | 6 | All six are in `src/error.c`, and none of them is a defect a caller could reach: ``` src/error.c:644:15: style: Condition 'refused!=0' is always true [knownConditionTrueFalse] src/error.c:393:9: style: The scope of the variable 'oldid' can be reduced. [variableScope] src/error.c:393:15: style: Variable 'oldid' is assigned a value that is never used. [unreadVariable] src/error.c:145:51: style: Parameter 'ptr' can be declared as pointer to const [constParameterPointer] src/error.c:622:47: style: Parameter 'name' can be declared as pointer to const [constParameterPointer] src/error.c:354:64: style: Parameter 'errctx' can be declared as pointer to const [constParameterCallback] ``` Three of them are worth reading before reaching for a fix. `refused != 0` at 644 is reported always-true because `refused = 1` at 641 sits inside an `ATTEMPT` block that `FINISH_NORETURN` closes at 643. cppcheck is not following the error macros -- the same blind spot gcov has, and for the same reason: they expand at the call site. The condition is not always true; expect this to be an inline suppression with that reason written down. `oldid` at 393 in `akerr_release_error` is assigned and never read. That is either dead code or a check somebody meant to write, and it should be read before it is deleted. The `constParameterCallback` at 354 is `akerr_default_handler_unhandled_error`, and cppcheck says so itself: const-qualifying it means casting the function pointer at 247. Another suppression candidate rather than a change. The remaining three are mechanical, but two of them touch published declarations -- `akerr_valid_error_address`'s `ptr` and `akerr_name_for_status`'s `name`. Const-qualifying a pointer parameter is source-compatible for callers and does not move the soname, but the header and the definition move together, and the C guide requires their parameter names match. ## Scope for this repository ``` SCOPE=( src include ) ``` No `examples/`, `tools/` or `util/` in this tree. `tests/` (38 files) is excluded per the plan below. ## Work - [ ] Fix or inline-suppress all six findings. Every suppression carries its reason on the line above it. - [ ] Add `scripts/cppcheck.sh` with the scope above. - [ ] Add the `static_analysis` job to `.gitea/workflows/ci.yaml`. - [ ] Prove the gate red once on a throwaway branch, then delete the branch. - [ ] Wire it into the pre-push hook. **This repository has no pre-push hook and no `.githooks/` at all** -- that is #32, and this box waits on it. - [ ] Record the `tests/` exclusion in TODO.md with its consequence. --- # The plan (identical in all four repositories) ## The gate ```sh cppcheck <scope> \ --std=c99 \ --platform=unix64 \ --enable=style \ --inline-suppr \ --error-exitcode=1 \ --quiet ``` Four deviations from the sketch, each with a measurement behind it: 1. **An explicit scope, not `.`.** `cppcheck .` descends into `deps/` and into whatever `build*/` happens to be lying around. Measured in libakstdlib: `cppcheck . -i tests` reports 30 findings, 24 of them inside `deps/libakerror` -- the same six defects this plan already gates in libakerror's own pipeline, reported a second time from a repository that cannot fix them. akbasic and libakgl vendor SDL, SDL_image, SDL_mixer, SDL_ttf, jansson, libccd and clay, which is thousands of findings and minutes of runtime for code nobody here owns. Build directories are the other half: the mutation harness copies the whole tree into one, and the pre-push hook runs on a workstation where two or three of them exist. 2. **`--error-exitcode=1`.** cppcheck exits 0 when it has findings. Verified on 2.19.0: exit 0 with six findings reported, exit 1 with the flag, exit 0 on a clean tree. Without this flag the job is green no matter what it prints. 3. **`--inline-suppr`, and no baseline file.** Every finding is either fixed or carries a `// cppcheck-suppress <id>` on the line above it with the reason in a comment. A suppression list in a separate file is a list nobody reads; a suppression at the site is a documented, tracked defect, which is what the engineering rules ask for. There is no "current findings" baseline: the tree goes clean before the gate turns on. 4. **`-i tests`, as sketched -- deferred, not decided against.** Tests are where a leak is most likely to hide, and this excludes them. That is a named deferral for TODO.md, not a judgement that test code does not matter. (`-i 'tests/*'` and `-i tests` both work on 2.19.0; an explicit scope makes the question moot.) ## Where it lives `scripts/cppcheck.sh` -- one definition that CI and the pre-push hook both run, so a push that would fail CI fails on the workstation first. Same shape as the existing `scripts/coverage.py`, `scripts/memcheck.sh` and `scripts/mutation_test.py`. ```sh #!/usr/bin/env bash # # Static analysis gate. Run by .gitea/workflows/ci.yaml and by the pre-push hook, # so both check the same thing. set -u if ! command -v cppcheck > /dev/null 2>&1; then echo "cppcheck: not installed" >&2 exit 127 fi root=$(git rev-parse --show-toplevel) || exit 1 cd "$root" || exit 1 exec cppcheck "${SCOPE[@]}" \ --std=c99 \ --platform=unix64 \ --enable=style \ --inline-suppr \ --error-exitcode=1 \ --quiet \ "$@" ``` `SCOPE` is the repository's own C and nothing else -- named per repository below. ## CI A new `static_analysis` job: ```yaml static_analysis: runs-on: ubuntu-latest steps: - name: Check out repository code uses: actions/checkout@v4 # No submodules on purpose: the scope is this repository's own C, and the # dependencies gate themselves in their own pipelines. - name: dependencies run: | sudo apt-get update -y sudo apt-get install -y cppcheck # Findings move with the version. Record which cppcheck gated this run, so a # red job that nobody's commit caused can be identified as such in one look. - run: cppcheck --version - name: static analysis run: scripts/cppcheck.sh - run: echo "🍏 This job's status is ${{ job.status }}." ``` The check itself is 2 s (libakgl) to 6 s (akbasic) measured on this workstation; the apt install dominates the job. Version drift is a real cost and is accepted rather than engineered around: a newer cppcheck finds new things, and the gate goes red on a push that did not cause it. That is fix-forward. Printing the version is what makes it diagnosable. Do not pin an old cppcheck to keep the tree quiet. ## pre-push First step in the hook -- it is the cheapest gate there by an order of magnitude. Skipped with a warning when cppcheck is not installed, matching the doxygen precedent in libakstdlib's hook: a hook that fails closed on a missing optional tool only teaches everyone to pass `--no-verify`. CI is the hard gate. ## Order 1. **libakerror** (6 findings) -- smallest tree, one source file. Proves the script and the job shape; the other three copy it. 2. **libakstdlib** (15, one of them a real error) -- already has `.githooks/pre-push`. 3. **akbasic** (102) and **libakgl** (103) in parallel -- both need a pre-push hook first. ## Acceptance, per repository - `scripts/cppcheck.sh` exits 0 on a clean checkout with no suppressions beyond those whose reason is written at the site. - The `static_analysis` job exists and has been proven red once, by pushing a deliberate finding on a throwaway branch and watching the pipeline fail. - The pre-push hook runs it -- or the hook bug is filed, linked here, and this repository's wiring waits on it. - Every remaining finding is either fixed or inline-suppressed with a reason. - The `-i tests` deferral is recorded in TODO.md with its consequence. Measured with cppcheck 2.19.0 on 2026-08-05.

tachikoma added the hygiene blast-radius:low status::ready labels 2026-08-05 15:53:48 -04:00

tachikoma commented

2026-08-05 15:54:04 -04:00

Siblings, same plan:

akbasic: andrew/akbasic#48 (hook bug: andrew/akbasic#47)
libakerror: andrew/libakerror#33 (hook bug: andrew/libakerror#32)
libakstdlib: andrew/libakstdlib#45 (already has .githooks/pre-push)
libakgl: andrew/libakgl#83 (hook bug: andrew/libakgl#82)

Order: libakerror, then libakstdlib, then akbasic and libakgl in parallel.

Siblings, same plan: - akbasic: andrew/akbasic#48 (hook bug: andrew/akbasic#47) - libakerror: andrew/libakerror#33 (hook bug: andrew/libakerror#32) - libakstdlib: andrew/libakstdlib#45 (already has `.githooks/pre-push`) - libakgl: andrew/libakgl#83 (hook bug: andrew/libakgl#82) Order: libakerror, then libakstdlib, then akbasic and libakgl in parallel.

tachikoma referenced this issue from andrew/libakstdlib

2026-08-05 15:54:05 -04:00

Gate every push on cppcheck: 15 findings, one of them a reported use-after-free in aksl_fclose #45

tachikoma referenced this issue from andrew/akbasic

2026-08-05 15:54:05 -04:00

Gate every push on cppcheck: 102 findings, including a missing return in src/sink_akgl.c that no CI build compiles #48

tachikoma referenced this issue from andrew/libakgl

2026-08-05 15:54:06 -04:00

Gate every push on cppcheck: 103 findings, and two files cppcheck cannot parse at all #83

andrew added a new dependency 2026-08-05 16:07:48 -04:00

#32 No pre-push hook: every gate in this repository runs only after the push has landed

andrew added a new dependency 2026-08-05 16:09:01 -04:00