Working through the known cppcheck defects, starting with the suppressions file

[Pranav-error]

I have started working through the static analysis findings and wanted to flag what I
am doing before going further, so it does not overlap with anyone else.

I opened #7835 today. It fixes a null pointer dereference in Vect_cat_list_to_array():
a cat_list with no ranges leaves the array unallocated, and the function then reads its
first element. It is reachable through the public API, since Vect_new_cat_list() returns
a list with no ranges and Vect_copy_table_by_cat_list() only checks that the list is not
NULL, not that it has any. Run the way CI runs pytest, the added test segfaults without
the guard and passes with it.

@wenzeslaus, in #7002 you wrote that when fixing these it is worth "adding tests and/or
focus on places with tests", since the risk is introducing an algorithm bug while fixing
what is often a readability issue. That is what I tried to do here: the change is six
lines, and the test covers both the empty list and the sorting and deduplication that
must not change.

I found it in .cppcheck-suppressions under the "True positives" heading. Going through
that section, the other three C entries no longer reproduce with cppcheck 2.21:

• imagery/i.gensigset/subcluster.c:369 — now guarded by if (subsum > 0)
• imagery/i.smap/model.c:158
• va_end_missing in lib/gis/debug.c:82 — va_start and va_end are paired

So those look fixed upstream with the suppressions left behind. Two questions:

1. Would a PR removing the stale entries be welcome, or would you rather that file stay
   as it is until each one is re-checked?
2. Is anyone working on the remaining findings? [Feat] Use of cppcheck as a GitHub action? #1313 was closed in July, with @echoix
   noting that cppcheck never got ported into the checks. I have a baseline-gated cppcheck
   job working on another C project — it counts findings per id:file and fails only on
   new ones, so an existing backlog does not block the build — and I would be glad to put
   one together here if that is wanted. From that thread I understand leaks in modules are
   not treated as bugs, and that the report has to stay quiet to be useful.

Happy to keep going either way. Longer term I am interested in the "Fix Known Code
Defects" idea and would like to work toward GSoC 2027.

[Pranav-error]

Following up my own question 1 with the actual measurements, since guessing at this is not much use to anyone.

I re-ran cppcheck 2.21.0 over every entry in the # True positives section of .cppcheck-suppressions against current main (8f39c47). None of the six is doing what it says.

Four no longer reproduce at all:

invalidFunctionArg:imagery/i.gensigset/subcluster.c:369
invalidFunctionArg:imagery/i.smap/model.c:158
memleakOnRealloc:lib/external/shapelib/dbfopen.c:448
va_end_missing:lib/gis/debug.c:82

subcluster.c is now guarded by if (subsum > 0), and debug.c pairs va_start/va_end correctly, so those two were fixed upstream at some point and the suppression outlived the defect.

The other two are worse than stale — they are silently inactive:

nullPointer:lib/vector/Vlib/cats.c:513
nullPointer:lib/vector/Vlib/cats.c:517

The defect is still there, but the line moved to 515, and suppressions match on line number. Running with --suppressions-list=.cppcheck-suppressions still reports nullPointer:lib/vector/Vlib/cats.c:515. So the file reads as though that finding is acknowledged and handled, and it is not. (#7835 fixes the defect and drops those two lines.)

That is the argument for the baseline-gated job in question 2, more than any new finding would be: a suppressions file nobody re-runs drifts out of alignment with the code and stops being true, quietly. A baseline keyed on id:file counts rather than line numbers does not drift the same way.

Happy to send the removal of the four dead entries as a small PR, separate from #7835 so they can be reviewed independently — but I would rather have a nod first, given #1313 was closed only in July.

One thing I have not checked: cppcheck also reports invalidFunctionArg:lib/vector/Vlib/cats.c:616, which is not in the file at all. I have not looked at whether it is real yet.

[nilason]

I personally do not trust .cppcheck-suppressions, it isn’t maintained and not really reviewed in detail. So if you want to work on it, please have that in mind.

[echoix]

I read this as: do whatever you want to help you while working with cppcheck; you're probably the only one now.

I was open to just merge your suggestion to clear some invalid/resolved exclusions, but personally, it really doesn't merit a PR for it. If you have some useful, reviewable, justified fixes to make, it probably would be a better use of your time. Remember to add your (human) value to the work.

[Pranav-error]

That's clear enough, thanks both — I'll drop the suppressions cleanup. No PR for it.

@nilason, that's useful to know up front. I'll treat the file as a hint about where to look rather than as a list of agreed defects, and check each finding against the code myself.

@echoix, fair point, and it matches what I'd rather be doing anyway. The PRs I have open are all of that kind: #7835 fixes a null dereference reachable through the public API, #7837 a null function pointer crash in v.to.rast -d, #7840 unbounded PATH growth on repeated gs.setup.init(), #7844 out of range categories accepted on ASCII import — each with a test that fails without the fix. #7847 is still a draft waiting on @wenzeslaus for the API contract question on #7735. I'll keep to that and leave the CI job alone unless someone asks for it.