Charusso updated this revision to Diff 233905. Charusso marked 6 inline comments as done. Charusso added a comment.
- Try to conjure the index of the 'ElementRegion'. 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/CStringChecker.cpp clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp clang/test/Analysis/cert/str30-c-explain.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 'pathname'}} + + 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/test/Analysis/cert/str30-c-explain.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str30-c-explain.cpp @@ -0,0 +1,25 @@ +// RUN: %clang_analyze_cc1 \ +// RUN: -analyzer-checker=core,unix,alpha.security.cert.str.30c \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: %s 2>&1 | FileCheck %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" + +char *strrchr(const char *str, int c); + +void clang_analyzer_explain(char *); +void clang_analyzer_dump(char *); + +void dump(const char *pathname) { + char *slash; + slash = strrchr(pathname, '/'); + + clang_analyzer_dump(slash); + // CHECK: &Element{pathname,conj_$1{long long, LC1, S{{[0-9]+}}, #1},char} + + clang_analyzer_explain(slash); + // CHECK: pointer to element of type 'char' with index 'symbol of type 'long long' conjured at statement 'pathname'' of parameter 'pathname' +} 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; @@ -90,10 +105,26 @@ std::unique_ptr<BugType> BT; 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}}}, @@ -116,6 +147,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. //===----------------------------------------------------------------------===// @@ -272,6 +306,82 @@ // 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(); + std::string ArgName = "first argument"; + if (const auto *MR = Call.getReturnValue().getAsRegion()) + if (const auto *ER = dyn_cast<ElementRegion>(MR)) + if (const auto *VR = dyn_cast<VarRegion>(ER->getBaseRegion())) + 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(); + }, + /*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) @@ -454,8 +564,45 @@ return true; } +static bool checkConstModify(SVal L, SVal V, const Stmt *S, CheckerContext &C, + 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; @@ -474,15 +621,16 @@ } void StrCheckerBase::checkPostStmt(const DeclStmt *S, CheckerContext &C) const { - if (const auto *VD = cast<VarDecl>(S->getSingleDecl())) { - if (VD->getType()->getAsArrayTypeUnsafe()) { - VD->dumpColor(); - ProgramStateRef State = C.getState(); + if (EnableStr32cChecker) { + if (const auto *VD = cast<VarDecl>(S->getSingleDecl())) { + if (VD->getType()->getAsArrayTypeUnsafe()) { + ProgramStateRef State = C.getState(); - const VarRegion *VR = State->getRegion(VD, C.getLocationContext()); + const VarRegion *VR = State->getRegion(VD, C.getLocationContext()); - std::string ArrayName = VD->getNameAsString(); - createWrongSizeReport(VR, ArrayName, /*IsAlloc=*/false, State, C); + std::string ArrayName = VD->getNameAsString(); + createWrongSizeReport(VR, ArrayName, /*IsAlloc=*/false, State, C); + } } } } @@ -508,5 +656,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,104 @@ C.addTransition(State); } +void CStringChecker::evalCharPtrCommon(CheckerContext &C, + const CallExpr *CE) const { + CurrentFunctionDescription = "char pointer returning function"; + + SValBuilder &SVB = C.getSValBuilder(); + ProgramStateRef State, StateNull; + std::tie(StateNull, State) = C.getState()->assume(SVB.makeNull()); + + const LocationContext *LCtx = C.getLocationContext(); + if (StateNull) { + StateNull = StateNull->BindExpr(CE, LCtx, SVB.makeNull()); + C.addTransition(StateNull); + } + + if (!State) + return; + + // Check whether we already made the 'ElementRegion'. + const Expr *SrcExpr = CE->getArg(0); + SVal SrcV = C.getSVal(SrcExpr); + if (const auto *MR = SrcV.getAsRegion()) { + if (isa<ElementRegion>(MR) && isa<VarRegion>(MR->getBaseRegion())) { + State = State->BindExpr(CE, LCtx, SrcV); + C.addTransition(State); + return; + } + } + + // Check for a 'StringRegion'. + if (const auto *SL = dyn_cast<StringLiteral>(SrcExpr->IgnoreImpCasts())) { + const StringRegion *ResultMR = + C.getStoreManager().getRegionManager().getStringRegion(SL); + SVal ResultV = loc::MemRegionVal(ResultMR); + State = State->BindExpr(CE, LCtx, ResultV); + C.addTransition(State); + return; + } + + // Try to create an 'ElementRegion' to the string's 'VarRegion' with a + // conjured index. + const VarRegion *StrVR = nullptr; + bool IsArgument = false; + if (const auto *DRE = dyn_cast<DeclRefExpr>(SrcExpr->IgnoreImpCasts())) { + if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) { + StrVR = State->getRegion(VD, LCtx); + IsArgument = isa<ParmVarDecl>(VD) || isa<ImplicitParamDecl>(VD); + } + } + + if (!StrVR) + return; + + DefinedOrUnknownSVal Index = SVB.conjureSymbolVal( + SrcExpr, LCtx, SVB.getArrayIndexType(), C.blockCount()); + + QualType Ty = SrcExpr->getType(); + if (Ty->isPointerType()) + Ty = Ty->getPointeeType(); + + const ElementRegion *ER = SVB.getRegionManager().getElementRegion( + Ty, Index.castAs<NonLoc>(), cast<SubRegion>(StrVR), C.getASTContext()); + + SVal ResultV = loc::MemRegionVal(ER); + 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 @@ -807,6 +807,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 @@ -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-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
