[analyzer] Move `TaintBugVisitor` from `GenericTaintChecker.cpp` to `BugReporterVisitors.h`.

[MTC]

Summary

TaintBugVisitor is a universal visitor, and many checkers rely on it, such as ArrayBoundCheckerV2.cpp, DivZeroChecker.cpp and VLASizeChecker.cpp. Moving TaintBugVisitor to BugReporterVisitors.h enables other checker can also track where tainted value came from.

[george.karpenkov]

I'm new to the taint visitor, but I am quite confused by your change description.

and many checkers rely on it

How can other checkers rely on it if it's private to the taint checker?

Also, it's probably to explicitly include BugReporterVisitors.h in the checker file then.

[MTC]

In D45682#1074407, @george.karpenkov wrote:

I'm new to the taint visitor, but I am quite confused by your change description.

and many checkers rely on it

How can other checkers rely on it if it's private to the taint checker?

Thanks for your review, george! TaintBugVisitor is an utility to add extra information to illustrate where the taint information originated from. There are several checkers use taint information, e.g. ArrayBoundCheckerV2.cpp, in some cases it will report a warning, like warning: Out of bound memory access (index is tainted). If TaintBugVisitor moves to BugReporterVisitors.h, ArrayBoundCheckerV2 can add extra notes like Taint originated here to the report by adding TaintBugVisitor.

Also, it's probably to explicitly include BugReporterVisitors.h in the checker file then.

If these checkers want to add Taint originated here using TaintBugVisitor, it is necessary to explicitly include BugReporterVisitors.h in following patch.

[george.karpenkov]

can add extra notes like

Ah right, than it's "can rely on" rather than "rely on".

it is necessary to explicitly include

I've meant adding the include to GenericTaintChecker.cpp.

LGTM otherwise.

include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

public:
   void Profile(llvm::FoldingSetNodeID &ID) const override {}
 
   std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *Succ,
                                                  const ExplodedNode *Pred,
                                                  BugReporterContext &BRC,
                                                  BugReport &BR) override;
 };
 
+/// The bug visitor prints a diagnostic message at the location where a given
+/// variable was tainted.
+class TaintBugVisitor final : public BugReporterVisitorImpl<TaintBugVisitor> {
+private:
+  const SVal V;
+
+public:
+  TaintBugVisitor(const SVal V) : V(V) {}
+  void Profile(llvm::FoldingSetNodeID &ID) const override { ID.Add(V); }
+
+  std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+                                                 const ExplodedNode *PrevN,
+                                                 BugReporterContext &BRC,
+                                                 BugReport &BR) override;
+};
+
 namespace bugreporter {
 
 /// Attempts to add visitors to trace a null or undefined value back to its
 /// point of origin, whether it is a symbol constrained to null or an explicit
 /// assignment.
 ///
 /// \param N A node "downstream" from the evaluation of the statement.
 /// \param S The statement whose value is null or undefined.

lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

private:
   static const char MsgTaintedBufferSize[];
   bool checkTaintedBufferSize(const CallExpr *CE, const FunctionDecl *FDecl,
                               CheckerContext &C) const;
 
   /// Generate a report if the expression is tainted or points to tainted data.
   bool generateReportIfTainted(const Expr *E, const char Msg[],
                                CheckerContext &C) const;
 
-  /// The bug visitor prints a diagnostic message at the location where a given
-  /// variable was tainted.
-  class TaintBugVisitor
-      : public BugReporterVisitorImpl<TaintBugVisitor> {
-  private:
-    const SVal V;
-
-  public:
-    TaintBugVisitor(const SVal V) : V(V) {}
-    void Profile(llvm::FoldingSetNodeID &ID) const override { ID.Add(V); }
-
-    std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
-                                                   const ExplodedNode *PrevN,
-                                                   BugReporterContext &BRC,
-                                                   BugReport &BR) override;
-  };
-
   typedef SmallVector<unsigned, 2> ArgVector;
 
   /// \brief A struct used to specify taint propagation rules for a function.
   ///
   /// If any of the possible taint source arguments is tainted, all of the
   /// destination arguments should also be tainted. Use InvalidArgIndex in the
   /// src list to specify that all of the arguments can introduce taint. Use
   /// InvalidArgIndex in the dst arguments to signify that all the non-const

 } // end of anonymous namespace
 
 /// A set which is used to pass information from call pre-visit instruction
 /// to the call post-visit. The values are unsigned integers, which are either
 /// ReturnValueIndex, or indexes of the pointer/reference argument, which
 /// points to data, which should be tainted on return.
 REGISTER_SET_WITH_PROGRAMSTATE(TaintArgsOnPostVisit, unsigned)
 
-std::shared_ptr<PathDiagnosticPiece>
-GenericTaintChecker::TaintBugVisitor::VisitNode(const ExplodedNode *N,
-    const ExplodedNode *PrevN, BugReporterContext &BRC, BugReport &BR) {
-
-  // Find the ExplodedNode where the taint was first introduced
-  if (!N->getState()->isTainted(V) || PrevN->getState()->isTainted(V))
-    return nullptr;
-
-  const Stmt *S = PathDiagnosticLocation::getStmt(N);
-  if (!S)
-    return nullptr;
-
-  const LocationContext *NCtx = N->getLocationContext();
-  PathDiagnosticLocation L =
-      PathDiagnosticLocation::createBegin(S, BRC.getSourceManager(), NCtx);
-  if (!L.isValid() || !L.asLocation().isValid())
-    return nullptr;
-
-  return std::make_shared<PathDiagnosticEventPiece>(
-      L, "Taint originated here");
-}
-
 GenericTaintChecker::TaintPropagationRule
 GenericTaintChecker::TaintPropagationRule::getTaintPropagationRule(
                                                      const FunctionDecl *FDecl,
                                                      StringRef Name,
                                                      CheckerContext &C) {
   // TODO: Currently, we might lose precision here: we always mark a return
   // value as tainted even if it's just a pointer, pointing to tainted data.
 

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

CXXSelfAssignmentBRVisitor::VisitNode(const ExplodedNode *Succ,
   Out << "Assuming " << Met->getParamDecl(0)->getName() <<
     ((Param == This) ? " == " : " != ") << "*this";
 
   auto Piece = std::make_shared<PathDiagnosticEventPiece>(L, Out.str());
   Piece->addRange(Met->getSourceRange());
 
   return std::move(Piece);
 }
+
+std::shared_ptr<PathDiagnosticPiece>
+TaintBugVisitor::VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN,
+                           BugReporterContext &BRC, BugReport &BR) {
+
+  // Find the ExplodedNode where the taint was first introduced
+  if (!N->getState()->isTainted(V) || PrevN->getState()->isTainted(V))
+    return nullptr;
+
+  const Stmt *S = PathDiagnosticLocation::getStmt(N);
+  if (!S)
+    return nullptr;
+
+  const LocationContext *NCtx = N->getLocationContext();
+  PathDiagnosticLocation L =
+      PathDiagnosticLocation::createBegin(S, BRC.getSourceManager(), NCtx);
+  if (!L.isValid() || !L.asLocation().isValid())
+    return nullptr;
+
+  return std::make_shared<PathDiagnosticEventPiece>(L, "Taint originated here");
+}