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

New Issue

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

tachikoma commented

2026-08-05 15:53:55 -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 akbasic half.

Do libakerror and libakstdlib first. They are 6 and 15 findings and they are where the
shape of scripts/cppcheck.sh and the static_analysis job gets settled; this repository
copies it. akbasic is 102 findings and is the larger cleanup of the four.

Measured, before any change

cppcheck 2.19.0, --std=c99 --platform=unix64 --enable=style.

Over src include:

severity	count
error	1
warning	2
portability	4
style	81

Over examples tools: 2 warning, 12 style.

102 findings in total. The style bulk is mechanical -- constParameterPointer 26,
unreadVariable 19, constVariablePointer 15, variableScope 13 -- and spread thinly:
src/host.c 12, src/value.c 8, src/runtime.c 8, then a long tail of 3--7 per file.

The nine that are not style are where the value is:

src/sink_akgl.c:66:5:  error:   Found an exit path from function with non-void return type
                                that has missing return statement [missingReturn]
src/host.c:64:45:      warning: Either the condition 'hostbase!=NULL' is redundant or there
                                is pointer arithmetic with NULL pointer [nullPointerArithmeticRedundantCheck]
src/host.c:121:33:     warning: (as above)
src/host.c:84:62:      portability: Casting between const char * and const float * which have
                                    an incompatible binary data representation [invalidPointerCast]
src/host.c:85:54:      portability: (const char * / const double *)
src/host.c:162:3:      portability: (char * / float *)
src/host.c:165:3:      portability: (char * / double *)
tools/screenshot.c:112:48: warning: If resource allocation fails, then there is a possible
                                    null pointer dereference [nullPointerOutOfResources]
tools/screenshot.c:113:12: warning: (as above)

Three of these deserve reading rather than patching:

src/sink_akgl.c:66 is a control path out of a non-void function with no return. That
file is only compiled with AKBASIC_WITH_AKGL=ON, so the default CI build has never
compiled it -- and cppcheck reads it regardless of the CMake option, which is one of the
concrete things this gate buys.
The four invalidPointerCast in src/host.c are the embedding boundary punning a byte
buffer as float * / double *. That may well be deliberate and documented, in which
case it is an inline suppression with the alignment argument written next to it -- but
"deliberate" needs to be established, not assumed, because it is genuine UB when the
buffer is not suitably aligned.
The two nullPointerArithmeticRedundantCheck at src/host.c:64 and :121 say the
hostbase != NULL guard and the pointer arithmetic disagree about whether NULL is
possible. One of the two is wrong.

Scope for this repository

SCOPE=( src include examples tools )

deps/ is excluded by the scope and that is load-bearing here: this tree vendors SDL,
SDL_image, SDL_mixer, SDL_ttf, jansson, libccd, clay, libakerror and libakstdlib. A bare
cppcheck . would check all of them -- thousands of findings in code this repository
cannot fix, and minutes of runtime. tests/ (46 files) is excluded per the plan below.

Work

Read and resolve the nine non-style findings first. Any that turn out to be real
defects get their own issue and a link back here, rather than being fixed quietly
inside this one.
Fix or inline-suppress the remaining 93. 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 -- in ci.yaml rather
than release.yaml: the check is 6 seconds measured, which is a per-commit gate, not
a release gate.
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 #47, 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 akbasic half. Do libakerror and libakstdlib first. They are 6 and 15 findings and they are where the shape of `scripts/cppcheck.sh` and the `static_analysis` job gets settled; this repository copies it. akbasic is 102 findings and is the larger cleanup of the four. ## Measured, before any change cppcheck 2.19.0, `--std=c99 --platform=unix64 --enable=style`. Over `src include`: | severity | count | | --- | --- | | error | 1 | | warning | 2 | | portability | 4 | | style | 81 | Over `examples tools`: 2 warning, 12 style. **102 findings in total.** The style bulk is mechanical -- `constParameterPointer` 26, `unreadVariable` 19, `constVariablePointer` 15, `variableScope` 13 -- and spread thinly: `src/host.c` 12, `src/value.c` 8, `src/runtime.c` 8, then a long tail of 3--7 per file. The nine that are not style are where the value is: ``` src/sink_akgl.c:66:5: error: Found an exit path from function with non-void return type that has missing return statement [missingReturn] src/host.c:64:45: warning: Either the condition 'hostbase!=NULL' is redundant or there is pointer arithmetic with NULL pointer [nullPointerArithmeticRedundantCheck] src/host.c:121:33: warning: (as above) src/host.c:84:62: portability: Casting between const char * and const float * which have an incompatible binary data representation [invalidPointerCast] src/host.c:85:54: portability: (const char * / const double *) src/host.c:162:3: portability: (char * / float *) src/host.c:165:3: portability: (char * / double *) tools/screenshot.c:112:48: warning: If resource allocation fails, then there is a possible null pointer dereference [nullPointerOutOfResources] tools/screenshot.c:113:12: warning: (as above) ``` Three of these deserve reading rather than patching: - `src/sink_akgl.c:66` is a control path out of a non-void function with no return. That file is only compiled with `AKBASIC_WITH_AKGL=ON`, so the default CI build has never compiled it -- and cppcheck reads it regardless of the CMake option, which is one of the concrete things this gate buys. - The four `invalidPointerCast` in `src/host.c` are the embedding boundary punning a byte buffer as `float *` / `double *`. That may well be deliberate and documented, in which case it is an inline suppression with the alignment argument written next to it -- but "deliberate" needs to be established, not assumed, because it is genuine UB when the buffer is not suitably aligned. - The two `nullPointerArithmeticRedundantCheck` at `src/host.c:64` and `:121` say the `hostbase != NULL` guard and the pointer arithmetic disagree about whether NULL is possible. One of the two is wrong. ## Scope for this repository ``` SCOPE=( src include examples tools ) ``` `deps/` is excluded by the scope and that is load-bearing here: this tree vendors SDL, SDL_image, SDL_mixer, SDL_ttf, jansson, libccd, clay, libakerror and libakstdlib. A bare `cppcheck .` would check all of them -- thousands of findings in code this repository cannot fix, and minutes of runtime. `tests/` (46 files) is excluded per the plan below. ## Work - [ ] Read and resolve the nine non-style findings first. Any that turn out to be real defects get their own issue and a link back here, rather than being fixed quietly inside this one. - [ ] Fix or inline-suppress the remaining 93. 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` -- in `ci.yaml` rather than `release.yaml`: the check is 6 seconds measured, which is a per-commit gate, not a release gate. - [ ] 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 #47, 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:medium status::ready labels 2026-08-05 15:53:55 -04:00

tachikoma referenced this issue from andrew/libakerror

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

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

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 commented

2026-08-05 15:54:05 -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/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:09:46 -04:00

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

andrew added a new dependency 2026-08-05 16:10:12 -04:00

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

Sign in to join this conversation.