Charusso created this revision. Charusso added a reviewer: NoQ. Charusso added a project: clang. Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. Charusso added a comment. Charusso added a parent revision: D71033: [analyzer] CERT: StrChecker: 32.c.
Examples generated by CodeChecker from the `curl` project: F10986729: str30-c.tar.gz <https://reviews.llvm.org/F10986729> This checker is implemented based on the following 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()`. Repository: rC Clang https://reviews.llvm.org/D71155 Files: clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp 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.30.c \ +// 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.30.c \ +// 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 'pathname'}} + + if (slash) { + // expected-note@-1 {{Assuming '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: +// - '30.c' +// https://wiki.sei.cmu.edu/confluence/display/c/STR30-C.+Do+not+attempt+to+modify+string+literals +// // - '31.c' // 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 // @@ -53,9 +56,6 @@ }; class StrCheckerBase : public Checker<check::PostCall, check::Bind> { - 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 @@ -70,23 +70,54 @@ // store the string is being null-terminated in the 'NullTerminationMap'. void checkBind(SVal L, SVal V, const Stmt *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; + CheckerContext &C) const; void checkSprintf(const CallEvent &Call, const CallContext &CallC, - CheckerContext &C) const; - void checkFscanf(const CallEvent &Call, const CallContext &CallC, CheckerContext &C) const; + void checkFscanf(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; void checkStrcpy(const CallEvent &Call, const CallContext &CallC, - CheckerContext &C) const; + CheckerContext &C) const; + + // STR32-C. void checkStrncpy(const CallEvent &Call, const CallContext &CallC, - CheckerContext &C) const; + CheckerContext &C) const; std::unique_ptr<BugType> BT; - 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}}}, @@ -109,6 +140,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. //===----------------------------------------------------------------------===// @@ -225,8 +259,84 @@ // 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. + const Expr *SrcArg = Call.getArgExpr(*CallC.SourcePos); + QualType Ty = SrcArg->getType(); + if (Ty->isPointerType()) + Ty = Ty->getPointeeType(); + if (!Ty.isConstQualified()) + return; + + std::string CallName = Call.getCalleeIdentifier()->getName(); + std::string ArgName = "first argument"; + if (const auto *SR = MR->getAs<SymbolicRegion>()) { + if (const auto *SC = dyn_cast<SymbolConjured>(SR->getSymbol())) { + if (Optional<std::string> ArgNameTmp = + getName(cast<CallExpr>(SC->getStmt())->getArg(0), C)) { + ArgName = *ArgNameTmp; + } + } + } + + 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(); + }, + /*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 { + CheckerContext &C) const { if (EnableStr31cChecker) createOverflowNote(Call, CallC, C); } @@ -387,8 +497,45 @@ return true; } +static bool checkConstModify(SVal L, SVal V, const Stmt *S, CheckerContext &C, + BugType &BT) { + const MemRegion *MR = getRegion(L); + if (!MR) + return false; + + if (!C.getState()->contains<PointerList>(MR)) + return false; + + // Check for simple bindings. + const auto *BO = dyn_cast<BinaryOperator>(S); + if (!BO) + 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; @@ -426,5 +573,6 @@ \ bool ento::shouldRegister##Name(const LangOptions &LO) { return true; } +REGISTER_CHECKER(Str30cChecker) REGISTER_CHECKER(Str31cChecker) REGISTER_CHECKER(Str32cChecker) Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -79,6 +79,7 @@ {{CDF_MaybeBuiltin, "mempcpy", 3}, &CStringChecker::evalMempcpy}, {{CDF_MaybeBuiltin, "memcmp", 3}, &CStringChecker::evalMemcmp}, {{CDF_MaybeBuiltin, "memmove", 3}, &CStringChecker::evalMemmove}, + {{CDF_MaybeBuiltin, "memchr", 3}, &CStringChecker::evalMemchr}, {{CDF_MaybeBuiltin, "memset", 3}, &CStringChecker::evalMemset}, {{CDF_MaybeBuiltin, "explicit_memset", 3}, &CStringChecker::evalMemset}, {{CDF_MaybeBuiltin, "strcpy", 2}, &CStringChecker::evalStrcpy}, @@ -95,6 +96,10 @@ {{CDF_MaybeBuiltin, "strcasecmp", 2}, &CStringChecker::evalStrcasecmp}, {{CDF_MaybeBuiltin, "strncasecmp", 3}, &CStringChecker::evalStrncasecmp}, {{CDF_MaybeBuiltin, "strsep", 2}, &CStringChecker::evalStrsep}, + {{CDF_MaybeBuiltin, "strpbrk", 2}, &CStringChecker::evalStrpbrk}, + {{CDF_MaybeBuiltin, "strchr", 2}, &CStringChecker::evalStrchr}, + {{CDF_MaybeBuiltin, "strrchr", 2}, &CStringChecker::evalStrrchr}, + {{CDF_MaybeBuiltin, "strstr", 2}, &CStringChecker::evalStrstr}, {{CDF_MaybeBuiltin, "bcopy", 3}, &CStringChecker::evalBcopy}, {{CDF_MaybeBuiltin, "bcmp", 3}, &CStringChecker::evalMemcmp}, {{CDF_MaybeBuiltin, "bzero", 2}, &CStringChecker::evalBzero}, @@ -152,6 +157,13 @@ void evalStrsep(CheckerContext &C, const CallExpr *CE) const; + void evalCharPtrCommon(CheckerContext &C, const CallExpr *CE) const; + void evalMemchr(CheckerContext &C, const CallExpr *CE) const; + void evalStrchr(CheckerContext &C, const CallExpr *CE) const; + void evalStrrchr(CheckerContext &C, const CallExpr *CE) const; + void evalStrstr(CheckerContext &C, const CallExpr *CE) const; + void evalStrpbrk(CheckerContext &C, const CallExpr *CE) const; + void evalStdCopy(CheckerContext &C, const CallExpr *CE) const; void evalStdCopyBackward(CheckerContext &C, const CallExpr *CE) const; void evalStdCopyCommon(CheckerContext &C, const CallExpr *CE) const; @@ -2082,6 +2094,70 @@ C.addTransition(State); } +void CStringChecker::evalCharPtrCommon(CheckerContext &C, + const CallExpr *CE) const { + CurrentFunctionDescription = "char pointer returning function"; + + ProgramStateRef State = C.getState(); + SValBuilder &SVB = C.getSValBuilder(); + ASTContext &Ctx = C.getASTContext(); + const LocationContext *LCtx = C.getLocationContext(); + + // Check whether we already conjured a symbol for the pointer. + SVal SrcV = C.getSVal(CE->getArg(0)); + if (const MemRegion *MR = SrcV.getAsRegion()) { + if (const auto *SR = MR->getBaseRegion()->getAs<SymbolicRegion>()) { + State = State->BindExpr(CE, LCtx, SrcV); + C.addTransition(State); + return; + } + } + + // Conjure a symbol to represent the pointer. + NonLoc RandomIdx = SVB.makeArrayIndex(0); + QualType Ty = CE->getArg(0)->getType(); + if (Ty->isPointerType()) + Ty = Ty->getPointeeType(); + + SVal ConjuredV = SVB.getConjuredHeapSymbolVal(CE, LCtx, C.blockCount()); + SVal ResultV = loc::MemRegionVal(SVB.getRegionManager().getElementRegion( + Ty, RandomIdx, cast<SubRegion>(ConjuredV.getAsRegion()), Ctx)); + + State = State->BindExpr(CE, LCtx, ResultV); + C.addTransition(State); +} + +void CStringChecker::evalMemchr(CheckerContext &C, const CallExpr *CE) const { + // void *memchr(const void *buffer, int c, size_t count); + const Expr *BufferExpr = CE->getArg(0); + QualType Ty = BufferExpr->getType().getUnqualifiedType(); + if (Ty->isPointerType()) + Ty = Ty->getPointeeType(); + + if (Ty->isAnyCharacterType()) + evalCharPtrCommon(C, CE); +} + +void CStringChecker::evalStrchr(CheckerContext &C, const CallExpr *CE) const { + // char *strchr(const char *str, int c); + evalCharPtrCommon(C, CE); +} + +void CStringChecker::evalStrrchr(CheckerContext &C, const CallExpr *CE) const { + // char *strrchr(const char *str, int c); + evalCharPtrCommon(C, CE); +} + +void CStringChecker::evalStrstr(CheckerContext &C, const CallExpr *CE) const { + // char *strstr(const char *str, const char *strSearch); + evalCharPtrCommon(C, CE); +} + +void CStringChecker::evalStrpbrk(CheckerContext &C, const CallExpr *CE) const { + // char *strpbrk(const char *str, const char *strCharSet); + evalCharPtrCommon(C, CE); +} + // These should probably be moved into a C++ standard library checker. void CStringChecker::evalStdCopy(CheckerContext &C, const CallExpr *CE) const { evalStdCopyCommon(C, CE); Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -795,6 +795,11 @@ Documentation<HasDocumentation>, Hidden; +def Str30cChecker : Checker<"30.c">, + HelpText<"SEI CERT checker of rules defined in STR30-C">, + Dependencies<[StrCheckerBase]>, + Documentation<HasDocumentation>; + def Str31cChecker : Checker<"31.c">, 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 @@ -1905,6 +1905,16 @@ SEI CERT checkers of STR `C coding rules<https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152038>`_. +.. _alpha-security-cert-str-30-c: + +alpha.security.cert.str.30.c +"""""""""""""""""""""""""""" + +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-31-c: alpha.security.cert.str.31.c
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
