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