andrew/libakstdlib
1
0
Fork 0
Code Issues 27 Pull Requests Actions Packages Projects Releases Wiki Activity

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

New Issue
Open
opened 2026-08-05 15:53:49 -04:00 by tachikoma · 1 comment
tachikoma commented 2026-08-05 15:53:49 -04:00
Collaborator
Copy Link

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 libakstdlib half.

Do libakerror first. It is six findings in one file and it is where the shape of
scripts/cppcheck.sh and the static_analysis job gets settled; this repository copies it.
libakstdlib is second because it is the only one of the four that already has a pre-push
hook, so it is the first place the hook half of the plan can actually be finished.

Measured, before any change

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

severity count
error 1
warning 0
style 14

By file: src/stdlib.c 7, src/collections.c 6, src/string.c 2.

The one error is the reason this is worth doing at all:

src/stdlib.c:399:35: error: Dereferencing 'stream' after it is deallocated / released [deallocuse]

Read that one before anything else. It points at stream inside
FAIL_NONZERO_RETURN(e, fclose(stream), ...) in aksl_fclose -- cppcheck's caret sits on
the argument of the fclose that does the deallocating. The obvious way for that to be a
real defect is a macro that evaluates its argument twice, and it does not:
FAIL_NONZERO_RETURN expands to if ( __x != 0 ) { FAIL(...); return ...; }, one
evaluation. So this is most likely cppcheck mis-modelling a deallocating call used as a
condition, and the likely outcome is an inline suppression naming that.

It is still the first thing to check rather than the first thing to dismiss. A
use-after-free that ASan, valgrind and a 99.5%-line coverage suite have all run past would
be exactly the class of defect a static checker exists to catch. If it turns out real, it
gets its own defect issue rather than being fixed quietly inside this one.

The rest are style: unreadVariable 5, constVariablePointer 5, variableScope 2,
constParameterPointer 1, and one knownConditionTrueFalse:

src/stdlib.c:1298:15: style: Condition 'drained!=NULL' is always true [knownConditionTrueFalse]

Expect that last one to be the error macros again -- cppcheck does not follow
ATTEMPT/FINISH any better than gcov does, which is the same reason this repository's CI
comments already give for its 40% branch gate.

Scope for this repository

SCOPE=( src include )

No examples/, tools/ or util/ in this tree. tests/ (21 files) is excluded per the
plan below, and deps/libakerror is excluded by the scope: measured, cppcheck . -i tests
here reports 30 findings and 24 of them are libakerror's, gated in libakerror's own
pipeline by the companion issue.

Work

  • Read src/stdlib.c:399. If the use-after-free is real, file it as a defect in its
    own right and link it here; if it is not, inline-suppress it with the reason.
  • Fix or inline-suppress the remaining 14. 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.
  • Add it to .githooks/pre-push as the first step, before the default build. It is
    2--6 seconds against that hook's existing tens of seconds, and it should be skipped
    with a warning when cppcheck is absent -- the same way that hook already treats
    doxygen, and for the reason written there.
  • 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:

  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.

#!/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

  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.

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 libakstdlib half. Do libakerror first. It is six findings in one file and it is where the shape of `scripts/cppcheck.sh` and the `static_analysis` job gets settled; this repository copies it. libakstdlib is second because it is the only one of the four that already has a pre-push hook, so it is the first place the hook half of the plan can actually be finished. ## Measured, before any change cppcheck 2.19.0, `--std=c99 --platform=unix64 --enable=style`, over `src include`: | severity | count | | --- | --- | | error | 1 | | warning | 0 | | style | 14 | By file: `src/stdlib.c` 7, `src/collections.c` 6, `src/string.c` 2. The one error is the reason this is worth doing at all: ``` src/stdlib.c:399:35: error: Dereferencing 'stream' after it is deallocated / released [deallocuse] ``` Read that one before anything else. It points at `stream` inside `FAIL_NONZERO_RETURN(e, fclose(stream), ...)` in `aksl_fclose` -- cppcheck's caret sits on the argument of the `fclose` that does the deallocating. The obvious way for that to be a real defect is a macro that evaluates its argument twice, and it does not: `FAIL_NONZERO_RETURN` expands to `if ( __x != 0 ) { FAIL(...); return ...; }`, one evaluation. So this is most likely cppcheck mis-modelling a deallocating call used as a condition, and the likely outcome is an inline suppression naming that. It is still the first thing to check rather than the first thing to dismiss. A use-after-free that ASan, valgrind and a 99.5%-line coverage suite have all run past would be exactly the class of defect a static checker exists to catch. If it turns out real, it gets its own defect issue rather than being fixed quietly inside this one. The rest are style: `unreadVariable` 5, `constVariablePointer` 5, `variableScope` 2, `constParameterPointer` 1, and one `knownConditionTrueFalse`: ``` src/stdlib.c:1298:15: style: Condition 'drained!=NULL' is always true [knownConditionTrueFalse] ``` Expect that last one to be the error macros again -- cppcheck does not follow `ATTEMPT`/`FINISH` any better than gcov does, which is the same reason this repository's CI comments already give for its 40% branch gate. ## Scope for this repository ``` SCOPE=( src include ) ``` No `examples/`, `tools/` or `util/` in this tree. `tests/` (21 files) is excluded per the plan below, and `deps/libakerror` is excluded by the scope: measured, `cppcheck . -i tests` here reports 30 findings and 24 of them are libakerror's, gated in libakerror's own pipeline by the companion issue. ## Work - [ ] Read `src/stdlib.c:399`. If the use-after-free is real, file it as a defect in its own right and link it here; if it is not, inline-suppress it with the reason. - [ ] Fix or inline-suppress the remaining 14. 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. - [ ] Add it to `.githooks/pre-push` as the first step, before the default build. It is 2--6 seconds against that hook's existing tens of seconds, and it should be skipped with a warning when cppcheck is absent -- the same way that hook already treats doxygen, and for the reason written there. - [ ] 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 hygieneblast-radius:lowstatus::ready labels 2026-08-05 15:53:49 -04:00
tachikoma commented 2026-08-05 15:54:05 -04:00
Author
Collaborator
Copy Link

Siblings, same plan:

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.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
abi-break
Cannot land without a soname bump
 api-gap
A caller cannot express something
 blast-radius:high
Ordering: highest blast radius
 blast-radius:low
Ordering: lowest blast radius
 blast-radius:medium
Ordering: middling blast radius
 defect
Wrong behaviour reachable from the public API
 design-decision
Needs a decision written down before code
 docs
Documentation, comments, AGENTS.md
 hygiene
Style, dead code, consistency
 packaging
Build, install, pkg-config, CMake config
 performance
Has a measurement behind it
 status::blocked
Waiting on something else -- a decision, a dependency, or another issue
 status::grooming
Not yet worked out: scope, approach or priority still being settled
 status::ready
Understood well enough to start: scope and approach are settled
 status::review
A pull request has been generated and this ticket is waiting on review of that PR
 test-coverage
A gap in the suites, not a defect
No Label blast-radius:low hygiene status::ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
logikoma tachikoma andrew
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Blocks
#48 Gate every push on cppcheck: 102 findings, including a missing return in src/sink_akgl.c that no CI build compiles
andrew/akbasic
Depends on
#33 Gate every push on cppcheck: 6 findings, all in src/error.c
andrew/libakerror
Reference: andrew/libakstdlib#45
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?