[analyzer][NFC] Refactor GenericTaintChecker to use CallDescriptionMap

[gamesh411]

Summary

GenericTaintChecker now uses CallDescriptionMap to describe the possible
operation in code which trigger the introduction (sources), the removal
(filters), the passing along (propagations) and detection (sinks) of
tainted values.

[NoQ]

I haven't read this whole patch with full scrutiny but it sure looks lovely. I now also see what the problem is with non-static strings in call descriptions.

[gamesh411]

Tidy things up thanks to the recommendations of @steakhal

[steakhal]

Sweet stuff!

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

55

typo

163–164

312–313

I would expect this in plural form.

318

Definitely reads oddly. The return type is singular but the function parses name parts.

647

A comment like this would make it cleaner:
// We don't have external taint config, no parsing required.

750–752

It's unfortunate that we don't shortcircuit after the patch.

865

Use inclusive terms.

[gamesh411]

Fixes round two

[gamesh411]

Applied typo and naming fixes, introduced 2 move operations, and re-introduced short circuiting.

[gamesh411]

All (except the last) commits were excluded in the previous patch upload

[steakhal]

LGTM

[gamesh411]

Remove explicit template keyword for MSVC compatibility

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

 
 /// Check for:
 /// CERT/STR02-C. "Sanitize data passed to complex subsystems"
 /// CWE-78, "Failure to Sanitize Data into an OS Command"
 constexpr llvm::StringLiteral MsgSanitizeSystemArgs =
     "Untrusted data is passed to a system call "
     "(CERT/STR02-C. Sanitize data passed to complex subsystems)";
 
-/// Check if tainted data is used as a buffer size ins strn.. functions,
+/// Check if tainted data is used as a buffer size in strn.. functions,
 /// and allocators.
 constexpr llvm::StringLiteral MsgTaintedBufferSize =
     "Untrusted data is used to specify the buffer size "
     "(CERT/STR31-C. Guarantee that storage for strings has sufficient space "
     "for character data and the null terminator)";
 
 /// Check if tainted data is used as a custom sink's parameter.
 constexpr llvm::StringLiteral MsgCustomSink =

 
 /// ArgSet is used to describe arguments relevant for taint detection or
 /// taint application. A discrete set of argument indexes and a variadic
 /// argument list signified by a starting index are supported.
 class ArgSet {
 public:
   ArgSet() = default;
   ArgSet(ArgVecTy &&DiscreteArgs, Optional<ArgIdxTy> VariadicIndex = None)
-      : DiscreteArgs(DiscreteArgs), VariadicIndex(VariadicIndex) {}
+      : DiscreteArgs(std::move(DiscreteArgs)),
+        VariadicIndex(std::move(VariadicIndex)) {}
 
   bool contains(ArgIdxTy ArgIdx) const {
     if (llvm::is_contained(DiscreteArgs, ArgIdx))
       return true;
 
     return VariadicIndex && ArgIdx >= *VariadicIndex;
   }
 
struct GenericTaintRuleParser {
   /// Container type used to gather call identification objects grouped into
   /// pairs with their corresponding taint rules. It is temporary as it is used
   /// to finally initialize RuleLookupTy, which is considered to be immutable.
   using RulesContTy = std::vector<std::pair<CallDescription, GenericTaintRule>>;
   RulesContTy parseConfiguration(const std::string &Option,
                                  TaintConfiguration &&Config) const;
 
 private:
-  using NamePartTy = llvm::SmallVector<SmallString<32>, 2>;
+  using NamePartsTy = llvm::SmallVector<SmallString<32>, 2>;
 
   /// Validate part of the configuration, which contains a list of argument
   /// indexes.
   void validateArgVector(const std::string &Option, const ArgVecTy &Args) const;
 
-  template <typename Config> static NamePartTy parseNameParts(const Config &C);
+  template <typename Config> static NamePartsTy parseNameParts(const Config &C);
 
   // Takes the config and creates a CallDescription for it and associates a Rule
   // with that.
   template <typename Config>
   static void consumeRulesFromConfig(const Config &C, GenericTaintRule &&Rule,
                                      RulesContTy &Rules);
 
   void parseConfig(const std::string &Option, TaintConfiguration::Sink &&P,
if (Arg < ReturnValueIndex) {
       Mgr.reportInvalidCheckerOptionValue(
           Mgr.getChecker<GenericTaintChecker>(), Option,
           "an argument number for propagation rules greater or equal to -1");
     }
   }
 }
 
 template <typename Config>
-GenericTaintRuleParser::NamePartTy
+GenericTaintRuleParser::NamePartsTy
 GenericTaintRuleParser::parseNameParts(const Config &C) {
-  NamePartTy NameParts;
+  NamePartsTy NameParts;
   if (!C.Scope.empty()) {
     // If the Scope argument contains multiple "::" parts, those are considered
     // namespace identifiers.
     llvm::SmallVector<StringRef, 2> NSParts;
     StringRef{C.Scope}.split(NSParts, "::", /*MaxSplit*/ -1,
                              /*KeepEmpty*/ false);
     NameParts.append(NSParts.begin(), NSParts.end());
   }
   NameParts.emplace_back(C.Name);
   return NameParts;
 }
 
 template <typename Config>
 void GenericTaintRuleParser::consumeRulesFromConfig(const Config &C,
                                                     GenericTaintRule &&Rule,
                                                     RulesContTy &Rules) {
-  NamePartTy NameParts = parseNameParts(C);
+  NamePartsTy NameParts = parseNameParts(C);
   llvm::SmallVector<const char *, 2> CallDescParts{NameParts.size()};
   llvm::transform(NameParts, CallDescParts.begin(),
                   [](SmallString<32> &S) { return S.c_str(); });
   Rules.template emplace_back(CallDescParts, std::move(Rule));
 }
 
 void GenericTaintRuleParser::parseConfig(const std::string &Option,
                                          TaintConfiguration::Sink &&S,
void GenericTaintChecker::initTaintRules(CheckerContext &C) const {
   assert(Mgr);
   GenericTaintRuleParser ConfigParser{*Mgr};
   std::string Option{"Config"};
   StringRef ConfigFile =
       Mgr->getAnalyzerOptions().getCheckerStringOption(this, Option);
   llvm::Optional<TaintConfiguration> Config =
       getConfiguration<TaintConfiguration>(*Mgr, this, Option, ConfigFile);
   if (!Config) {
+    // We don't have external taint config, no parsing required.
     DynamicTaintRules = RuleLookupTy{};
     return;
   }
 
   GenericTaintRuleParser::RulesContTy Rules{
       ConfigParser.parseConfiguration(Option, std::move(Config.getValue()))};
 
   DynamicTaintRules.emplace(std::make_move_iterator(Rules.begin()),
                             std::make_move_iterator(Rules.end()));
ForEachCallArg([this, &C, &State](ArgIdxTy I, const Expr *E, SVal S) {
     }
   });
 
   /// Check for taint propagation sources.
   /// A rule is relevant if PropSrcArgs is empty, or if any of its signified
   /// args are tainted in context of the current CallEvent.
   bool IsMatching = PropSrcArgs.isEmpty();
   ForEachCallArg(
       [this, &C, &IsMatching, &State](ArgIdxTy I, const Expr *E, SVal) {
-        IsMatching |=
-            PropSrcArgs.contains(I) && isTaintedOrPointsToTainted(E, State, C);
+        IsMatching = IsMatching || (PropSrcArgs.contains(I) &&
+                                    isTaintedOrPointsToTainted(E, State, C));
       });
 
   if (!IsMatching)
     return;
 
   const auto WouldEscape = [](SVal V, QualType Ty) -> bool {
     if (!V.getAs<Loc>())
       return false;
void GenericTaintChecker::taintUnsafeSocketProtocol(const CallEvent &Call,
   const IdentifierInfo *ID = Call.getCalleeIdentifier();
   if (!ID)
     return;
   if (!ID->getName().equals("socket"))
     return;
 
   SourceLocation DomLoc = Call.getArgExpr(0)->getExprLoc();
   StringRef DomName = C.getMacroNameOrSpelling(DomLoc);
-  // White list the internal communication protocols.
+  // Allow internal communication protocols.
   bool SafeProtocol = DomName.equals("AF_SYSTEM") ||
                       DomName.equals("AF_LOCAL") || DomName.equals("AF_UNIX") ||
                       DomName.equals("AF_RESERVED_36");
   if (SafeProtocol)
     return;
 
   C.addTransition(C.getState()->add<TaintArgsOnPostVisit>(ReturnValueIndex));
 }