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

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL327962: [analyzer] Fix the crash in IteratorChecker.cpp when 
'SymbolConjured' has a… (authored by henrywong, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D44606?vs=139075&id=139091#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D44606

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  cfe/trunk/test/Analysis/loop-widening.c


Index: cfe/trunk/test/Analysis/loop-widening.c
===
--- cfe/trunk/test/Analysis/loop-widening.c
+++ cfe/trunk/test/Analysis/loop-widening.c
@@ -1,4 +1,5 @@
 // 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();
@@ -188,3 +189,16 @@
   }
   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
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -604,7 +604,7 @@
   if (const auto *BSE = dyn_cast(SE)) {
 return BSE->getOpcode();
   } else if (const auto *SC = dyn_cast(SE)) {
-const auto *COE = dyn_cast(SC->getStmt());
+const auto *COE = dyn_cast_or_null(SC->getStmt());
 if (!COE)
   return BO_Comma; // Extremal value, neither EQ nor NE
 if (COE->getOperator() == OO_EqualEqual) {


Index: cfe/trunk/test/Analysis/loop-widening.c
===
--- cfe/trunk/test/Analysis/loop-widening.c
+++ cfe/trunk/test/Analysis/loop-widening.c
@@ -1,4 +1,5 @@
 // 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();
@@ -188,3 +189,16 @@
   }
   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
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -604,7 +604,7 @@
   if (const auto *BSE = dyn_cast(SE)) {
 return BSE->getOpcode();
   } else if (const auto *SC = dyn_cast(SE)) {
-const auto *COE = dyn_cast(SC->getStmt());
+const auto *COE = dyn_cast_or_null(SC->getStmt());
 if (!COE)
   return BO_Comma; // Extremal value, neither EQ nor NE
 if (COE->getOperator() == OO_EqualEqual) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 139075.
MTC added a comment.

Add the comments as suggested by @szepet .


Repository:
  rC Clang

https://reviews.llvm.org/D44606

Files:
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  test/Analysis/loop-widening.c


Index: test/Analysis/loop-widening.c
===
--- test/Analysis/loop-widening.c
+++ test/Analysis/loop-widening.c
@@ -1,4 +1,5 @@
 // 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();
@@ -188,3 +189,16 @@
   }
   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
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -604,7 +604,7 @@
   if (const auto *BSE = dyn_cast(SE)) {
 return BSE->getOpcode();
   } else if (const auto *SC = dyn_cast(SE)) {
-const auto *COE = dyn_cast(SC->getStmt());
+const auto *COE = dyn_cast_or_null(SC->getStmt());
 if (!COE)
   return BO_Comma; // Extremal value, neither EQ nor NE
 if (COE->getOperator() == OO_EqualEqual) {


Index: test/Analysis/loop-widening.c
===
--- test/Analysis/loop-widening.c
+++ test/Analysis/loop-widening.c
@@ -1,4 +1,5 @@
 // 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();
@@ -188,3 +189,16 @@
   }
   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
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -604,7 +604,7 @@
   if (const auto *BSE = dyn_cast(SE)) {
 return BSE->getOpcode();
   } else if (const auto *SC = dyn_cast(SE)) {
-const auto *COE = dyn_cast(SC->getStmt());
+const auto *COE = dyn_cast_or_null(SC->getStmt());
 if (!COE)
   return BO_Comma; // Extremal value, neither EQ nor NE
 if (COE->getOperator() == OO_EqualEqual) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

> 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.


Repository:
  rC Clang

https://reviews.llvm.org/D44606



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

> 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.


Repository:
  rC Clang

https://reviews.llvm.org/D44606



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

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

   1void clang_analyzer_eval(bool);
   2
   3class C {
   4  int &x;
   5public:
   6  C(int &x): x(x) {}
   7  ~C();
   8};
   9
  10void foo() {
  11  int x = 1;
  12  {
  13C c(x);
  14  }
  15  int y = x;
  16  {
  17C 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 :)


Repository:
  rC Clang

https://reviews.llvm.org/D44606



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Peter Szecsi via Phabricator via cfe-commits]

szepet added a comment.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D44606



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: NoQ, baloghadamsoftware, szepet, dcoughlin.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, xazax.hun.
Herald added a reviewer: george.karpenkov.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D44606

Files:
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  test/Analysis/loop-widening.c


Index: test/Analysis/loop-widening.c
===
--- test/Analysis/loop-widening.c
+++ test/Analysis/loop-widening.c
@@ -1,4 +1,5 @@
 // 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();
@@ -188,3 +189,13 @@
   }
   clang_analyzer_eval(i >= 2); // expected-warning {{TRUE}}
 }
+
+#ifdef TEST_NULL_TERM
+void null_terminator_loop_widen(int *a) {
+  int c;
+  for (;;) {
+c = *a; // no-crash
+a++;
+  }
+}
+#endif
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -604,7 +604,7 @@
   if (const auto *BSE = dyn_cast(SE)) {
 return BSE->getOpcode();
   } else if (const auto *SC = dyn_cast(SE)) {
-const auto *COE = dyn_cast(SC->getStmt());
+const auto *COE = dyn_cast_or_null(SC->getStmt());
 if (!COE)
   return BO_Comma; // Extremal value, neither EQ nor NE
 if (COE->getOperator() == OO_EqualEqual) {


Index: test/Analysis/loop-widening.c
===
--- test/Analysis/loop-widening.c
+++ test/Analysis/loop-widening.c
@@ -1,4 +1,5 @@
 // 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();
@@ -188,3 +189,13 @@
   }
   clang_analyzer_eval(i >= 2); // expected-warning {{TRUE}}
 }
+
+#ifdef TEST_NULL_TERM
+void null_terminator_loop_widen(int *a) {
+  int c;
+  for (;;) {
+c = *a; // no-crash
+a++;
+  }
+}
+#endif
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -604,7 +604,7 @@
   if (const auto *BSE = dyn_cast(SE)) {
 return BSE->getOpcode();
   } else if (const auto *SC = dyn_cast(SE)) {
-const auto *COE = dyn_cast(SC->getStmt());
+const auto *COE = dyn_cast_or_null(SC->getStmt());
 if (!COE)
   return BO_Comma; // Extremal value, neither EQ nor NE
 if (COE->getOperator() == OO_EqualEqual) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits