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

[MTC]

Summary

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

[a.sidorin]

Mostly LG.

lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

75

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.

[MTC]

lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

75

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.

[MTC]

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

[NoQ]

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.

lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp

 
 namespace {
 class ArrayBoundCheckerV2 :
     public Checker<check::Location> {
   mutable std::unique_ptr<BuiltinBug> BT;
 
   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<BugReporterVisitor> Visitor = nullptr) const;
 
 public:
   void checkLocation(SVal l, bool isLoad, const Stmt*S,
                      CheckerContext &C) const;
 };
 
 // FIXME: Eventually replace RegionRawOffset with this class.
 class RegionRawOffsetV2 {
if (!upperboundToCheck)
       break;
 
     ProgramStateRef state_exceedsUpperBound, state_withinUpperBound;
     std::tie(state_exceedsUpperBound, state_withinUpperBound) =
       state->assume(*upperboundToCheck);
 
     // If we are under constrained and the index variables are tainted, report.
     if (state_exceedsUpperBound && state_withinUpperBound) {
-      if (state->isTainted(rawOffset.getByteOffset())) {
-        reportOOB(checkerContext, state_exceedsUpperBound, OOB_Tainted);
+      SVal ByteOffset = rawOffset.getByteOffset();
+      if (state->isTainted(ByteOffset)) {
+        reportOOB(checkerContext, state_exceedsUpperBound, OOB_Tainted,
+                  llvm::make_unique<TaintBugVisitor>(ByteOffset));
         return;
       }
     } else if (state_exceedsUpperBound) {
       // If we are constrained enough to definitely exceed the upper bound,
       // report.
       assert(!state_withinUpperBound);
       reportOOB(checkerContext, state_exceedsUpperBound, OOB_Excedes);
       return;
     }
 
     assert(state_withinUpperBound);
     state = state_withinUpperBound;
   }
   while (false);
 
   if (state != originalState)
     checkerContext.addTransition(state);
 }
 
-void ArrayBoundCheckerV2::reportOOB(CheckerContext &checkerContext,
-                                    ProgramStateRef errorState,
-                                    OOB_Kind kind) const {
+void ArrayBoundCheckerV2::reportOOB(
+    CheckerContext &checkerContext, ProgramStateRef errorState, OOB_Kind kind,
+    std::unique_ptr<BugReporterVisitor> Visitor) const {
 
   ExplodedNode *errorNode = checkerContext.generateErrorNode(errorState);
   if (!errorNode)
     return;
 
   if (!BT)
     BT.reset(new BuiltinBug(this, "Out-of-bound access"));
 
void ArrayBoundCheckerV2::reportOOB(
   case OOB_Excedes:
     os << "(access exceeds upper limit of memory block)";
     break;
   case OOB_Tainted:
     os << "(index is tainted)";
     break;
   }
 
-  checkerContext.emitReport(
-      llvm::make_unique<BugReport>(*BT, os.str(), errorNode));
+  auto BR = llvm::make_unique<BugReport>(*BT, os.str(), errorNode);
+  BR->addVisitor(std::move(Visitor));
+  checkerContext.emitReport(std::move(BR));
 }
 
 #ifndef NDEBUG
 LLVM_DUMP_METHOD void RegionRawOffsetV2::dump() const {
   dumpToStream(llvm::errs());
 }
 
 void RegionRawOffsetV2::dumpToStream(raw_ostream &os) const {

lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp

 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 class DivZeroChecker : public Checker< check::PreStmt<BinaryOperator> > {
   mutable std::unique_ptr<BuiltinBug> BT;
-  void reportBug(const char *Msg,
-                 ProgramStateRef StateZero,
-                 CheckerContext &C) const ;
+  void reportBug(const char *Msg, ProgramStateRef StateZero, CheckerContext &C,
+                 std::unique_ptr<BugReporterVisitor> 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<BugReporterVisitor> Visitor) const {
   if (ExplodedNode *N = C.generateErrorNode(StateZero)) {
     if (!BT)
       BT.reset(new BuiltinBug(this, "Division by zero"));
 
     auto R = llvm::make_unique<BugReport>(*BT, Msg, N);
+    R->addVisitor(std::move(Visitor));
     bugreporter::trackNullOrUndefValue(N, bugreporter::GetDenomExpr(N), *R);
     C.emitReport(std::move(R));
   }
 }
 
 void DivZeroChecker::checkPreStmt(const BinaryOperator *B,
                                   CheckerContext &C) const {
   BinaryOperator::Opcode Op = B->getOpcode();
void DivZeroChecker::checkPreStmt(const BinaryOperator *B,
   if (!stateNotZero) {
     assert(stateZero);
     reportBug("Division by zero", stateZero, C);
     return;
   }
 
   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<TaintBugVisitor>(*DV));
     return;
   }
 
   // If we get here, then the denom should not be zero. We abandon the implicit
   // zero denom case for now.
   C.addTransition(stateNotZero);
 }
 
 void ento::registerDivZeroChecker(CheckerManager &mgr) {
   mgr.registerChecker<DivZeroChecker>();
 }

lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

 using namespace clang;
 using namespace ento;
 
 namespace {
 class VLASizeChecker : public Checker< check::PreStmt<DeclStmt> > {
   mutable std::unique_ptr<BugType> 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<BugReporterVisitor> 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<BugReporterVisitor> Visitor) const {
   // Generate an error node.
   ExplodedNode *N = C.generateErrorNode(State);
   if (!N)
     return;
 
   if (!BT)
     BT.reset(new BuiltinBug(
         this, "Dangerous variable-length array (VLA) declaration"));
case VLA_Tainted:
     os << "has tainted size";
     break;
   case VLA_Negative:
     os << "has negative size";
     break;
   }
 
   auto report = llvm::make_unique<BugReport>(*BT, os.str(), N);
+  report->addVisitor(std::move(Visitor));
   report->addRange(SizeE->getSourceRange());
   bugreporter::trackNullOrUndefValue(N, SizeE, *report);
   C.emitReport(std::move(report));
 }
 
 void VLASizeChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
   if (!DS->isSingleDecl())
     return;
void VLASizeChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
 
   // See if the size value is known. It can't be undefined because we would have
   // warned about that already.
   if (sizeV.isUnknown())
     return;
 
   // 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<TaintBugVisitor>(sizeV));
     return;
   }
 
   // Check if the size is zero.
   DefinedSVal sizeD = sizeV.castAs<DefinedSVal>();
 
   ProgramStateRef stateNotZero, stateZero;
   std::tie(stateNotZero, stateZero) = state->assume(sizeD);

test/Analysis/taint-diagnostic-visitor.c

-// 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
 
 int scanf(const char *restrict format, ...);
 int system(const char *command);
 
 void taintDiagnostic()
 {
   char buf[128];
   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}}
+}