[PATCH] D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL331345: [analyzer] Add `TaintBugVisitor` to the 
ArrayBoundV2, DivideZero and VLASize. (authored by henrywong, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D46007?vs=143908&id=144861#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D46007

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  cfe/trunk/test/Analysis/taint-diagnostic-visitor.c

Index: cfe/trunk/test/Analysis/taint-diagnostic-visitor.c
===
--- cfe/trunk/test/Analysis/taint-diagnostic-visitor.c
+++ cfe/trunk/test/Analysis/taint-diagnostic-visitor.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core -analyzer-output=text -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 -analyzer-output=text -verify %s
 
 // This file is for testing enhanced diagnostics produced by the GenericTaintChecker
 
@@ -11,3 +11,26 @@
   scanf("%s", buf); // expected-note {{Taint originated here}}
   system(buf); // expected-warning {{Untrusted data is passed to a system call}} // expected-note {{Untrusted data is passed to a system call (CERT/STR02-C. Sanitize data passed to complex subsystems)}}
 }
+
+int taintDiagnosticOutOfBound() {
+  int index;
+  int Array[] = {1, 2, 3, 4, 5};
+  scanf("%d", &index); // expected-note {{Taint originated here}}
+  return Array[index]; // expected-warning {{Out of bound memory access (index is tainted)}}
+   // expected-note@-1 {{Out of bound memory access (index is tainted)}}
+}
+
+int taintDiagnosticDivZero(int operand) {
+  scanf("%d", &operand); // expected-note {{Value assigned to 'operand'}}
+ // expected-note@-1 {{Taint originated here}}
+  return 10 / operand; // expected-warning {{Division by a tainted value, possibly zero}}
+   // expected-note@-1 {{Division by a tainted value, possibly zero}}
+}
+
+void taintDiagnosticVLA() {
+  int x;
+  scanf("%d", &x); // expected-note {{Value assigned to 'x'}}
+   // expected-note@-1 {{Taint originated here}}
+  int vla[x]; // expected-warning {{Declared variable-length array (VLA) has tainted size}}
+  // expected-note@-1 {{Declared variable-length array (VLA) has tainted size}}
+}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
@@ -24,22 +24,23 @@
 namespace {
 class DivZeroChecker : public Checker< check::PreStmt > {
   mutable std::unique_ptr BT;
-  void reportBug(const char *Msg,
- ProgramStateRef StateZero,
- CheckerContext &C) const ;
+  void reportBug(const char *Msg, ProgramStateRef StateZero, CheckerContext &C,
+ std::unique_ptr Visitor = nullptr) const;
+
 public:
   void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const;
 };
 } // end anonymous namespace
 
-void DivZeroChecker::reportBug(const char *Msg,
-   ProgramStateRef StateZero,
-   CheckerContext &C) const {
+void DivZeroChecker::reportBug(
+const char *Msg, ProgramStateRef StateZero, CheckerContext &C,
+std::unique_ptr Visitor) const {
   if (ExplodedNode *N = C.generateErrorNode(StateZero)) {
 if (!BT)
   BT.reset(new BuiltinBug(this, "Division by zero"));
 
 auto R = llvm::make_unique(*BT, Msg, N);
+R->addVisitor(std::move(Visitor));
 bugreporter::trackNullOrUndefValue(N, bugreporter::GetDenomExpr(N), *R);
 C.emitReport(std::move(R));
   }
@@ -78,7 +79,8 @@
 
   bool TaintedD = C.getState()->isTainted(*DV);
   if ((stateNotZero && stateZero && TaintedD)) {
-reportBug("Division by a tainted value, possibly zero", stateZero, C);
+reportBug("Division by a tainted value, possibly zero", stateZero, C,
+  llvm::make_unique(*DV));
 return;
   }
 
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -33,8 +33,8 @@
 
   enum OOB_Kind { OOB_Precedes, OOB_Excedes, OOB_Tainted };
 
-  void reportOOB(CheckerContext &C, ProgramStateRef errorState,
- OOB_Kind kind) const;
+  void reportOOB(CheckerContext &C, ProgramStateRef errorState, OOB_Kind kind,
+ std::unique_ptr Visitor = nullptr) const;
 
 public:
   void checkLocation(SVal l, bool isLoad, const Stmt*S,
@@ -

[PATCH] D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize.

[Artem Dergachev via Phabricator via cfe-commits]

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

Looks great, thanks!

I think the overall plan for any taint work would be to remove it from the 
program state API and move getters/setters into its own translation unit (like 
dynamic type propagation) as part of the overall plan to introduce shared 
checker states. So, like, not just the visitor, but the whole trait itself.


Repository:
  rC Clang

https://reviews.llvm.org/D46007



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

[PATCH] D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 143908.
MTC marked an inline comment as done.
MTC added a comment.

Since `BugReport::addVisitor()` has checks for the null `Visitor`, remove the 
checks before `BugReport->addVisitor()`.


Repository:
  rC Clang

https://reviews.llvm.org/D46007

Files:
  lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
  lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  test/Analysis/taint-diagnostic-visitor.c

Index: test/Analysis/taint-diagnostic-visitor.c
===
--- test/Analysis/taint-diagnostic-visitor.c
+++ test/Analysis/taint-diagnostic-visitor.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core -analyzer-output=text -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 -analyzer-output=text -verify %s
 
 // This file is for testing enhanced diagnostics produced by the GenericTaintChecker
 
@@ -11,3 +11,26 @@
   scanf("%s", buf); // expected-note {{Taint originated here}}
   system(buf); // expected-warning {{Untrusted data is passed to a system call}} // expected-note {{Untrusted data is passed to a system call (CERT/STR02-C. Sanitize data passed to complex subsystems)}}
 }
+
+int taintDiagnosticOutOfBound() {
+  int index;
+  int Array[] = {1, 2, 3, 4, 5};
+  scanf("%d", &index); // expected-note {{Taint originated here}}
+  return Array[index]; // expected-warning {{Out of bound memory access (index is tainted)}}
+   // expected-note@-1 {{Out of bound memory access (index is tainted)}}
+}
+
+int taintDiagnosticDivZero(int operand) {
+  scanf("%d", &operand); // expected-note {{Value assigned to 'operand'}}
+ // expected-note@-1 {{Taint originated here}}
+  return 10 / operand; // expected-warning {{Division by a tainted value, possibly zero}}
+   // expected-note@-1 {{Division by a tainted value, possibly zero}}
+}
+
+void taintDiagnosticVLA() {
+  int x;
+  scanf("%d", &x); // expected-note {{Value assigned to 'x'}}
+   // expected-note@-1 {{Taint originated here}}
+  int vla[x]; // expected-warning {{Declared variable-length array (VLA) has tainted size}}
+  // expected-note@-1 {{Declared variable-length array (VLA) has tainted size}}
+}
Index: lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
@@ -32,19 +32,18 @@
   mutable std::unique_ptr BT;
   enum VLASize_Kind { VLA_Garbage, VLA_Zero, VLA_Tainted, VLA_Negative };
 
-  void reportBug(VLASize_Kind Kind,
- const Expr *SizeE,
- ProgramStateRef State,
- CheckerContext &C) const;
+  void reportBug(VLASize_Kind Kind, const Expr *SizeE, ProgramStateRef State,
+ CheckerContext &C,
+ std::unique_ptr Visitor = nullptr) const;
+
 public:
   void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const;
 };
 } // end anonymous namespace
 
-void VLASizeChecker::reportBug(VLASize_Kind Kind,
-   const Expr *SizeE,
-   ProgramStateRef State,
-   CheckerContext &C) const {
+void VLASizeChecker::reportBug(
+VLASize_Kind Kind, const Expr *SizeE, ProgramStateRef State,
+CheckerContext &C, std::unique_ptr Visitor) const {
   // Generate an error node.
   ExplodedNode *N = C.generateErrorNode(State);
   if (!N)
@@ -73,6 +72,7 @@
   }
 
   auto report = llvm::make_unique(*BT, os.str(), N);
+  report->addVisitor(std::move(Visitor));
   report->addRange(SizeE->getSourceRange());
   bugreporter::trackNullOrUndefValue(N, SizeE, *report);
   C.emitReport(std::move(report));
@@ -108,7 +108,8 @@
 
   // Check if the size is tainted.
   if (state->isTainted(sizeV)) {
-reportBug(VLA_Tainted, SE, nullptr, C);
+reportBug(VLA_Tainted, SE, nullptr, C,
+  llvm::make_unique(sizeV));
 return;
   }
 
Index: lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
@@ -24,22 +24,23 @@
 namespace {
 class DivZeroChecker : public Checker< check::PreStmt > {
   mutable std::unique_ptr BT;
-  void reportBug(const char *Msg,
- ProgramStateRef StateZero,
- CheckerContext &C) const ;
+  void reportBug(const char *Msg, ProgramStateRef StateZero, CheckerContext &C,
+ std::unique_ptr Visitor = nullptr) const;
+
 public:
   void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const;
 };
 } // end anonymous namespace
 
-void DivZeroChecker::reportBug(const char *Msg,
-   ProgramStateR

[PATCH] D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize.

[Henry Wong via Phabricator via cfe-commits]

MTC marked an inline comment as done.
MTC added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp:75
   auto report = llvm::make_unique(*BT, os.str(), N);
+  report->addVisitor(std::move(Visitor));
   report->addRange(SizeE->getSourceRange());

a.sidorin wrote:
> In this patch, sometimes we check the visitor to be non-null, sometimes not.  
> As I can see, `BugReport::addVisitor()` works well with `nullptr` arguments 
> (it checks arguments) so I think we can omit the checks.
Thanks for your reminder, a.sidorin! 

My mistakes led to some checkers doing the check and some did not check!  But 
as you said, there is no need to check the nullptr.

I will update the patch.


Repository:
  rC Clang

https://reviews.llvm.org/D46007



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

[PATCH] D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize.

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Mostly LG.




Comment at: lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp:75
   auto report = llvm::make_unique(*BT, os.str(), N);
+  report->addVisitor(std::move(Visitor));
   report->addRange(SizeE->getSourceRange());

In this patch, sometimes we check the visitor to be non-null, sometimes not.  
As I can see, `BugReport::addVisitor()` works well with `nullptr` arguments (it 
checks arguments) so I think we can omit the checks.


Repository:
  rC Clang

https://reviews.llvm.org/D46007



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

[PATCH] D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize.

[Henry Wong via Phabricator via cfe-commits]

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

Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero, VLASize to be able to 
indicate where the taint information originated from.


Repository:
  rC Clang

https://reviews.llvm.org/D46007

Files:
  lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
  lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  test/Analysis/taint-diagnostic-visitor.c

Index: test/Analysis/taint-diagnostic-visitor.c
===
--- test/Analysis/taint-diagnostic-visitor.c
+++ test/Analysis/taint-diagnostic-visitor.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core -analyzer-output=text -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 -analyzer-output=text -verify %s
 
 // This file is for testing enhanced diagnostics produced by the GenericTaintChecker
 
@@ -11,3 +11,26 @@
   scanf("%s", buf); // expected-note {{Taint originated here}}
   system(buf); // expected-warning {{Untrusted data is passed to a system call}} // expected-note {{Untrusted data is passed to a system call (CERT/STR02-C. Sanitize data passed to complex subsystems)}}
 }
+
+int taintDiagnosticOutOfBound() {
+  int index;
+  int Array[] = {1, 2, 3, 4, 5};
+  scanf("%d", &index); // expected-note {{Taint originated here}}
+  return Array[index]; // expected-warning {{Out of bound memory access (index is tainted)}}
+   // expected-note@-1 {{Out of bound memory access (index is tainted)}}
+}
+
+int taintDiagnosticDivZero(int operand) {
+  scanf("%d", &operand); // expected-note {{Value assigned to 'operand'}}
+ // expected-note@-1 {{Taint originated here}}
+  return 10 / operand; // expected-warning {{Division by a tainted value, possibly zero}}
+   // expected-note@-1 {{Division by a tainted value, possibly zero}}
+}
+
+void taintDiagnosticVLA() {
+  int x;
+  scanf("%d", &x); // expected-note {{Value assigned to 'x'}}
+   // expected-note@-1 {{Taint originated here}}
+  int vla[x]; // expected-warning {{Declared variable-length array (VLA) has tainted size}}
+  // expected-note@-1 {{Declared variable-length array (VLA) has tainted size}}
+}
Index: lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
@@ -32,19 +32,18 @@
   mutable std::unique_ptr BT;
   enum VLASize_Kind { VLA_Garbage, VLA_Zero, VLA_Tainted, VLA_Negative };
 
-  void reportBug(VLASize_Kind Kind,
- const Expr *SizeE,
- ProgramStateRef State,
- CheckerContext &C) const;
+  void reportBug(VLASize_Kind Kind, const Expr *SizeE, ProgramStateRef State,
+ CheckerContext &C,
+ std::unique_ptr Visitor = nullptr) const;
+
 public:
   void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const;
 };
 } // end anonymous namespace
 
-void VLASizeChecker::reportBug(VLASize_Kind Kind,
-   const Expr *SizeE,
-   ProgramStateRef State,
-   CheckerContext &C) const {
+void VLASizeChecker::reportBug(
+VLASize_Kind Kind, const Expr *SizeE, ProgramStateRef State,
+CheckerContext &C, std::unique_ptr Visitor) const {
   // Generate an error node.
   ExplodedNode *N = C.generateErrorNode(State);
   if (!N)
@@ -73,6 +72,7 @@
   }
 
   auto report = llvm::make_unique(*BT, os.str(), N);
+  report->addVisitor(std::move(Visitor));
   report->addRange(SizeE->getSourceRange());
   bugreporter::trackNullOrUndefValue(N, SizeE, *report);
   C.emitReport(std::move(report));
@@ -108,7 +108,8 @@
 
   // Check if the size is tainted.
   if (state->isTainted(sizeV)) {
-reportBug(VLA_Tainted, SE, nullptr, C);
+reportBug(VLA_Tainted, SE, nullptr, C,
+  llvm::make_unique(sizeV));
 return;
   }
 
Index: lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
@@ -24,22 +24,25 @@
 namespace {
 class DivZeroChecker : public Checker< check::PreStmt > {
   mutable std::unique_ptr BT;
-  void reportBug(const char *Msg,
- ProgramStateRef StateZero,
- CheckerContext &C) const ;
+  void reportBug(const char *Msg, ProgramStateRef StateZero, CheckerContext &C,
+ std::unique_ptr Visitor = nullptr) const;
+
 public:
   void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const;
 };
 } // end anonymous namespace
 
-void DivZeroChecker::reportBug(c