[PATCH] D59637: [analyzer] Use the custom propagation rules and sinks in GenericTaintChecker

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

boga95 closed this revision.
boga95 marked 10 inline comments as done.
boga95 added a comment.

Closed by 080ecafdd8b3e990e5ad19202d089c91c9c9b164 <https://reviews.llvm.org/rG080ecafdd8b3e990e5ad19202d089c91c9c9b164>.



================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:251
+const llvm::StringLiteral GenericTaintChecker::MsgCustomSink =
+    "Untrusted data is passed to a user defined sink";
+;
----------------
NoQ wrote:
> It should be "user-defined" because it's a single adjective.
> 
> I recommend against using the word "sink" in user-facing messages because it's too jargony. Do we have a better word for this?
Currently, I don't have any better idea. I will think about it.


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:118
   /// Check for CWE-134: Uncontrolled Format String.
-  static const char MsgUncontrolledFormatString[];
+  static constexpr llvm::StringLiteral MsgUncontrolledFormatString =
+      "Untrusted data is used as a format string "
----------------
steakhal wrote:
> Shouldn't we still need an out-of-class initializer part for each static constexpr class member variable?
> These would provide the memory locations for the declarations.
> ```
> constexpr llvm::StringLiteral GenericTaintChecker::MsgUncontrolledFormatString;
> constexpr llvm::StringLiteral GenericTaintChecker::MsgSanitizeSystemArgs;
> constexpr llvm::StringLiteral GenericTaintChecker::MsgTaintedBufferSize;
> constexpr llvm::StringLiteral GenericTaintChecker::MsgCustomSink;
> ```
Constexpr values cannot be initialized out of the class, that's why I moved them to here.


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:191
     static TaintPropagationRule
-    getTaintPropagationRule(const FunctionDecl *FDecl, StringRef Name,
+    getTaintPropagationRule(const GenericTaintChecker *Checker,
+                            const FunctionDecl *FDecl, StringRef Name,
----------------
steakhal wrote:
> Szelethus wrote:
> > How about only passing `CustomPropagations`?
> I would even consider to move this function out of the whole class. (Not only this function, but the others as well. Like isStdin, etc.)
> I think pure, free-functions (in an anonymous namespace) are easier to reason about.
I could do that in a separate patch if necessary.


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:605
     // Mark the given argument.
-    assert(ArgNum < CE->getNumArgs());
     State = State->add<TaintArgsOnPostVisit>(ArgNum);
----------------
Szelethus wrote:
> I get that there isn't much substance to this assert, but why remove it? We might as well populate the lines in between that and the branch.
I think there is no need for this. Maybe I can make the ArgNum const.


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

https://reviews.llvm.org/D59637