ci: fix the cppcheck gate, and run cppcheck and clang-tidy on PRs at all - #403
ci: fix the cppcheck gate, and run cppcheck and clang-tidy on PRs at all#403xanimo wants to merge 2 commits into
Conversation
The cppcheck job fails on 0.1.5-dev itself. Reproduced on an untouched
checkout, so it is red for every open pull request and none of them
caused it.
The header of the workflow states the phase-0 policy plainly:
Gating policy (phase 0): only `error` severity fails the job. The
full report (all severities, CWE IDs included) is uploaded as an
artifact for triage. Gating widens to `warning` once the initial
backlog is dispositioned in cppcheck-suppressions.txt.
but the step named "gate on error severity" invoked cppcheck with
--enable=warning --error-exitcode=1, so warnings were fatal too. On the
CI image (ubuntu-24.04, cppcheck 2.13) that is 26 warnings against 2
errors.
Drops --enable=warning from the gating step only. The reporting step
above still runs --enable=warning,style,performance,portability and
uploads the full XML, and the CWE summary step is untouched, so nothing
stops being visible -- it stops blocking.
That leaves the two error-severity findings, both false positives, both
in OP-TEE code and both now suppressed with a rationale as the
suppressions file requires:
- src/optee/host/main.c: `seed` is passed as a TEEC_MEMREF_TEMP_OUTPUT
buffer that the trusted application fills. cppcheck reads taking its
address as a use of an uninitialised value.
- src/optee/ta/libdogecoin_ta.c: cppcheck cannot build an AST for
assert() as expanded there.
Deliberately not suppressed: three further internalAstError hits on
HASH_DEL that cppcheck 2.7 reports locally and 2.13 on the CI image does
not. Suppressing findings the gate does not produce would be writing the
file against a stale tool version. Worth noting 2.13 already cleared
three of the four internalAstError that 2.7 reports, so a version bump
beats a suppression here; pinning the cppcheck version so local and CI
agree is a sensible follow-up, since `apt-get install cppcheck` tracks
whatever the runner image ships.
The backlog this leaves for phase 1, from the CI run: 18
invalidPrintfArgType_sint (%d with unsigned -- real, mechanical, should
be fixed rather than suppressed), 4 nullPointerRedundantCheck and 2
ctunullpointer in wallet.c/utils.c which need actual review since
wallet.c dereferences tx before the !tx check, and 1
literalWithCharPtrCompare in bip32.c.
The gate has already paid for itself: it is what surfaced the unbounded
fscanf fixed in dogecoinfoundation#401, which had been shipping since v0.1.2.
Both jobs filter on '*-dev-*', for push and for pull_request. That pattern never matches `0.1.5-dev` itself, because there is nothing after "-dev". The consequence is that neither has ever run on a pull request upstream: the only runs are pushes to fork branches that happen to carry a suffix, so the failures land in one author's inbox and gate nothing. This is the same defect dogecoinfoundation#377 fixes in ql.yml, and the same one dogecoinfoundation#371 already fixed in ci.yml -- `*` does not cross `/`, and `*-dev-*` requires a trailing segment that the release branch does not have. All three phase-0 static-analysis workflows shipped with it. Aligns both with ci.yml: push on '*-dev*' (which matches the bare release branch as well as suffixed work branches) and pull_request on '*', because a pull request must be analysed whichever branch it targets. Without this, narrowing the severity gate in the previous commit would make the job green somewhere nobody is looking.
|
Added a second commit — the gate fix alone would have made this job green somewhere nobody looks. Neither cppcheck nor clang-tidy has ever run on a pull request upstream. Both filter Same defect #377 fixes in Both are now aligned with
Worth flagging for review order: this makes both jobs start running on every PR for the first time. clang-tidy is advisory (it uploads a report and does not gate), but cppcheck does gate, so the severity fix in the first commit needs to land with it rather than after. |
The cppcheck job fails on
0.1.5-devitself. I reproduced it on an untouched checkout, so it is red for every open PR and none of them caused it. Right now it is generating a failure notification per push across ~29 open branches.The contradiction
The header of
.github/workflows/cppcheck.ymlstates the policy plainly:But the step named
gate on error severityinvoked:so warnings were fatal too. On the CI image (
ubuntu-24.04, cppcheck 2.13) that's 26 warnings against 2 errors.The change
Drops
--enable=warningfrom the gating step only. The reporting step above still runs--enable=warning,style,performance,portability, still writescppcheck-report.xml, and the CWE summary step is untouched — nothing stops being visible, it stops blocking.That leaves the two error-severity findings. I checked both; both are false positives, both in OP-TEE code, and both are now suppressed with a rationale, as the suppressions file requires:
uninitvar—src/optee/host/main.c:85seedis passed as aTEEC_MEMREF_TEMP_OUTPUTbuffer that the trusted application fills. cppcheck reads taking its address as a use of an uninitialised value.internalAstError—src/optee/ta/libdogecoin_ta.c:65assert()as expanded there. Parser limitation, not a code defect.What I deliberately did not suppress
cppcheck 2.7 locally reports three further
internalAstErrorhits onHASH_DEL(eckey.c,smpv.c,transaction.c) that 2.13 on the CI image does not. Suppressing findings the gate doesn't produce would be writing the file against a stale tool version.Worth noting 2.13 already cleared three of the four
internalAstErrorthat 2.7 reports — a version bump beats a suppression here. Pinning the cppcheck version so local and CI agree is a sensible follow-up, sinceapt-get install cppchecktracks whatever the runner image happens to ship.The phase-1 backlog this leaves
From the CI run, for whoever dispositions it before widening the gate:
invalidPrintfArgType_sint—%dwith unsigned. Real, mechanical, should be fixed rather than suppressed.nullPointerRedundantCheck+ 2ctunullpointerinwallet.c/utils.c— these need actual review, not a blanket disposition.wallet.c:1725dereferencestxbefore the!txcheck at 1742.literalWithCharPtrCompareinbip32.c— almost certainly a false positive on thestrlenshelper.The principle I'd suggest holding: suppressions are for false positives with a written reason, never for real findings. Using the file to hide the 18 printf ones is how these gates rot.
The gate has already earned its keep: it is what surfaced the unbounded
fscanffixed in #401, which had been shipping since v0.1.2.