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

[Borsik Gábor via Phabricator via cfe-commits]

boga95 marked 6 inline comments as done.
boga95 added a comment.

@steakhal's revision is on the top of this. Changing the order will only cause unnecessary work on both sides.



================
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;
 
----------------
Szelethus wrote:
> Szelethus wrote:
> > I know this isn't really relevant, but isn't this redundant with `CallDescription`?
> Ah, okay, so `CallDescription` doesn't really have the `FunctionDecl`, but this still feels like a duplication of functionalities.
We have already thought about that and it is on our TODO list.


================
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;
----------------
Szelethus wrote:
> Extraction operator? Is that a thing?
I can call it `operator>>` if you think that is better.


================
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>;
----------------
Szelethus wrote:
> 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.
I didn't find any multimap among the alternatives. I think the performance won't be an issue here, because the elements are inserted at the beginning of the analysis if there is any. Otherwise, we are planning to replace it with CallDescriptionMap.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:268
                                            CheckerContext &C) {
-      if (isTainted(State, E, C.getLocationContext()) || isStdin(E, C))
+      if (isTainted(State, E, C.getLocationContext()) || isStdin(E, C) ||
+          isStdstream(E, C))
----------------
xazax.hun wrote:
> If we consider `Stdin`  and `Stdstream` to be tainted does it make sense to fold them into `isTainted` so we never miss checking for them?
Then we have to pass the `CheckerContext` to the `isTainted` functions. I think this function wraps it well.


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

https://reviews.llvm.org/D71524