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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
