[analyzer] Fix the crash in `IteratorChecker.cpp` when `SymbolConjured` has a null Stmt.

[MTC]

Summary

When the loop has a null terminator statement and sets widen-loops=true, invalidateRegions() will constructs the SymbolConjured with null Stmt. And this will lead to a crash in IteratorChecker.cpp.

Given the code below:

void null_terminator_loop_widen(int *a) {
  int c;
  for (;;) {
    c = *a;
    a++;
  }
}

I haven't found any problems with SymbolConjured containing null Stmt for the time being. So I just use
dyn_cast_or_null<> instead of dyn_cast<> in IteratorChecker.cpp, and didn't delve into the widen loop part.

[szepet]

Nice catch, it looks good to me! Thank you!
One small nit for future debugging people: Could you insert a comment line in the test case where you explain what is this all about? E.g what you just have written in the description: "invalidateRegions() will construct the SymbolConjured with null Stmt" or something like this.

[NoQ]

Just in case: we indeed do not guarantee that SymbolConjured corresponds to a statement; it is, however, not intended, but rather a bug. Consider:

 1	void clang_analyzer_eval(bool);
 2
 3	class C {
 4	  int &x;
 5	public:
 6	  C(int &x): x(x) {}
 7	  ~C();
 8	};
 9
10	void foo() {
11	  int x = 1;
12	  {
13	    C c(x);
14	  }
15	  int y = x;
16	  {
17	    C c(x);
18	  }
19	  int z = x;
20	  clang_analyzer_eval(y == z);
21	}

We currently evaluate clang_analyzer_eval(y == z) to TRUE but that's unsound because ~C(); could have been defined as ~C() { ++x; }. We make such unsound assumption because SymbolConjured put into x during destructor call on line 14 and on line 18 is the same conjured symbol because they have no statements attached because destructors are implicit and have no corresponding statements, and everything else about these two symbols is the same. So, well, we should work around this problem somehow.

---

Now the real question here is whether IteratorChecker should scan any of the symbols that have no statement attached. I believe that IteratorChecker should only be interested in symbols that were returned from specific functions that return iterators. In the test case there are no iterators at all, so i suspect that the iterator checker is scanning more symbols than it needs.

---

Regardless, the patch makes sense :)

[MTC]

One small nit for future debugging people: Could you insert a comment line in the test case where you explain what is this all about? E.g what you just have written in the description: "invalidateRegions() will construct the SymbolConjured with null Stmt" or something like this.

Thank you for pointing this out, and I'll add the comment.

[MTC]

Just in case: we indeed do not guarantee that SymbolConjured corresponds to a statement; it is, however, not intended, but rather a bug.

Thank you for your explanation and the reasonable example, NoQ.

[MTC]

Add the comments as suggested by @szepet .

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp

bool isDereferenceOperator(OverloadedOperatorKind OK) {
   return OK == OO_Star || OK == OO_Arrow || OK == OO_ArrowStar ||
          OK == OO_Subscript;
 }
 
 BinaryOperator::Opcode getOpcode(const SymExpr *SE) {
   if (const auto *BSE = dyn_cast<BinarySymExpr>(SE)) {
     return BSE->getOpcode();
   } else if (const auto *SC = dyn_cast<SymbolConjured>(SE)) {
-    const auto *COE = dyn_cast<CXXOperatorCallExpr>(SC->getStmt());
+    const auto *COE = dyn_cast_or_null<CXXOperatorCallExpr>(SC->getStmt());
     if (!COE)
       return BO_Comma; // Extremal value, neither EQ nor NE
     if (COE->getOperator() == OO_EqualEqual) {
       return BO_EQ;
     } else if (COE->getOperator() == OO_ExclaimEqual) {
       return BO_NE;
     }
     return BO_Comma; // Extremal value, neither EQ nor NE

test/Analysis/loop-widening.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
+// RUN: %clang_analyze_cc1 -DTEST_NULL_TERM -analyzer-checker=core,unix.Malloc,debug.ExprInspection,alpha.cplusplus.IteratorRange -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
 
 void clang_analyzer_eval(int);
 void clang_analyzer_warnIfReached();
 
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
 
for (i = 0; i < 2; i++) {
     clang_analyzer_eval(i < 2); // expected-warning {{TRUE}}
     for (j = 0; j < 10; j++) {
       clang_analyzer_eval(j < 10); // expected-warning {{TRUE}}
     }
     clang_analyzer_eval(j >= 10); // expected-warning {{TRUE}}
   }
   clang_analyzer_eval(i >= 2); // expected-warning {{TRUE}}
 }
+
+#ifdef TEST_NULL_TERM
+void null_terminator_loop_widen(int *a) {
+  int c;
+  // Loop widening will call 'invalidateRegions()' and 'invalidateRegions()'
+  // will construct the SymbolConjured with null Stmt because of the null
+  // terminator statement. Accessing the null Stmt will cause a crash.
+  for (;;) {
+    c = *a; // no-crash
+    a++;
+  }
+}
+#endif