This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][NFC] Use CallEvent checker callback in GenericTaintChecker
ClosedPublic

Authored by steakhal on Dec 31 2019, 4:04 AM.

Details

Summary

Intended to be a non-functional change but it turned out CallEvent handles constructor calls unlike CallExpr which doesn't triggered for constructors.

All in all, this change shouldn't be observable since constructors are not yet propagating taintness like functions.
In the future constructors should propagate taintness as well.

This change includes:

  • NFCi change all uses of the CallExpr to CallEvent
  • NFC rename some functions, mark static them etc.
  • NFC omit explicit TaintPropagationRule type in switches
  • NFC apply some clang-tidy fixits

Diff Detail

Event Timeline

steakhal created this revision.Dec 31 2019, 4:04 AM
steakhal added inline comments.Dec 31 2019, 4:13 AM
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
358

I'm not sure why should we adjust (workaround) the number of arguments of CXXInstanceCalls calls, can someone explain it to me?

The same question raised for getArg too.

554

I'm not sure if this is the right way.

NoQ added inline comments.Dec 31 2019, 7:04 AM
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
358

Remove this :)

I think this is about this inconsistency with operator calls where one of {decl, expr} treats this as an argument, but the other doesn't. CallEvent automatically accounts for that (see getAdjustedParameterIndex() and getASTArgumentIndex() as they're overridden in various sub-classes of CallEvent).

427–441

Pls eventually transform this into CallDescriptionMap ^.^

554

You might want to cast Call to CXXMemberOperatorCall but i'm not sure it saves you anything.

Hmm, have you branched off of D71524? If so, this patch should definitely land first.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
158

Nice!

358

I have some thoughts about this here: D71524#1859435.

steakhal updated this revision to Diff 243492.Feb 10 2020, 2:58 AM

Rebased on top of master, instead of D71524.

Szelethus accepted this revision.Feb 24 2020, 7:33 AM

Wow. Its a joy to see you do C++. LGTM. Are you planning to introduce CallDescriptionMap at one point? :)

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
492–510

Whoa, this is amazing, but looks like a google interview question or something -- is this a technique I should know about it? I feel like we kinda sacrificed readability for coolness.

This revision is now accepted and ready to land.Feb 24 2020, 7:33 AM
steakhal marked 5 inline comments as done.Feb 25 2020, 5:56 AM

Wow. Its a joy to see you do C++. LGTM. Are you planning to introduce CallDescriptionMap at one point? :)

Yes, definitely.

clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
492–510

Actually, I agree but still convinced that it is the future-compatible way of doing this.

const auto OneOf = [FDecl](const auto &... Name) {
  // FIXME: use fold expression in C++17
  using unused = int[];
  bool ret = false;
  static_cast<void>(unused{
      0, (ret |= CheckerContext::isCLibraryFunction(FDecl, Name), 0)...});
  return ret;
};

In C++17 using fold expressions the whole lambda could be implemented such a way:

const auto OneOf = [FDecl](const auto &... Name) {
  return (CheckerContext::isCLibraryFunction(FDecl, Name) || ...);
};
This revision was automatically updated to reflect the committed changes.
NoQ added inline comments.Apr 20 2020, 6:16 AM
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
113
steakhal marked 2 inline comments as done.Apr 20 2020, 7:57 AM
steakhal added inline comments.
clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
113

Nice catch! Thank you for the fix.