Page MenuHomePhabricator

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

Authored by MTC on Mar 18 2018, 1:05 AM.

Details

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.

Diff Detail

Event Timeline

MTC created this revision.Mar 18 2018, 1:05 AM
MTC edited the summary of this revision. (Show Details)Mar 18 2018, 1:07 AM

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 accepted this revision.Mar 19 2018, 2:04 PM

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 :)

This revision is now accepted and ready to land.Mar 19 2018, 2:04 PM
MTC added a comment.Mar 19 2018, 6:29 PM

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 added a comment.Mar 19 2018, 6:47 PM

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 updated this revision to Diff 139075.Mar 19 2018, 10:08 PM

Add the comments as suggested by @szepet .

This revision was automatically updated to reflect the committed changes.