[Feat] Use of cppcheck as a GitHub action?

[neteler]

Is your feature request related to a problem? Please describe.

By chance I came across cppcheck (cited from their manual, http://cppcheck.sourceforge.net/manual.pdf):

Cppcheck is a static analysis tool for C/C++ code. It provides unique code analysis to detect bugs and focuses on detecting undefined behaviour and dangerous coding constructs. The goal is to have very few false positives. Cppcheck is designed to be able to analyze your C/C++ code even if it has non-standard syntax (common in embedded projects).

It has been used years ago by Debian: https://qa.debian.org/daca/cppcheck/squeeze/grass_6.4.0~rc6+42329-3.html

So I gave it locally a try:

# run on entire GRASS-core tree
cppcheck --std=c++17 --enable=warning,style,performance,portability --inconclusive -j 4 --error-exitcode=1 -q  -Iinclude/ .

which would deliver plenty of warnings and errors.

An only very limited list is returned with:

# as an example, run on vector subdir:
cppcheck  --enable=performance,portability --inconclusive -j 4 --error-exitcode=1 -q  -Iinclude/ vector
vector/v.in.dwg/main.c:141:43: error: Undefined behavior: Variable 'buf' is used as parameter and destination in sprintf(). [sprintfOverlappingData]
     sprintf(buf, _("%s Cannot open %s"), buf, path);
                                          ^
vector/v.lidar.growing/ConvexHull.c:244:5: error: Memory leak: v [memleak]
    return v - nl + NR_END;
    ^
vector/v.lidar.growing/ConvexHull.c:307:10: portability:inconclusive: Casting from double * * to signed char * is not portable due to different binary data representations on different platforms. [invalidPointerCast]
    free((FREE_ARG) (v + nl - NR_END));
         ^
vector/v.net.visibility/data_structures.c:101:22: style: Array index 'i' is used before limits check. [arrayIndexThenCheck]
 while (cmp_points(&a[i], &pivot, NULL) < 1 && i <= r);
                     ^
vector/v.overlay/main.c:162:31: style: Array index 'i' is used before limits check. [arrayIndexThenCheck]
    while (ofield_opt->answers[i] && i < 3) {
                              ^
vector/v.split/main.c:257:24: error: Uninitialized variable: x [uninitvar]
       Points2->x[0] = x;
                       ^
vector/v.split/main.c:258:24: error: Uninitialized variable: y [uninitvar]
       Points2->y[0] = y;
                       ^
vector/v.split/main.c:259:24: error: Uninitialized variable: z [uninitvar]
       Points2->z[0] = z;
                       ^
vector/v.split/main.c:257:24: error: Uninitialized variable: x [uninitvar]
       Points2->x[0] = x;
                       ^
vector/v.split/main.c:249:12: note: Assuming condition is false
   if (ret == 0) {
           ^
vector/v.split/main.c:257:24: note: Uninitialized variable: x
       Points2->x[0] = x;
                       ^
vector/v.split/main.c:258:24: error: Uninitialized variable: y [uninitvar]
       Points2->y[0] = y;
                       ^
vector/v.split/main.c:249:12: note: Assuming condition is false
   if (ret == 0) {
           ^
vector/v.split/main.c:258:24: note: Uninitialized variable: y
       Points2->y[0] = y;
                       ^
vector/v.split/main.c:259:24: error: Uninitialized variable: z [uninitvar]
       Points2->z[0] = z;
                       ^
vector/v.split/main.c:249:12: note: Assuming condition is false
   if (ret == 0) {
           ^
vector/v.split/main.c:259:24: note: Uninitialized variable: z
       Points2->z[0] = z;
                       ^
vector/v.surf.idw/read_sites.c:106:10: error: Uninitialized variable: ret [uninitvar]
     if (ret != DB_OK) {
         ^
vector/v.surf.idw/read_sites.c:106:10: error: Uninitialized variable: ret [uninitvar]
     if (ret != DB_OK) {
         ^
vector/v.surf.idw/read_sites.c:90:14: note: Assuming condition is false
     if (cat < 0) /* skip features without category */
             ^
vector/v.surf.idw/read_sites.c:93:17: note: Assuming condition is false
            if (col) {
                ^
vector/v.surf.idw/read_sites.c:106:10: note: Uninitialized variable: ret
     if (ret != DB_OK) {
         ^
vector/v.to.rast3/main.c:144:6: error: Uninitialized variable: ret [uninitvar]
 if (ret != DB_OK) {
     ^

I cannot judge if that's correct/relevant but wanted to bring this tool to our attention.

[neteler]

An alternative/further tool might be

https://github.com/google/sanitizers/wiki/AddressSanitizer

[nilason]

So I gave it locally a try:

# run on entire GRASS-core tree
cppcheck --std=c++17 --enable=warning,style,performance,portability --inconclusive -j 4 --error-exitcode=1 -q  -Iinclude/ .

which would deliver plenty of warnings and errors.

Testing this against --std=c++11, would that make any difference?

[neteler]

Testing this against --std=c++11, would that make any difference?

I tried, it looks similar.

[neteler]

It might well be that there are false alarms when using --enable=warning,style,performance,portability:

e.g. (random example):

imagery/i.cca/main.c:129:12: warning:inconclusive: Either the condition '(sigfp=I_fopen_signature_file_old(grp_opt->answer,subgrp_opt->answer,sig_opt->answer))==NULL' is redundant or there is possible null pointer dereference: sigfp. [nullPointerRedundantCheck]
    fclose(sigfp);

Here the imagery/ folder with simplified settings:

cppcheck  --enable=performance,portability --inconclusive -j 4 --error-exitcode=1 -q  imagery/
imagery/i.albedo/main.c:333:22: error: Array 'histogram[100]' accessed at index 100, which is out of bounds. [arrayIndexOutOfBounds]
        if (histogram[i] < bottom3b) {
                     ^
imagery/i.atcorr/6s.cpp:374:2: portability: Using memset() on struct which contains a floating point number. [memsetClassFloat]
 memset(&is, 0, sizeof(is));
 ^
imagery/i.atcorr/output.h:54:43: performance: Function parameter 's' should be passed by const reference. [passedByValue]
 static void WriteLn(int cnt, std::string s)
                                          ^
imagery/i.atcorr/transform.h:45:46: performance: Function parameter 'ti' should be passed by const reference. [passedByValue]
extern double transform(const TransformInput ti, InputMask imask, double idn);
                                             ^
imagery/i.atcorr/aerosolmodel.cpp:276:13: error: Uninitialized variable: mub [uninitvar]
    for(k = mub; k >= 1; k--) xj[k-1] = (2 * k + 1) * xj[k] / X - xj[k+1];
            ^
imagery/i.atcorr/main.cpp:240:29: performance: Function parameter 'ti' should be passed by const reference. [passedByValue]
    void add(TransformInput ti, double alt, double vis)
                            ^
imagery/i.atcorr/computations.cpp:1600:5: portability: Using memset() on struct which contains a floating point number. [memsetClassFloat]
    memset(&oap, 0, sizeof(oap));
    ^
imagery/i.atcorr/computations.cpp:1606:5: portability: Using memset() on struct which contains a floating point number. [memsetClassFloat]
    memset(&sixs_trunc, 0, sizeof(sixs_trunc));  /* clear this to keep preconditions the same and output consistent */
    ^
imagery/i.atcorr/transform.cpp:135:39: performance: Function parameter 'ti' should be passed by const reference. [passedByValue]
double transform(const TransformInput ti, InputMask imask, double idn)
                                      ^
imagery/i.eb.soilheatflux/g0.c:19:28: error: Uninitialized variable: r0_coef [uninitvar]
    a = (0.0032 * (bbalb / r0_coef) +
                           ^
imagery/i.gensig/alloc.c:29:12: portability:inconclusive: Casting from double * to signed char * is not portable due to different binary data representations on different platforms. [invalidPointerCast]
    G_free((char *)(v + nl));
           ^
imagery/i.gensig/alloc.c:39:9: portability:inconclusive: Casting from double * to signed char * is not portable due to different binary data representations on different platforms. [invalidPointerCast]
 G_free((char *)(m[i] + ncl));
        ^
imagery/i.gensig/alloc.c:40:12: portability:inconclusive: Casting from double * * to signed char * is not portable due to different binary data representations on different platforms. [invalidPointerCast]
    G_free((char *)(m + nrl));
           ^
imagery/i.gensig/means.c:35:7: error: Uninitialized variable: n_nulls [uninitvar]
      n_nulls++;
      ^
imagery/i.segment/mean_shift.c:453:17: error: Uninitialized struct member: ngbr_rc.next [uninitStructMember]
    *pngbr_rc = ngbr_rc;
                ^
imagery/i.segment/mean_shift.c:564:17: error: Uninitialized struct member: ngbr_rc.next [uninitStructMember]
    *pngbr_rc = ngbr_rc;
                ^
imagery/i.segment/region_growing.c:719:17: error: Uninitialized struct member: ngbr_rc.next [uninitStructMember]
    *pngbr_rc = ngbr_rc;
                ^
imagery/i.segment/region_growing.c:1061:14: error: Uninitialized struct member: ngbr_rc.next [uninitStructMember]
 *pngbr_rc = ngbr_rc;
             ^
imagery/i.segment/region_growing.c:1528:14: error: Uninitialized struct member: ngbr_rc.next [uninitStructMember]
 *pngbr_rc = ngbr_rc;
             ^
imagery/i.smap/interp.c:141:15: portability:inconclusive: Casting from double * * * to signed char * is not portable due to different binary data representations on different platforms. [invalidPointerCast]
    multifree((char *)N, 3);
              ^
imagery/i.smap/interp.c:285:12: portability:inconclusive: Casting from double * to signed char * is not portable due to different binary data representations on different platforms. [invalidPointerCast]
    G_free((char *)pdf);
           ^
imagery/i.smap/model.c:167:12: portability:inconclusive: Casting from double * to signed char * is not portable due to different binary data representations on different platforms. [invalidPointerCast]
    G_free((char *)diff);
           ^
imagery/i.smap/model.c:168:12: portability:inconclusive: Casting from double * to signed char * is not portable due to different binary data representations on different platforms. [invalidPointerCast]
    G_free((char *)subll);
           ^
imagery/i.segment/region_growing.c:885:10: error: Uninitialized variable: pl1 [uninitvar]
    pl = pl1 + pl2 - nshared;
         ^
imagery/i.segment/region_growing.c:885:16: error: Uninitialized variable: pl2 [uninitvar]
    pl = pl1 + pl2 - nshared;
               ^
imagery/i.segment/region_growing.c:903:13: error: Uninitialized variable: count1 [uninitvar]
    count = count1 + count2;
            ^
imagery/i.segment/region_growing.c:903:22: error: Uninitialized variable: count2 [uninitvar]
    count = count1 + count2;

[nilason]

Looking into a couple of random issues from the list, they seem to be real issues.

[nilason]

e.g. imagery/i.segment/region_growing.c:885:

    ...
    int pl1, pl2, count1, count2;
    int e1, n1, s1, w1, e2, n2, s2, w2, ns_extent, ew_extent;

    pl = pl1 + pl2 - nshared;
    ...

[marisn]

Without looking into any specific issue raised – one has to carefully examine each case as 1) memory leaks in modules so far are not considered to be bugs (after run OS reclaims whole memory); 2) linters are bad at "might be used uninitialized" (and similar errors) due to frequent calls to G_fatal_error in libraries etc. thus guarding against bad scenarios. Keeping those two points in mind, we should try to reduce number of issues raised to bare minimum.

[wenzeslaus]

Use of cppcheck as a GitHub action?

I tested cppcheck couple times in the past, but there was just too many things to make it useful. With the compiler warning fixes and couple years of cppcheck presumably reducing false positives, I think it becomes interesting again.

... 2) linters are bad at "might be used uninitialized" (and similar errors) due to frequent calls to G_fatal_error in libraries etc. thus guarding against bad scenarios.

If that's the case, it might be possible to provide some help to the linter by flagging G_fatal_error appropriately. G_fatal_error already has __attribute__ ((noreturn)).

Then there is of course the rule "not understandable by a machine is unnecessary difficult to understand by a human."

...we should try to reduce number of issues raised to bare minimum.

:+1: