Search code examples
c++sonarqubestatic-analysiscppcheckklocwork

static analysis checks fails to find trivial C++ issue


I encountered a surprising False Negative in our C++ Static Analysis tool.

We use Klocwork (Currently 2021.1),
and several colleages reported finding issues KW should have found.

I got example down to as simple as:

int theIndex = 40;

int main()
{
  int arr[10] = {0,1,2,3,4,5,6,7,8,9};
  return arr[theIndex];
}

Any amateur can see I am definitely accessing out of bound array member [40] of the array [0..9].
But KW does not report that clear defect!

TBH, I used CppCheck and SonarQube too, and those failed too!

Testing an more direct flow like:

int main()
{
  int theIndex = 40;
  int arr[10] = {0,1,2,3,4,5,6,7,8,9};
  return arr[theIndex];
}

does find the abundant issue.

My guess was that KW does not see main() as the entrypoint, therefore assume theIndex might be changed before it's called.

I also tired a version that 'might work' (if there is another task that synchronizes perfectly)

int theIndex;

int foo() {
  const int arr[10] = {0,1,2,3,4,5,6,7,8,9};
  return arr[theIndex];
}

int main()
{
  theIndex = 40;
  return foo();
}

Which CppCheck found as "bug free".

My Question is:

  • Am I mis-configuring the tools?
    • what should I do?
  • Should KW catch this issue or is it a limitation of SA tools?
  • Is there a good tool that is capable of catching such issues ?

Edit:

as @RichardCritten assume SA Tools realize other Compilation Units can change the value of theIndex therefore does not indicate the problem.
which holds true as declaring static int theIndex = 40 Does indicate the issue.

Now I wonder:
KW is fed with the full build-spec,
so theoretically, the tool could trace all branching of the software and track possible values of theIndex (might be a computational limitation).

  • Is there a way to instruct the tool to do so?
    • somewhat as a 'link' stage?

Solution

  • My guess was that KW does not see main() as the entrypoint, therefore assume theIndex might be changed before it's called.

    theIndex can in fact be changed before main is entered. Every initializer of a global variable anywhere in the program can execute arbitrary code and access all global variables. So the tool would potential produce a lot of false positives if it assumed that all initial values of global variables remain unchanged until main is entered.

    Of course this doesn't mean that the tool couldn't decide to warn anyway, risking false positives. I don't know whether the mentioned tools are configurable to do so.

    If this is intended to be a constant mark it as constexpr. I then expect tools to recognize the issue.

    If it is not supposed to be a constant, try to get rid of it. Global variables that aren't constants cause many issues. Because they are potentially modified by any call to a function whose body isn't known (and before entry to main or a thread), they are difficult to keep track of for humans, static analyzers and optimizers alike.

    Giving the variable internal linkage may simplify the analysis, because the tool may be able to prove that nothing in the given translation unit could be accessed from another translation unit to set the value of the variable. If there was anything like that, then a global initializer in another unit may still modify it before main is entered. If that is not the case and there is also no global initializer in the variable's translation unit that modifies it, then the tool can be sure that the value remains unchanged before main.

    With external linkage that doesn't work, because any translation unit can gain access to the variable simply by declaring it.

    Technically I suppose a sufficiently sophisticated tool could do whole-program analysis to verify whether or not the global variable is modified before main. However, this is already problematic in theory if dynamic libraries are involved and I don't think that is a typical approach taken by static analyzers. (I could be wrong on this.)