Charusso updated this revision to Diff 242159. Charusso marked 8 inline comments as done. Charusso added a comment.
- Remove the modeling from CStringChecker. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71155/new/ https://reviews.llvm.org/D71155 Files: clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp clang/test/Analysis/cert/str30-c-notes.cpp clang/test/Analysis/cert/str30-c.cpp
Index: clang/test/Analysis/cert/str30-c.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str30-c.cpp @@ -0,0 +1,59 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,unix,alpha.security.cert.str.30c \ +// RUN: -verify %s + +// See the examples on the page of STR30-C: +// https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals + +#include "../Inputs/system-header-simulator.h" + +#define EOF -1 +typedef __SIZE_TYPE__ size_t; +typedef __typeof__((char *)0 - (char *)0) ptrdiff_t; + +void free(void *memblock); +void *malloc(size_t size); + +char *strrchr(const char *str, int c); +int puts(const char *str); + +namespace test_strrchr_bad { +const char *get_dirname(const char *pathname) { + char *slash; + slash = strrchr(pathname, '/'); + if (slash) { + *slash = '\0'; /* Undefined behavior */ + // expected-warning@-1 {{'slash' is pointing to a constant string}} + } + return pathname; +} + +int main(void) { + puts(get_dirname(__FILE__)); + return 0; +} +} // namespace test_strrchr_bad + +namespace test_strrchr_good { +char *get_dirname(const char *pathname, char *dirname, size_t size) { + const char *slash; + slash = strrchr(pathname, '/'); + if (slash) { + ptrdiff_t slash_idx = slash - pathname; + if ((size_t)slash_idx < size) { + memcpy(dirname, pathname, slash_idx); + dirname[slash_idx] = '\0'; + return dirname; + } + } + return 0; +} + +int main(void) { + char dirname[260]; + if (get_dirname(__FILE__, dirname, sizeof(dirname))) { + puts(dirname); + } + return 0; +} +} // namespace test_strrchr_good Index: clang/test/Analysis/cert/str30-c-notes.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str30-c-notes.cpp @@ -0,0 +1,44 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,unix,alpha.security.cert.str.30c \ +// RUN: -analyzer-output=text -verify %s + +// See the examples on the page of STR30-C: +// https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals + +#include "../Inputs/system-header-simulator.h" + +#define EOF -1 +typedef __SIZE_TYPE__ size_t; + +void free(void *memblock); +void *malloc(size_t size); + +char *strrchr(const char *str, int c); +int puts(const char *str); + +namespace test_strrchr_bad { +const char *get_dirname(const char *pathname) { + char *slash; + slash = strrchr(pathname, '/'); + // expected-note@-1 {{'strrchr' returns a pointer to the constant 'pathname'}} + + slash = strrchr(slash, '/'); + // expected-note@-1 {{'strrchr' returns a pointer to the constant 'slash'}} + + if (slash) { + // expected-note@-1 {{'slash' is non-null}} + // expected-note@-2 {{Taking true branch}} + + *slash = '\0'; /* Undefined behavior */ + // expected-note@-1 {{'slash' is pointing to a constant string}} + // expected-warning@-2 {{'slash' is pointing to a constant string}} + } + return pathname; +} + +int main(void) { + puts(get_dirname(__FILE__)); + // expected-note@-1 {{Calling 'get_dirname'}} + return 0; +} +} // namespace test_strrchr_bad Index: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp @@ -12,6 +12,9 @@ // https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152038 // // This checker is a base checker which consist of the following checkers: +// - '30c' +// https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals +// // - '31c' // https://wiki.sei.cmu.edu/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator // @@ -54,9 +57,6 @@ class StrCheckerBase : public Checker<check::PostCall, check::Bind, check::PostStmt<DeclStmt>> { - using StrCheck = std::function<void(const StrCheckerBase *, const CallEvent &, - const CallContext &, CheckerContext &)>; - public: // We report a note when any of the calls in 'CDM' is being used because // they can cause a not null-terminated string. In this case we store the @@ -73,6 +73,19 @@ void checkPostStmt(const DeclStmt *S, CheckerContext &C) const; + // STR30-C. + void checkMemchr(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; + void checkStrchr(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; + void checkStrrchr(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; + void checkStrstr(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; + void checkStrpbrk(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; + + // STR31-C. void checkGets(const CallEvent &Call, const CallContext &CallC, CheckerContext &C) const; void checkSprintf(const CallEvent &Call, const CallContext &CallC, @@ -81,6 +94,8 @@ CheckerContext &C) const; void checkStrcpy(const CallEvent &Call, const CallContext &CallC, CheckerContext &C) const; + + // STR32-C. void checkStrncpy(const CallEvent &Call, const CallContext &CallC, CheckerContext &C) const; @@ -88,10 +103,26 @@ CheckerContext &C) const; bool WarnOnCall; - bool EnableStr31cChecker = false, EnableStr32cChecker = false; + bool EnableStr30cChecker = false, EnableStr31cChecker = false, + EnableStr32cChecker = false; private: + using StrCheck = std::function<void(const StrCheckerBase *, const CallEvent &, + const CallContext &, CheckerContext &)>; + const CallDescriptionMap<std::pair<StrCheck, CallContext>> CDM = { + // The following checks STR30-C rules. + // void *memchr(const void *buffer, int c, size_t count); + {{"memchr", 3}, {&StrCheckerBase::checkMemchr, {None, 0}}}, + // char *strchr(const char *str, int c); + {{"strchr", 2}, {&StrCheckerBase::checkStrchr, {None, 0}}}, + // char *strrchr(const char *str, int c); + {{"strrchr", 2}, {&StrCheckerBase::checkStrrchr, {None, 0}}}, + // char *strstr(const char *str, const char *strSearch); + {{"strstr", 2}, {&StrCheckerBase::checkStrstr, {None, 0}}}, + // char *strpbrk(const char *str, const char *strCharSet); + {{"strpbrk", 2}, {&StrCheckerBase::checkStrpbrk, {None, 0}}}, + // The following checks STR31-C rules. // char *gets(char *dest); {{"gets", 1}, {&StrCheckerBase::checkGets, {0}}}, @@ -117,6 +148,9 @@ // It stores the allocations which we emit notes on. REGISTER_LIST_WITH_PROGRAMSTATE(AllocationList, const MemRegion *) +// It stores the strings which we have pointers to them. +REGISTER_LIST_WITH_PROGRAMSTATE(PointerList, const MemRegion *) + //===----------------------------------------------------------------------===// // Helper functions. //===----------------------------------------------------------------------===// @@ -274,6 +308,83 @@ // Evaluating problematic function calls. //===----------------------------------------------------------------------===// +static void checkCharPtr(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) { + ProgramStateRef State = C.getState(); + SVal ReturnV = Call.getReturnValue(); + const MemRegion *MR = getRegion(ReturnV); + if (!MR) + return; + + // Check for constant source string. + unsigned SourcePos = *CallC.SourcePos; + const Expr *SrcArg = Call.getArgExpr(SourcePos); + QualType Ty = SrcArg->getType(); + if (Ty->isPointerType()) + Ty = Ty->getPointeeType(); + if (!Ty.isConstQualified()) + return; + + std::string CallName = Call.getCalleeIdentifier()->getName().str(); + std::string ArgName = "first argument"; + + if (const auto *DRE = dyn_cast<DeclRefExpr>(SrcArg->IgnoreImpCasts())) + if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) + if (const auto *VR = State->getRegion(VD, C.getLocationContext())) + ArgName = '\'' + VR->getDecl()->getNameAsString() + '\''; + + const NoteTag *Tag = C.getNoteTag( + [=]() -> std::string { + SmallString<128> Msg; + llvm::raw_svector_ostream Out(Msg); + Out << '\'' << CallName << "' returns a pointer to the constant " + << ArgName; + + return Out.str().str(); + }, + /*IsPrunable=*/false); + + State = State->add<PointerList>(MR); + C.addTransition(State, C.getPredecessor(), Tag); +} + +void StrCheckerBase::checkMemchr(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr30cChecker) + if (const MemRegion *MR = Call.getArgSVal(0).getAsRegion()) + if (const auto *ER = MR->getAs<ElementRegion>()) + if (ER->getValueType()->isAnyCharacterType()) + checkCharPtr(Call, CallC, C); +} + +void StrCheckerBase::checkStrchr(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr30cChecker) + checkCharPtr(Call, CallC, C); +} +void StrCheckerBase::checkStrrchr(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr30cChecker) + checkCharPtr(Call, CallC, C); +} + +void StrCheckerBase::checkStrstr(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr30cChecker) + checkCharPtr(Call, CallC, C); +} + +void StrCheckerBase::checkStrpbrk(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) const { + if (EnableStr30cChecker) + checkCharPtr(Call, CallC, C); +} + void StrCheckerBase::checkGets(const CallEvent &Call, const CallContext &CallC, CheckerContext &C) const { if (EnableStr31cChecker) @@ -392,7 +503,7 @@ } static bool checkAllocation(SVal L, SVal V, const Stmt *S, CheckerContext &C, - BugType BT) { + const BugType &BT) { const MemRegion *LocMR = getRegion(L); const MemRegion *NonLocMR = getRegion(V); if (!LocMR || !NonLocMR) @@ -456,8 +567,45 @@ return true; } +static bool checkConstModify(SVal L, SVal V, const Stmt *S, CheckerContext &C, + const BugType &BT) { + // Check for simple bindings. + const auto *BO = dyn_cast<BinaryOperator>(S); + if (!BO) + return false; + + const MemRegion *MR = getRegion(L); + if (!MR) + return false; + + if (!C.getState()->contains<PointerList>(MR)) + return false; + + SmallString<128> Msg; + llvm::raw_svector_ostream Out(Msg); + + std::string Name = "the dereferenced pointer"; + if (Optional<std::string> NameTmp = getName(BO->getLHS(), C)) + Name = *NameTmp; + + Out << Name << " is pointing to a constant string"; + + auto Report = std::make_unique<PathSensitiveBugReport>(BT, Out.str(), + C.generateErrorNode()); + + // Track the allocation. + bugreporter::trackExpressionValue(C.getPredecessor(), BO->getLHS(), *Report); + + C.emitReport(std::move(Report)); + return true; +} + void StrCheckerBase::checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const { + if (EnableStr30cChecker) + if (checkConstModify(L, V, S, C, BT)) + return; + if (EnableStr32cChecker) if (checkAllocation(L, V, S, C, BT)) return; @@ -505,5 +653,6 @@ \ bool ento::shouldRegister##Name(const LangOptions &LO) { return true; } +REGISTER_CHECKER(Str30cChecker) REGISTER_CHECKER(Str31cChecker) REGISTER_CHECKER(Str32cChecker) Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -837,6 +837,11 @@ Documentation<HasDocumentation>, Hidden; +def Str30cChecker : Checker<"30c">, + HelpText<"SEI CERT checker of rules defined in STR30-C">, + Dependencies<[StrCheckerBase]>, + Documentation<HasDocumentation>; + def Str31cChecker : Checker<"31c">, HelpText<"SEI CERT checker of rules defined in STR31-C">, Dependencies<[StrCheckerBase]>, Index: clang/docs/analyzer/checkers.rst =================================================================== --- clang/docs/analyzer/checkers.rst +++ clang/docs/analyzer/checkers.rst @@ -1930,6 +1930,16 @@ alpha.security ^^^^^^^^^^^^^^ +.. _alpha-security-cert-str-30c: + +alpha.security.cert.str.30c +""""""""""""""""""""""""""" + +SEI CERT checker of `STR30-C rule<https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals>`_. + +It warns on misusing the following functions: ``strpbrk()``, ``strchr()``, +``strrchr()``, ``strstr()``, ``memchr()``. + .. _alpha-security-cert-str-31c: alpha.security.cert.str.31c
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
