[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

I strongly agree with this comment: D70878#1780513 <https://reviews.llvm.org/D70878#1780513>, maybe the placement of functions like `getArg` and `getNumArgs` would be most appropriate in `CallDescription`. How about we try to cut down on duplicating functionalities?

I realize that many, if not all of my comments should've been placed at D70878 <https://reviews.llvm.org/D70878>, I was unfortunately not available then, but I think it would be a lot better if we settled on this before making the eventual changes too hard to switch. Checkers that were written with `CallDescriptionMap` (D70818 <https://reviews.llvm.org/D70818>, D63915 <https://reviews.llvm.org/D63915>) or have recently been converted to it (D67706 <https://reviews.llvm.org/D67706>, D62557 <https://reviews.llvm.org/D62557>, D68165 <https://reviews.llvm.org/D68165>) are a joy to look at, and reduce the overall complexity of the codebase greatly. Are there strong arguments against it?

The overall direction is great, as demonstrated by the test files. Its very exciting to see taintness analysis improving this much lately!



================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:103-132
   struct FunctionData {
     FunctionData() = delete;
     FunctionData(const FunctionData &) = default;
     FunctionData(FunctionData &&) = default;
     FunctionData &operator=(const FunctionData &) = delete;
     FunctionData &operator=(FunctionData &&) = delete;
 
----------------
I know this isn't really relevant, but isn't this redundant with `CallDescription`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:139
 
+  /// Add taint sources for extraction operator on pre-visit.
+  bool addOverloadedOpPre(const CallExpr *CE, CheckerContext &C) const;
----------------
Extraction operator? Is that a thing?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:202
   using ConfigDataMap =
       std::unordered_multimap<std::string, std::pair<std::string, T>>;
   using NameRuleMap = ConfigDataMap<TaintPropagationRule>;
----------------
http://llvm.org/docs/ProgrammersManual.html#other-map-like-container-options

> We never use hash_set and unordered_set because they are generally very expensive (each insertion requires a malloc) and very non-portable.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:373
+/// Treat implicit this parameter as the 0. argument.
+const Expr *getArg(const CallExpr *CE, unsigned ArgNum) {
+  if (const auto *MCE = dyn_cast<CXXMemberCallExpr>(CE)) {
----------------
I would be shocked if this is the first time I've come across very similar function -- in any case, could you rename it to something like `getArgIgnoringImplicitThis`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:816
+bool GenericTaintChecker::isStdstream(const Expr *E, CheckerContext &C) {
+  llvm::Regex R{".*std::basic_istream.*"};
+  std::string Error;
----------------
In this case, maybe `llvm::Regex` is overkill?  [[https://llvm.org/doxygen/classllvm_1_1StringRef.html#a82369bea2700347f68e1f43e30d2d47b | `llvm::StringRef::find()`]] may be sufficient.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71524/new/

https://reviews.llvm.org/D71524