Charusso updated this revision to Diff 228675. Charusso added a comment. - The packaging have not been addressed yet. - Inject the "zombie" size expression to the new function call (`fgets`) if none of the size expression's regions have been modified.
The idea is that: When we set up a variable `size = 13;` it modifies the region, but the size expression is not stored yet, so we do not invalidate anything. We store the `malloc(size + 1)`'s `size`, after that the dead-symbol-purging kick in and it either invalidate the region or makes it keep alive. - If the region of `size` is alive after the purge point we cannot inject the "zombie" `size + 1` as an expression, we need to obtain its concrete value: `14`. (When the redefinition happen I wanted to create a `NoteTag`, but I have not seen a simple way to do so.) - If the region of `size` has been purged out, it is safe to copy-and-paste the "zombie" `size + 1` as an expression. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69813/new/ https://reviews.llvm.org/D69813 Files: clang/include/clang/Lex/Preprocessor.h clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSizeInfo.h clang/lib/StaticAnalyzer/Checkers/AllocationState.h clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp clang/lib/StaticAnalyzer/Core/DynamicSize.cpp clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp clang/test/Analysis/Inputs/system-header-simulator.h clang/test/Analysis/analyzer-config.c clang/test/Analysis/cert/str31-alloc.cpp clang/test/Analysis/cert/str31-notes.cpp clang/test/Analysis/cert/str31-safe.cpp clang/test/Analysis/cert/str31-unsafe.cpp
Index: clang/test/Analysis/cert/str31-unsafe.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str31-unsafe.cpp @@ -0,0 +1,34 @@ +// RUN: %check_analyzer_fixit %s %t \ +// RUN: -analyzer-checker=core,unix,security.cert.Str \ +// RUN: -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \ +// RUN: -I %S + +// See the examples on the page of STR31: +// 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 + +#include "../Inputs/system-header-simulator.h" + +// The following is not defined therefore the safe functions are unavailable. +// #define __STDC_LIB_EXT1__ 1 +#define __STDC_WANT_LIB_EXT1__ 1 + +namespace test_gets_bad { +#define BUFFER_SIZE 1024 + +void func(void) { + char buf[BUFFER_SIZE]; + if (gets(buf)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf'}} + // CHECK-FIXES: if (fgets(buf, sizeof(buf), stdin)) {} +} +} // namespace test_gets_bad + +namespace test_gets_good { +enum { BUFFERSIZE = 32 }; + +void func(void) { + char buff[BUFFERSIZE]; + + if (fgets(buff, sizeof(buff), stdin)) {} +} +} // namespace test_gets_good Index: clang/test/Analysis/cert/str31-safe.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str31-safe.cpp @@ -0,0 +1,33 @@ +// RUN: %check_analyzer_fixit %s %t \ +// RUN: -analyzer-checker=core,unix,security.cert.Str \ +// RUN: -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \ +// RUN: -I %S + +// See the examples on the page of STR31: +// 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 + +#include "../Inputs/system-header-simulator.h" + +#define __STDC_LIB_EXT1__ 1 +#define __STDC_WANT_LIB_EXT1__ 1 + +namespace test_gets_bad { +#define BUFFER_SIZE 1024 + +void func(void) { + char buf[BUFFER_SIZE]; + if (gets(buf)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf'}} + // CHECK-FIXES: if (gets_s(buf, sizeof(buf))) {} +} +} // namespace test_gets_bad + +namespace test_gets_good { +enum { BUFFERSIZE = 32 }; + +void func(void) { + char buff[BUFFERSIZE]; + + if (gets_s(buff, sizeof(buff))) {} +} +} // namespace test_gets_good Index: clang/test/Analysis/cert/str31-notes.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str31-notes.cpp @@ -0,0 +1,48 @@ +// RUN: %check_analyzer_fixit %s %t \ +// RUN: -analyzer-checker=core,unix,security.cert.Str \ +// RUN: -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \ +// RUN: -analyzer-output=text -I %S + +// See the examples on the page of STR31: +// 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 + +#include "../Inputs/system-header-simulator.h" + +// The following is not defined therefore the safe functions are unavailable. +// #define __STDC_LIB_EXT1__ 1 +#define __STDC_WANT_LIB_EXT1__ 1 + +void *malloc(size_t size); +void free(void *memblock); + +void test_simple_size(unsigned size) { + size = 13; + // expected-note@-1 {{The value 13 is assigned to 'size'}} + + char *buf = (char *)malloc(size); + // expected-note@-1 {{Memory is allocated}} + + if (gets(buf)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf'}} + // expected-note@-2 {{'gets' could write outside of 'buf'}} + + // CHECK-FIXES: if (fgets(buf, size, stdin)) {} + free(buf); +} + +void test_size_redefinition() { + unsigned size = 13; + // expected-note@-1 {{'size' initialized to 13}} + + char *buf = (char *)malloc(size + 1); + // expected-note@-1 {{Memory is allocated}} + + size = 42; + + if (gets(buf)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf'}} + // expected-note@-2 {{'gets' could write outside of 'buf'}} + + // CHECK-FIXES: if (fgets(buf, 14, stdin)) {} + free(buf); +} Index: clang/test/Analysis/cert/str31-alloc.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/cert/str31-alloc.cpp @@ -0,0 +1,89 @@ +// RUN: %check_analyzer_fixit %s %t \ +// RUN: -analyzer-checker=core,unix,security.cert.Str \ +// RUN: -analyzer-config security.cert.Str:WantToUseSafeFunctions=true \ +// RUN: -I %S + +// See the examples on the page of STR31: +// 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 + +#include "../Inputs/system-header-simulator.h" + +void *malloc(size_t size); +void free(void *memblock); +#define alloca(size) __builtin_alloca(size) + +#define __STDC_LIB_EXT1__ 1 +#define __STDC_WANT_LIB_EXT1__ 1 + +void test_ambiguous_parameter(char *buf) { + if (gets(buf)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf'}} + // CHECK-FIXES: if (gets(buf)) {} +} + +// We cannot be sure why the offset set and the size would be different based +// on the offset and its meaning. +void test_offset() { + char buff[13]; + if (gets(buff + 1)) {} + // expected-warning@-1 {{'gets' could write outside of 'buff'}} + // CHECK-FIXES: if (gets(buff + 1)) {} +} + +void test_malloc_known(size_t size) { + char *buf1 = (char *)malloc(size); + if (gets(buf1)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf1'}} + // CHECK-FIXES: if (gets_s(buf1, size)) {} + free(buf1); +} + +void test_variable_array_ty(size_t size) { + char buf2[size]; + if (gets(buf2)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf2'}} + // CHECK-FIXES: if (gets_s(buf2, sizeof(buf2))) {} +} + +void test_constant_array_ty(size_t size) { + char buf3[13]; + if (gets(buf3)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf3'}} + // CHECK-FIXES: if (gets_s(buf3, sizeof(buf3))) {} +} + +template <typename T, size_t size> +struct MyArray { + T data[size]; + + MyArray() { test_dependent_sized_array_ty(); } + + void test_dependent_sized_array_ty() { + if (gets(data)) {} + // expected-warning@-1 {{'gets' could write outside of 'data'}} + // CHECK-FIXES: if (gets_s(data, sizeof(data))) {} + } +}; + +void test_member() { + MyArray<char, 13> buf4; + if (gets(buf4.data)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf4.data'}} + // CHECK-FIXES: if (gets_s(buf4.data, sizeof(buf4.data))) {} +} + +void test_new(size_t size) { + char *buf5; + buf5 = new char[13]; + if (gets(buf5)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf5'}} + // CHECK-FIXES: if (gets_s(buf5, 13)) {} + delete[] buf5; +} + +void test_alloca() { + char *buf6 = (char *)alloca(13); + if (gets(buf6)) {} + // expected-warning@-1 {{'gets' could write outside of 'buf6'}} + // CHECK-FIXES: if (gets_s(buf6, 13)) {} +} Index: clang/test/Analysis/analyzer-config.c =================================================================== --- clang/test/Analysis/analyzer-config.c +++ clang/test/Analysis/analyzer-config.c @@ -86,6 +86,7 @@ // CHECK-NEXT: prune-paths = true // CHECK-NEXT: region-store-small-struct-limit = 2 // CHECK-NEXT: report-in-main-source-file = false +// CHECK-NEXT: security.cert.Str:WantToUseSafeFunctions = true // CHECK-NEXT: serialize-stats = false // CHECK-NEXT: silence-checkers = "" // CHECK-NEXT: stable-report-filename = false @@ -98,4 +99,4 @@ // CHECK-NEXT: unroll-loops = false // CHECK-NEXT: widen-loops = false // CHECK-NEXT: [stats] -// CHECK-NEXT: num-entries = 95 +// CHECK-NEXT: num-entries = 96 Index: clang/test/Analysis/Inputs/system-header-simulator.h =================================================================== --- clang/test/Analysis/Inputs/system-header-simulator.h +++ clang/test/Analysis/Inputs/system-header-simulator.h @@ -18,11 +18,16 @@ extern FILE *__stdoutp; extern FILE *__stderrp; +typedef __SIZE_TYPE__ size_t; + int scanf(const char *restrict format, ...); int fscanf(FILE *restrict, const char *restrict, ...); int printf(const char *restrict format, ...); int fprintf(FILE *restrict, const char *restrict, ...); int getchar(void); +char *gets(char *buffer); +char *gets_s(char *buffer, size_t size); +char *fgets(char *str, int n, FILE *stream); // Note, on some platforms errno macro gets replaced with a function call. extern int errno; @@ -112,4 +117,4 @@ #define NULL __DARWIN_NULL #endif -#define offsetof(t, d) __builtin_offsetof(t, d) \ No newline at end of file +#define offsetof(t, d) __builtin_offsetof(t, d) Index: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp =================================================================== --- clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp +++ clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp @@ -161,9 +161,10 @@ E = Diags.end(); I != E; ++I) { const PathDiagnostic *PD = *I; + const auto WarningPiece = PD->path.back(); reportPiece(WarnID, PD->getLocation().asLocation(), - PD->getShortDescription(), PD->path.back()->getRanges(), - PD->path.back()->getFixits()); + PD->getShortDescription(), WarningPiece->getRanges(), + WarningPiece->getFixits()); // First, add extra notes, even if paths should not be included. for (const auto &Piece : PD->path) { @@ -183,8 +184,16 @@ if (isa<PathDiagnosticNotePiece>(Piece.get())) continue; + // Do not apply fix-its twice on the warning path piece. + bool IsFixItDuplication = WarningPiece == Piece && ApplyFixIts; + if (IsFixItDuplication) + ApplyFixIts = false; + reportPiece(NoteID, Piece->getLocation().asLocation(), Piece->getString(), Piece->getRanges(), Piece->getFixits()); + + if (IsFixItDuplication) + ApplyFixIts = true; } } Index: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/DynamicSize.cpp +++ clang/lib/StaticAnalyzer/Core/DynamicSize.cpp @@ -12,7 +12,9 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h" #include "clang/AST/Expr.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Basic/LLVM.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSizeInfo.h" #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" @@ -26,6 +28,13 @@ namespace clang { namespace ento { +using namespace ast_matchers; + +const DynamicSizeInfo *getDynamicSizeInfo(ProgramStateRef State, + const MemRegion *MR) { + return State->get<DynamicSizeMap>(MR); +} + ProgramStateRef setDynamicSize(ProgramStateRef State, const MemRegion *MR, DefinedOrUnknownSVal Size, const Expr *SizeExpr) { @@ -64,5 +73,37 @@ return DivisionV.castAs<DefinedOrUnknownSVal>(); } +ProgramStateRef +invalidateDynamicSizeExpr(ProgramStateRef State, + ArrayRef<const MemRegion *> ExplicitRegions, + const LocationContext *LCtx) { + constexpr llvm::StringLiteral ExprName = "expr"; + for (const auto &Elem : State->get<DynamicSizeMap>()) { + const DynamicSizeInfo &SizeInfo = Elem.second; + if (!SizeInfo.isValid()) + continue; + + const Expr *SizeExpr = SizeInfo.getSizeExpr(); + if (!SizeExpr) + continue; + + auto Matches = + match(expr(forEachDescendant(expr().bind(ExprName))), *SizeExpr, + State->getAnalysisManager().getASTContext()); + + for (const BoundNodes &Match : Matches) + if (const Expr *E = Match.getNodeAs<Expr>(ExprName)) + if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) + if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) + for (const MemRegion *MR : ExplicitRegions) + if (State->getRegion(VD, LCtx) == MR) + State = State->set<DynamicSizeMap>( + Elem.first, {SizeInfo.getSize(), SizeInfo.getSizeExpr(), + /*IsValid=*/false}); + } + + return State; +} + } // namespace ento } // namespace clang Index: clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp +++ clang/lib/StaticAnalyzer/Core/CommonBugCategories.cpp @@ -9,13 +9,19 @@ #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h" // Common strings used for the "category" of many static analyzer issues. -namespace clang { namespace ento { namespace categories { +namespace clang { +namespace ento { +namespace categories { -const char * const CoreFoundationObjectiveC = "Core Foundation/Objective-C"; -const char * const LogicError = "Logic error"; -const char * const MemoryRefCount = - "Memory (Core Foundation/Objective-C/OSObject)"; -const char * const MemoryError = "Memory error"; -const char * const UnixAPI = "Unix API"; -const char * const CXXObjectLifecycle = "C++ object lifecycle"; -}}} +const char *const CoreFoundationObjectiveC = "Core Foundation/Objective-C"; +const char *const LogicError = "Logic error"; +const char *const MemoryRefCount = + "Memory (Core Foundation/Objective-C/OSObject)"; +const char *const MemoryError = "Memory error"; +const char *const UnixAPI = "Unix API"; +const char *const CXXObjectLifecycle = "C++ object lifecycle"; +const char *const SecurityError = "SecurityError"; + +} // namespace categories +} // namespace ento +} // namespace clang Index: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp @@ -0,0 +1,297 @@ +//===- CERTStrChecker - Checker for CERT STR rules --------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// This file defines CERTStrChecker which tries to find and resolve the CERT +// string handler function issues with fix-it hints. +// +// The rules can be found in section 'Rule 07. Characters and Strings (STR)': +// https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152038 +// +//===----------------------------------------------------------------------===// + +#include "AllocationState.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h" +#include "llvm/ADT/Optional.h" +#include <utility> + +using namespace clang; +using namespace ento; +using namespace ast_matchers; + +namespace { + +struct CallContext { + CallContext(Optional<unsigned> DestinationPos) + : DestinationPos(DestinationPos) {} + + Optional<unsigned> DestinationPos; +}; + +class CERTStrChecker + : public Checker<eval::Call, check::RegionChanges, check::BeginFunction> { + using StrCheck = std::function<void(const CERTStrChecker *, const CallEvent &, + const CallContext &, CheckerContext &)>; + +public: + bool evalCall(const CallEvent &Call, CheckerContext &C) const; + void checkBeginFunction(CheckerContext &C) const; + ProgramStateRef + checkRegionChanges(ProgramStateRef State, const InvalidatedSymbols *, + ArrayRef<const MemRegion *> ExplicitRegions, + ArrayRef<const MemRegion *> Regions, + const LocationContext *LCtx, const CallEvent *Call) const; + + std::unique_ptr<BugType> BT; + mutable bool UseSafeFunctions; + + const CallDescriptionMap<std::pair<StrCheck, CallContext>> CDM = { + // The following checks STR31-C rules. + // char *gets(char *dest); + {{"gets", 1}, {&CERTStrChecker::evalGets, {0}}}}; + + void evalGets(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const; +}; +} // namespace + +//===----------------------------------------------------------------------===// +// Helper functions. +//===----------------------------------------------------------------------===// + +// Returns the proper token based end location of \p E. +static SourceLocation exprLocEnd(const Expr *E, CheckerContext &C) { + assert(E); + return Lexer::getLocForEndOfToken(E->getEndLoc(), /*Offset=*/0, + C.getSourceManager(), C.getLangOpts()); +} + +// Returns a string representation of \p E. +static std::string exprToStr(const Expr *E, CheckerContext &C) { + assert(E); + return Lexer::getSourceText( + CharSourceRange::getTokenRange(E->getSourceRange()), C.getSourceManager(), + C.getLangOpts()); +} + +static Optional<std::string> getDestSizeAsString(const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) { + SVal DestV = Call.getArgSVal(*CallC.DestinationPos); + const MemRegion *DestMR = DestV.getAsRegion(); + + // Arrays. + if (const auto *ER = dyn_cast<ElementRegion>(DestMR)) { + // Dependent-sized array type or a member array. + if (const auto *FR = dyn_cast<FieldRegion>(ER->getSuperRegion())) + if (FR->getDecl()->getType()->isArrayType()) + if (const Expr *ArrayExpr = Call.getArgExpr(*CallC.DestinationPos)) + return "sizeof(" + exprToStr(ArrayExpr, C) + ")"; + + // Constant or variable array type. + if (const auto *VR = dyn_cast<VarRegion>(ER->getSuperRegion())) + if (const ValueDecl *VD = VR->getDecl()) + if (VD->getType()->isArrayType()) + return "sizeof(" + VD->getNameAsString() + ")"; + } + + // 'malloc()' family functions, 'alloca()' and new[]. + ProgramStateRef State = C.getState(); + SValBuilder &SVB = C.getSValBuilder(); + if (const MemRegion *BaseMR = DestMR->getBaseRegion()) { + if (const DynamicSizeInfo *SizeInfo = getDynamicSizeInfo(State, BaseMR)) { + if (SizeInfo->isValid()) { + if (const Expr *SizeExpr = SizeInfo->getSizeExpr()) + return exprToStr(SizeExpr, C); + } else { + DefinedOrUnknownSVal Size = SizeInfo->getSize(); + if (const llvm::APSInt *IntValue = SVB.getKnownValue(State, Size)) + return Twine(IntValue->getZExtValue()).str(); + } + } + } + + return None; +} + +std::unique_ptr<PathSensitiveBugReport> getReport(BugType &BT, + const CallEvent &Call, + const CallContext &CallC, + CheckerContext &C) { + SmallString<128> Msg; + llvm::raw_svector_ostream Out(Msg); + Out << '\'' << Call.getCalleeIdentifier()->getName() + << "' could write outside of "; + + const Expr *DestExpr = Call.getArgExpr(*CallC.DestinationPos); + static constexpr llvm::StringLiteral ArrayName = "array"; + + auto DRE = declRefExpr().bind(ArrayName); + auto ME = memberExpr().bind(ArrayName); + + auto Matches = + match(expr(anyOf(ME, DRE, hasDescendant(ME), hasDescendant(DRE))), + *DestExpr, C.getASTContext()); + + if (Matches.size() == 1) + Out << '\'' << exprToStr(Matches[0].getNodeAs<Expr>(ArrayName), C) << '\''; + else + Out << "the array"; + + auto Report = std::make_unique<PathSensitiveBugReport>( + BT, Out.str(), C.generateNonFatalErrorNode()); + + // Track the allocation. + SVal DestV = Call.getArgSVal(*CallC.DestinationPos); + Report->addVisitor(allocation_state::getMallocBRVisitor(DestV.getAsSymbol())); + return Report; +} + +//===----------------------------------------------------------------------===// +// Rewrite decision helpfer functions. +//===----------------------------------------------------------------------===// + +// We only want to fix simple buffers because when the buffer has an offset +// the new size expression needs to be modified according to the offset. +// FIXME: Use the 'SValVisitor' to make sure the offset is not present or +// subtract the offset from the new size expression. +bool isSimpleBuffer(const CallEvent &Call, const CallContext &CallC) { + const Expr *DestArg = + Call.getArgExpr(*CallC.DestinationPos)->IgnoreImpCasts(); + + if (const auto *DRE = dyn_cast<DeclRefExpr>(DestArg)) { + // Do not try to fix in-parameter buffers. + return !isa<ParmVarDecl>(DRE->getDecl()); + } + + return isa<MemberExpr>(DestArg); +} + +//===----------------------------------------------------------------------===// +// Code injection functions. +//===----------------------------------------------------------------------===// + +static void renameFunctionFix(StringRef NewName, const CallEvent &Call, + PathSensitiveBugReport &Report) { + unsigned CallNameLength = Call.getCalleeIdentifier()->getLength(); + SourceLocation CallBegin = Call.getSourceRange().getBegin(); + SourceRange CallNameRange(CallBegin, + CallBegin.getLocWithOffset(CallNameLength - 1)); + + const auto FuncNameFix = FixItHint::CreateReplacement(CallNameRange, NewName); + Report.addFixItHint(FuncNameFix); +} + +//===----------------------------------------------------------------------===// +// Evaluating problematic function calls. +//===----------------------------------------------------------------------===// + +void CERTStrChecker::evalGets(const CallEvent &Call, const CallContext &CallC, + CheckerContext &C) const { + unsigned DestPos = *CallC.DestinationPos; + const Expr *DestArg = Call.getArgExpr(DestPos)->IgnoreImpCasts(); + SVal DestV = Call.getArgSVal(DestPos); + + auto Report = getReport(*BT, Call, CallC, C); + if (!isSimpleBuffer(Call, CallC)) { + C.emitReport(std::move(Report)); + return; + } + + if (Optional<std::string> SizeStr = getDestSizeAsString(Call, CallC, C)) { + renameFunctionFix(UseSafeFunctions ? "gets_s" : "fgets", Call, *Report); + + std::string ArgumentFix = ", " + *SizeStr; + if (!UseSafeFunctions) + ArgumentFix += ", stdin"; + + auto CallFix = + FixItHint::CreateInsertion(exprLocEnd(DestArg, C), ArgumentFix); + Report->addFixItHint(CallFix); + + // Track the allocation's size expression. + constexpr llvm::StringLiteral ExprName = "expr"; + ProgramStateRef State = C.getState(); + const ExplodedNode *N = C.getPredecessor(); + const MemRegion *DestMR = DestV.getAsRegion()->getBaseRegion(); + + if (const Expr *SizeExpr = getDynamicSizeExpr(State, DestMR)) { + auto Matches = match(expr(forEachDescendant(expr().bind(ExprName))), + *SizeExpr, C.getASTContext()); + for (const BoundNodes &Match : Matches) + if (const Expr *E = Match.getNodeAs<Expr>(ExprName)) + bugreporter::trackExpressionValue(N, E, *Report); + } + } + + C.emitReport(std::move(Report)); +} + +//===----------------------------------------------------------------------===// +// Main logic to evaluate a call. +//===----------------------------------------------------------------------===// + +bool CERTStrChecker::evalCall(const CallEvent &Call, CheckerContext &C) const { + if (const auto *Lookup = CDM.lookup(Call)) { + const StrCheck &Check = Lookup->first; + Check(this, Call, Lookup->second, C); + } + + return false; +} + +ProgramStateRef CERTStrChecker::checkRegionChanges( + ProgramStateRef State, const InvalidatedSymbols *, + ArrayRef<const MemRegion *> ExplicitRegions, ArrayRef<const MemRegion *>, + const LocationContext *LCtx, const CallEvent *) const { + return invalidateDynamicSizeExpr(State, ExplicitRegions, LCtx); +} + +void CERTStrChecker::checkBeginFunction(CheckerContext &C) const { + // Check the preprocessor only when the analysis begins. + if (!C.inTopFrame()) + return; + + if (!UseSafeFunctions) + return; + + Preprocessor &PP = C.getPreprocessor(); + if (PP.isMacroDefined("__STDC_LIB_EXT1__")) { + MacroDefinition MD = PP.getMacroDefinition("__STDC_WANT_LIB_EXT1__"); + if (const MacroInfo *MI = MD.getMacroInfo()) { + const Token &T = MI->tokens().back(); + StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength()); + llvm::APInt IntValue; + ValueStr.getAsInteger(10, IntValue); + UseSafeFunctions = IntValue.getZExtValue(); + return; + } + } + + UseSafeFunctions = false; +} + +void ento::registerCERTStrChecker(CheckerManager &Mgr) { + auto *Checker = Mgr.registerChecker<CERTStrChecker>(); + + Checker->UseSafeFunctions = Mgr.getAnalyzerOptions().getCheckerBooleanOption( + Checker, "WantToUseSafeFunctions"); + + Checker->BT = std::make_unique<BugType>( + Mgr.getCurrentCheckerName(), "Insecure string handler function call", + categories::SecurityError); +} + +bool ento::shouldRegisterCERTStrChecker(const LangOptions &) { return true; } Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -3374,6 +3374,10 @@ namespace ento { namespace allocation_state { +std::unique_ptr<BugReporterVisitor> getMallocBRVisitor(SymbolRef Sym) { + return std::make_unique<MallocBugVisitor>(Sym); +} + ProgramStateRef markReleased(ProgramStateRef State, SymbolRef Sym, const Expr *Origin) { AllocationFamily Family = AF_InnerBuffer; Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -17,6 +17,7 @@ CastSizeChecker.cpp CastToStructChecker.cpp CastValueChecker.cpp + cert/StrChecker.cpp CheckObjCDealloc.cpp CheckObjCInstMethSignature.cpp CheckSecuritySyntaxOnly.cpp Index: clang/lib/StaticAnalyzer/Checkers/AllocationState.h =================================================================== --- clang/lib/StaticAnalyzer/Checkers/AllocationState.h +++ clang/lib/StaticAnalyzer/Checkers/AllocationState.h @@ -25,6 +25,9 @@ /// AF_InnerBuffer symbols. std::unique_ptr<BugReporterVisitor> getInnerPointerBRVisitor(SymbolRef Sym); +/// \returns The MallocBugVisitor. +std::unique_ptr<BugReporterVisitor> getMallocBRVisitor(SymbolRef Sym); + /// 'Sym' represents a pointer to the inner buffer of a container object. /// This function looks up the memory region of that object in /// DanglingInternalBufferChecker's program state map. Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSizeInfo.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSizeInfo.h +++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSizeInfo.h @@ -9,34 +9,42 @@ #ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_DYNAMICSIZEINFO_H #define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_DYNAMICSIZEINFO_H -namespace clang { -namespace ento { - #include "clang/AST/Expr.h" #include "clang/Basic/LLVM.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h" +namespace clang { +namespace ento { + /// Helper class to store information about the dynamic size. class DynamicSizeInfo { public: - DynamicSizeInfo(DefinedOrUnknownSVal Size, const Expr *SizeExpr = nullptr) - : Size(Size), SizeExpr(SizeExpr) {} + DynamicSizeInfo(DefinedOrUnknownSVal Size, const Expr *SizeExpr = nullptr, + bool IsValid = true) + : Size(Size), SizeExpr(SizeExpr), IsValid(IsValid) {} DefinedOrUnknownSVal getSize() const { return Size; } + const Expr *getSizeExpr() const { return SizeExpr; } + /// \returns The size expression being valid. + bool isValid() const { return IsValid; } + bool operator==(const DynamicSizeInfo &RHS) const { - return Size == RHS.Size && SizeExpr == RHS.SizeExpr; + return Size == RHS.Size && SizeExpr == RHS.SizeExpr && + IsValid == RHS.IsValid; } void Profile(llvm::FoldingSetNodeID &ID) const { ID.Add(Size); ID.AddPointer(SizeExpr); + ID.AddBoolean(IsValid); } private: DefinedOrUnknownSVal Size; const Expr *SizeExpr; + bool IsValid; }; } // namespace ento Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h +++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h @@ -13,6 +13,7 @@ #ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_DYNAMICSIZE_H #define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_DYNAMICSIZE_H +#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSizeInfo.h" #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" @@ -22,6 +23,10 @@ namespace clang { namespace ento { +/// \returns The dynamic size info for the region \p MR. +const DynamicSizeInfo *getDynamicSizeInfo(ProgramStateRef State, + const MemRegion *MR); + /// \returns The stored dynamic size for the region \p MR. DefinedOrUnknownSVal getDynamicSize(ProgramStateRef State, const MemRegion *MR, SValBuilder &SVB); @@ -40,6 +45,17 @@ ProgramStateRef setDynamicSize(ProgramStateRef State, const MemRegion *MR, DefinedOrUnknownSVal Size, const Expr *SizeExpr); +/// If a part of the size expression is modified later on the path the size +/// expression cannot be used to represent the allocated memory block's size. +/// This function tries to split up the size expression and see whether the +/// \p ExplicitRegions contains one of the regions of the size expression which +/// means the expression will be modified later on the path so we need to mark +/// the size expression invalid. +ProgramStateRef +invalidateDynamicSizeExpr(ProgramStateRef State, + ArrayRef<const MemRegion *> ExplicitRegions, + const LocationContext *LCtx); + } // namespace ento } // namespace clang Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h +++ clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h @@ -11,16 +11,17 @@ // Common strings used for the "category" of many static analyzer issues. namespace clang { - namespace ento { - namespace categories { - extern const char * const CoreFoundationObjectiveC; - extern const char * const LogicError; - extern const char * const MemoryRefCount; - extern const char * const MemoryError; - extern const char * const UnixAPI; - extern const char * const CXXObjectLifecycle; - } - } -} -#endif +namespace ento { +namespace categories { +extern const char *const CoreFoundationObjectiveC; +extern const char *const LogicError; +extern const char *const MemoryRefCount; +extern const char *const MemoryError; +extern const char *const UnixAPI; +extern const char *const CXXObjectLifecycle; +extern const char *const SecurityError; +} // namespace categories +} // namespace ento +} // namespace clang +#endif // LLVM_CLANG_STATICANALYZER_CORE_BUGREPORTER_COMMONBUGCATEGORIES_H Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -67,6 +67,7 @@ def Performance : Package<"performance">, ParentPackage<OptIn>; def Security : Package <"security">; +def CERT : Package<"cert">, ParentPackage<Security>; def InsecureAPI : Package<"insecureAPI">, ParentPackage<Security>; def SecurityAlpha : Package<"security">, ParentPackage<Alpha>; def Taint : Package<"taint">, ParentPackage<SecurityAlpha>; @@ -785,6 +786,24 @@ } // end "security" +let ParentPackage = CERT in { + +def CERTStrChecker : Checker<"Str">, + HelpText<"CERT checker of string related rules.">, + CheckerOptions<[ + CmdLineOption<Boolean, + "WantToUseSafeFunctions", + "An integer non-zero value specifying if the target " + "environment is considered to implement '_s' suffixed memory " + "and string handler functions which are safer than older " + "versions (e.g. 'memcpy_s()')", + "true", + Released>, + ]>, + Documentation<NotDocumented>; + +} // end "CERT" + let ParentPackage = SecurityAlpha in { def ArrayBoundChecker : Checker<"ArrayBound">, Index: clang/include/clang/Lex/Preprocessor.h =================================================================== --- clang/include/clang/Lex/Preprocessor.h +++ clang/include/clang/Lex/Preprocessor.h @@ -1032,6 +1032,9 @@ return MD && MD->isDefined(); } + MacroDefinition getMacroDefinition(StringRef Id) { + return getMacroDefinition(&Identifiers.get(Id)); + } MacroDefinition getMacroDefinition(const IdentifierInfo *II) { if (!II->hasMacroDefinition()) return {};
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
