Unrelated code has effect on the analysis result of CSA

[Geoffrey1014]

I got a false positive when compiling the following MCVE program:

#include <stdio.h>
int l ;

int *b( int *n) {
  for (; l < 3;)
    return n;
}

int d() {
  int f;
  int *g = &f;
  int *i;
  (i = b(g)) || 1; // removing "|| 1" would change analysis result;
  printf("NPD_FLAG\n");
  *i;
}

void main() { d(); }

Compiling it with options --analyze --analyzer-output text, CSA emits a FP warning as following:

<source>:15:3: warning: Dereference of null pointer (loaded from variable 'i') [core.NullDereference]
  *i;
  ^
<source>:18:15: note: Calling 'd'
void main() { d(); }
              ^~~
<source>:13:4: note: Value assigned to 'i'
  (i = b(g)) || 1; // removing "|| 1" would change analysis result;
   ^~~~~~~~
<source>:13:4: note: Assuming 'i' is null
  (i = b(g)) || 1; //removing "|| 1" would change analysis result;
   ^
<source>:13:4: note: Assuming pointer value is null
  (i = b(g)) || 1; // removing "|| 1" would change analysis result;
   ^~~~~~~~
<source>:13:3: note: Left side of '||' is false
  (i = b(g)) || 1; // removing "|| 1" would change analysis result;
  ^
<source>:15:3: note: Dereference of null pointer (loaded from variable 'i')
  *i;
  ^~
1 warning generated.

I think it is ok for CSA to report the FP warning, but unrelated code || 1 at line 12 can affect the result of analysis: removing this || 1, the FP disappears. This inconsistency may suggest some problem here.

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

[steakhal]

I think you creduced the example a bit too much. Look at the loop within b(). What should b() return if l < 3 false? CSA assumes that it returned 'something', then we also see a check for null part of that operator || for that pointer, by which we assume that i could refer to a null pointer, which you later dereference.

If you remove the || operator call, we no longer introduce the assumption that the pointer can be null, hence the report disappears.

BTW the mentioned example could be transformed into something like this, which would still demonstrate the same phenomenon:

void clang_analyzer_dump(const int *);

int rng;
int *identity(int *n) {
  if (rng)
    return n;
  // !! Missing return statement for the false branch!
  // return n;
}

void top() {
  int var;
  int *p = identity(&var);
  clang_analyzer_dump(p);
  clang_analyzer_dump(&var);
  if (p) {}
  *p;
}

Mind that by uncommenting the return n; in identity() would make the FP go away.

[haoNoQ]

Hmm, the false positive is here because it looks like our machine that tries to reason about state of global variables in main() (they are equal to their initializers unless explicitly overwritten; that's not true for any other analysis entry point) doesn't pick up that l is zero-initialized. This leads us along the path on which b() doesn't have a return, which makes its return value unknown.

The || 1 part affects it for a completely valid reason though. The report explicitly says: "Assuming...". Under that assumption (about the unknown value), the rest of the report is correct. And we have to assume that this is possible because otherwise the || 1 part becomes dead code which is in and of itself a valid reason to warn.

On a related note, I'm seeing an uptick of bug reports about CMVE programs. The static analyzer is operating under a much stronger assumption of "the code actually makes sense", so it'll sometimes emit warnings even when the program is technically correct. Dead code is an example of such assumption: we assume that users want to hear about it even if the program does nothing wrong, as it might be the problem that it doesn't do something it's supposed to. So even though in this case it's clearly a bug in our tool, and I'm very grateful for the bug report! - I also want to communicate that we don't find the requirement of "never warn on CMVE programs" practical. Like, this program also produces 3 compiler warnings (including the one about missing return) and nobody's complaining about that!