From 1053d246f451c399468248625d1146e3d845e21e Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Fri, 6 Nov 2009 02:24:13 +0000 Subject: [PATCH] static analyzer: refactor checking logic for returning the address of a stack variable or a garbage value into their own respective subclasses of Checker (and put them in .cpp files where their implementation details are hidden from GRExprEngine). git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@86215 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../clang/Analysis/PathSensitive/Checker.h | 4 + .../Analysis/PathSensitive/CheckerVisitor.def | 3 +- .../Analysis/PathSensitive/GRExprEngine.h | 14 --- lib/Analysis/CMakeLists.txt | 4 +- lib/Analysis/GRExprEngine.cpp | 84 ++++++---------- lib/Analysis/GRExprEngineInternalChecks.cpp | 78 +-------------- lib/Analysis/GRExprEngineInternalChecks.h | 26 +++++ lib/Analysis/ReturnStackAddressChecker.cpp | 97 +++++++++++++++++++ lib/Analysis/ReturnUndefChecker.cpp | 68 +++++++++++++ 9 files changed, 232 insertions(+), 146 deletions(-) create mode 100644 lib/Analysis/GRExprEngineInternalChecks.h create mode 100644 lib/Analysis/ReturnStackAddressChecker.cpp create mode 100644 lib/Analysis/ReturnUndefChecker.cpp diff --git a/include/clang/Analysis/PathSensitive/Checker.h b/include/clang/Analysis/PathSensitive/Checker.h index 4fc0a617ec..2564a73089 100644 --- a/include/clang/Analysis/PathSensitive/Checker.h +++ b/include/clang/Analysis/PathSensitive/Checker.h @@ -76,6 +76,10 @@ public: BugReporter &getBugReporter() { return Eng.getBugReporter(); } + + SourceManager &getSourceManager() { + return getBugReporter().getSourceManager(); + } ExplodedNode *GenerateNode(const Stmt *S, bool markAsSink = false) { return GenerateNode(S, getState(), markAsSink); diff --git a/include/clang/Analysis/PathSensitive/CheckerVisitor.def b/include/clang/Analysis/PathSensitive/CheckerVisitor.def index ff6528dae8..3f68a7a9d5 100644 --- a/include/clang/Analysis/PathSensitive/CheckerVisitor.def +++ b/include/clang/Analysis/PathSensitive/CheckerVisitor.def @@ -11,8 +11,9 @@ // //===---------------------------------------------------------------------===// +PREVISIT(BinaryOperator) PREVISIT(CallExpr) PREVISIT(ObjCMessageExpr) -PREVISIT(BinaryOperator) +PREVISIT(ReturnStmt) #undef PREVISIT diff --git a/include/clang/Analysis/PathSensitive/GRExprEngine.h b/include/clang/Analysis/PathSensitive/GRExprEngine.h index 25e47038d7..f6971374e8 100644 --- a/include/clang/Analysis/PathSensitive/GRExprEngine.h +++ b/include/clang/Analysis/PathSensitive/GRExprEngine.h @@ -223,10 +223,6 @@ public: return static_cast(lookupChecker(CHECKER::getTag())); } - bool isRetStackAddr(const ExplodedNode* N) const { - return N->isSink() && RetsStackAddr.count(const_cast(N)) != 0; - } - bool isUndefControlFlow(const ExplodedNode* N) const { return N->isSink() && UndefBranches.count(const_cast(N)) != 0; } @@ -267,14 +263,6 @@ public: return N->isSink() && UndefReceivers.count(const_cast(N)) != 0; } - typedef ErrorNodes::iterator ret_stackaddr_iterator; - ret_stackaddr_iterator ret_stackaddr_begin() { return RetsStackAddr.begin(); } - ret_stackaddr_iterator ret_stackaddr_end() { return RetsStackAddr.end(); } - - typedef ErrorNodes::iterator ret_undef_iterator; - ret_undef_iterator ret_undef_begin() { return RetsUndef.begin(); } - ret_undef_iterator ret_undef_end() { return RetsUndef.end(); } - typedef ErrorNodes::iterator undef_branch_iterator; undef_branch_iterator undef_branches_begin() { return UndefBranches.begin(); } undef_branch_iterator undef_branches_end() { return UndefBranches.end(); } @@ -560,8 +548,6 @@ protected: getTF().EvalObjCMessageExpr(Dst, *this, *Builder, ME, Pred); } - void EvalReturn(ExplodedNodeSet& Dst, ReturnStmt* s, ExplodedNode* Pred); - const GRState* MarkBranch(const GRState* St, Stmt* Terminator, bool branchTaken); diff --git a/lib/Analysis/CMakeLists.txt b/lib/Analysis/CMakeLists.txt index cd4697f2f3..111f273df6 100644 --- a/lib/Analysis/CMakeLists.txt +++ b/lib/Analysis/CMakeLists.txt @@ -36,6 +36,8 @@ add_clang_library(clangAnalysis PathDiagnostic.cpp RangeConstraintManager.cpp RegionStore.cpp + ReturnStackAddressChecker.cpp + ReturnUndefChecker.cpp SVals.cpp SValuator.cpp SimpleConstraintManager.cpp @@ -45,8 +47,8 @@ add_clang_library(clangAnalysis UndefinedArgChecker.cpp UndefinedAssignmentChecker.cpp UninitializedValues.cpp - ValueManager.cpp VLASizeChecker.cpp + ValueManager.cpp ) add_dependencies(clangAnalysis ClangDiagnosticAnalysis) diff --git a/lib/Analysis/GRExprEngine.cpp b/lib/Analysis/GRExprEngine.cpp index 212fea3a6b..1bfb49d9ef 100644 --- a/lib/Analysis/GRExprEngine.cpp +++ b/lib/Analysis/GRExprEngine.cpp @@ -2628,63 +2628,37 @@ void GRExprEngine::VisitAsmStmtHelperInputs(AsmStmt* A, VisitAsmStmtHelperInputs(A, I, E, *NI, Dst); } -void GRExprEngine::EvalReturn(ExplodedNodeSet& Dst, ReturnStmt* S, - ExplodedNode* Pred) { - assert (Builder && "GRStmtNodeBuilder must be defined."); - - unsigned size = Dst.size(); - - SaveAndRestore OldSink(Builder->BuildSinks); - SaveOr OldHasGen(Builder->HasGeneratedNode); - - getTF().EvalReturn(Dst, *this, *Builder, S, Pred); - - // Handle the case where no nodes where generated. - - if (!Builder->BuildSinks && Dst.size() == size && !Builder->HasGeneratedNode) - MakeNode(Dst, S, Pred, GetState(Pred)); -} - -void GRExprEngine::VisitReturnStmt(ReturnStmt* S, ExplodedNode* Pred, - ExplodedNodeSet& Dst) { - - Expr* R = S->getRetValue(); - - if (!R) { - EvalReturn(Dst, S, Pred); - return; +void GRExprEngine::VisitReturnStmt(ReturnStmt *RS, ExplodedNode *Pred, + ExplodedNodeSet &Dst) { + + ExplodedNodeSet Src; + if (Expr *RetE = RS->getRetValue()) { + Visit(RetE, Pred, Src); } + else { + Src.Add(Pred); + } + + ExplodedNodeSet CheckedSet; + CheckerVisit(RS, CheckedSet, Src, true); + + for (ExplodedNodeSet::iterator I = CheckedSet.begin(), E = CheckedSet.end(); + I != E; ++I) { - ExplodedNodeSet Tmp; - Visit(R, Pred, Tmp); - - for (ExplodedNodeSet::iterator I = Tmp.begin(), E = Tmp.end(); I != E; ++I) { - SVal X = (*I)->getState()->getSVal(R); - - // Check if we return the address of a stack variable. - if (isa(X)) { - // Determine if the value is on the stack. - const MemRegion* R = cast(&X)->getRegion(); - - if (R && R->hasStackStorage()) { - // Create a special node representing the error. - if (ExplodedNode* N = Builder->generateNode(S, GetState(*I), *I)) { - N->markAsSink(); - RetsStackAddr.insert(N); - } - continue; - } - } - // Check if we return an undefined value. - else if (X.isUndef()) { - if (ExplodedNode* N = Builder->generateNode(S, GetState(*I), *I)) { - N->markAsSink(); - RetsUndef.insert(N); - } - continue; - } - - EvalReturn(Dst, S, *I); + assert(Builder && "GRStmtNodeBuilder must be defined."); + + Pred = *I; + unsigned size = Dst.size(); + + SaveAndRestore OldSink(Builder->BuildSinks); + SaveOr OldHasGen(Builder->HasGeneratedNode); + + getTF().EvalReturn(Dst, *this, *Builder, RS, Pred); + + // Handle the case where no nodes where generated. + if (!Builder->BuildSinks && Dst.size() == size && + !Builder->HasGeneratedNode) + MakeNode(Dst, RS, Pred, GetState(Pred)); } } diff --git a/lib/Analysis/GRExprEngineInternalChecks.cpp b/lib/Analysis/GRExprEngineInternalChecks.cpp index 695f0b02e5..4bb5d226d1 100644 --- a/lib/Analysis/GRExprEngineInternalChecks.cpp +++ b/lib/Analysis/GRExprEngineInternalChecks.cpp @@ -12,6 +12,7 @@ // //===----------------------------------------------------------------------===// +#include "GRExprEngineInternalChecks.h" #include "clang/Analysis/PathSensitive/BugReporter.h" #include "clang/Analysis/PathSensitive/GRExprEngine.h" #include "clang/Analysis/PathSensitive/CheckerVisitor.h" @@ -290,79 +291,6 @@ public: } }; -class VISIBILITY_HIDDEN RetStack : public BuiltinBug { -public: - RetStack(GRExprEngine* eng) - : BuiltinBug(eng, "Return of address to stack-allocated memory") {} - - void FlushReportsImpl(BugReporter& BR, GRExprEngine& Eng) { - for (GRExprEngine::ret_stackaddr_iterator I=Eng.ret_stackaddr_begin(), - End = Eng.ret_stackaddr_end(); I!=End; ++I) { - - ExplodedNode* N = *I; - const Stmt *S = cast(N->getLocation()).getStmt(); - const Expr* E = cast(S)->getRetValue(); - assert(E && "Return expression cannot be NULL"); - - // Get the value associated with E. - loc::MemRegionVal V = cast(N->getState()->getSVal(E)); - - // Generate a report for this bug. - std::string buf; - llvm::raw_string_ostream os(buf); - SourceRange R; - - // Check if the region is a compound literal. - if (const CompoundLiteralRegion* CR = - dyn_cast(V.getRegion())) { - - const CompoundLiteralExpr* CL = CR->getLiteralExpr(); - os << "Address of stack memory associated with a compound literal " - "declared on line " - << BR.getSourceManager() - .getInstantiationLineNumber(CL->getLocStart()) - << " returned."; - - R = CL->getSourceRange(); - } - else if (const AllocaRegion* AR = dyn_cast(V.getRegion())) { - const Expr* ARE = AR->getExpr(); - SourceLocation L = ARE->getLocStart(); - R = ARE->getSourceRange(); - - os << "Address of stack memory allocated by call to alloca() on line " - << BR.getSourceManager().getInstantiationLineNumber(L) - << " returned."; - } - else { - os << "Address of stack memory associated with local variable '" - << V.getRegion()->getString() << "' returned."; - } - - RangedBugReport *report = new RangedBugReport(*this, os.str().c_str(), N); - report->addRange(E->getSourceRange()); - if (R.isValid()) report->addRange(R); - BR.EmitReport(report); - } - } -}; - -class VISIBILITY_HIDDEN RetUndef : public BuiltinBug { -public: - RetUndef(GRExprEngine* eng) : BuiltinBug(eng, "Garbage return value", - "Undefined or garbage value returned to caller") {} - - void FlushReportsImpl(BugReporter& BR, GRExprEngine& Eng) { - Emit(BR, Eng.ret_undef_begin(), Eng.ret_undef_end()); - } - - void registerInitialVisitors(BugReporterContext& BRC, - const ExplodedNode* N, - BuiltinBugReport *R) { - registerTrackNullOrUndefValue(BRC, GetRetValExpr(N), N); - } -}; - class VISIBILITY_HIDDEN UndefBranch : public BuiltinBug { struct VISIBILITY_HIDDEN FindUndefExpr { GRStateManager& VM; @@ -464,8 +392,6 @@ void GRExprEngine::RegisterInternalChecks() { // to 'FlushReports' from BugReporter. BR.Register(new UndefBranch(this)); BR.Register(new UndefResult(this)); - BR.Register(new RetStack(this)); - BR.Register(new RetUndef(this)); BR.Register(new BadMsgExprArg(this)); BR.Register(new BadReceiver(this)); BR.Register(new OutOfBoundMemoryAccess(this)); @@ -477,6 +403,8 @@ void GRExprEngine::RegisterInternalChecks() { // their associated BugType will get registered with the BugReporter // automatically. Note that the check itself is owned by the GRExprEngine // object. + RegisterReturnStackAddressChecker(*this); + RegisterReturnUndefChecker(*this); registerCheck(new AttrNonNullChecker()); registerCheck(new UndefinedArgChecker()); registerCheck(new UndefinedAssignmentChecker()); diff --git a/lib/Analysis/GRExprEngineInternalChecks.h b/lib/Analysis/GRExprEngineInternalChecks.h new file mode 100644 index 0000000000..622b5ab110 --- /dev/null +++ b/lib/Analysis/GRExprEngineInternalChecks.h @@ -0,0 +1,26 @@ +//=-- GRExprEngineInternalChecks.h- Builtin GRExprEngine Checks -----*- C++ -*-= +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This file defines functions to instantiate and register the "built-in" +// checks in GRExprEngine. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_GREXPRENGINE_INTERNAL_CHECKS +#define LLVM_CLANG_GREXPRENGINE_INTERNAL_CHECKS + +namespace clang { + +class GRExprEngine; + +void RegisterReturnStackAddressChecker(GRExprEngine &Eng); +void RegisterReturnUndefChecker(GRExprEngine &Eng); + +} // end clang namespace +#endif diff --git a/lib/Analysis/ReturnStackAddressChecker.cpp b/lib/Analysis/ReturnStackAddressChecker.cpp new file mode 100644 index 0000000000..9f22b3c48e --- /dev/null +++ b/lib/Analysis/ReturnStackAddressChecker.cpp @@ -0,0 +1,97 @@ +//== ReturnStackAddressChecker.cpp ------------------------------*- C++ -*--==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This file defines ReturnStackAddressChecker, which is a path-sensitive +// check which looks for the addresses of stack variables being returned to +// callers. +// +//===----------------------------------------------------------------------===// + +#include "GRExprEngineInternalChecks.h" +#include "clang/Analysis/PathSensitive/GRExprEngine.h" +#include "clang/Analysis/PathSensitive/BugReporter.h" +#include "clang/Analysis/PathSensitive/CheckerVisitor.h" +#include "llvm/ADT/SmallString.h" + +using namespace clang; + +namespace { +class VISIBILITY_HIDDEN ReturnStackAddressChecker : + public CheckerVisitor { + BuiltinBug *BT; +public: + ReturnStackAddressChecker() : BT(0) {} + static void *getTag(); + void PreVisitReturnStmt(CheckerContext &C, const ReturnStmt *RS); +}; +} + +void clang::RegisterReturnStackAddressChecker(GRExprEngine &Eng) { + Eng.registerCheck(new ReturnStackAddressChecker()); +} + +void *ReturnStackAddressChecker::getTag() { + static int x = 0; return &x; +} + +void ReturnStackAddressChecker::PreVisitReturnStmt(CheckerContext &C, + const ReturnStmt *RS) { + + const Expr *RetE = RS->getRetValue(); + if (!RetE) + return; + + SVal V = C.getState()->getSVal(RetE); + const MemRegion *R = V.getAsRegion(); + + if (!R || !R->hasStackStorage()) + return; + + ExplodedNode *N = C.GenerateNode(RS, C.getState(), true); + + if (!N) + return; + + if (!BT) + BT = new BuiltinBug("Return of address to stack-allocated memory"); + + // Generate a report for this bug. + llvm::SmallString<100> buf; + llvm::raw_svector_ostream os(buf); + SourceRange range; + + // Check if the region is a compound literal. + if (const CompoundLiteralRegion* CR = dyn_cast(R)) { + const CompoundLiteralExpr* CL = CR->getLiteralExpr(); + os << "Address of stack memory associated with a compound literal " + "declared on line " + << C.getSourceManager().getInstantiationLineNumber(CL->getLocStart()) + << " returned to caller"; + range = CL->getSourceRange(); + } + else if (const AllocaRegion* AR = dyn_cast(R)) { + const Expr* ARE = AR->getExpr(); + SourceLocation L = ARE->getLocStart(); + range = ARE->getSourceRange(); + os << "Address of stack memory allocated by call to alloca() on line " + << C.getSourceManager().getInstantiationLineNumber(L) + << " returned to caller"; + } + else { + os << "Address of stack memory associated with local variable '" + << R->getString() << "' returned."; + } + + RangedBugReport *report = new RangedBugReport(*BT, os.str().data(), N); + report->addRange(RS->getSourceRange()); + if (range.isValid()) + report->addRange(range); + + C.EmitReport(report); +} diff --git a/lib/Analysis/ReturnUndefChecker.cpp b/lib/Analysis/ReturnUndefChecker.cpp new file mode 100644 index 0000000000..adde3f5d25 --- /dev/null +++ b/lib/Analysis/ReturnUndefChecker.cpp @@ -0,0 +1,68 @@ +//== ReturnUndefChecker.cpp -------------------------------------*- C++ -*--==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This file defines ReturnUndefChecker, which is a path-sensitive +// check which looks for undefined or garbage values being returned to the +// caller. +// +//===----------------------------------------------------------------------===// + +#include "GRExprEngineInternalChecks.h" +#include "clang/Analysis/PathSensitive/GRExprEngine.h" +#include "clang/Analysis/PathSensitive/BugReporter.h" +#include "clang/Analysis/PathSensitive/CheckerVisitor.h" +#include "llvm/ADT/SmallString.h" + +using namespace clang; + +namespace { +class VISIBILITY_HIDDEN ReturnUndefChecker : + public CheckerVisitor { + BuiltinBug *BT; +public: + ReturnUndefChecker() : BT(0) {} + static void *getTag(); + void PreVisitReturnStmt(CheckerContext &C, const ReturnStmt *RS); +}; +} + +void clang::RegisterReturnUndefChecker(GRExprEngine &Eng) { + Eng.registerCheck(new ReturnUndefChecker()); +} + +void *ReturnUndefChecker::getTag() { + static int x = 0; return &x; +} + +void ReturnUndefChecker::PreVisitReturnStmt(CheckerContext &C, + const ReturnStmt *RS) { + + const Expr *RetE = RS->getRetValue(); + if (!RetE) + return; + + if (!C.getState()->getSVal(RetE).isUndef()) + return; + + ExplodedNode *N = C.GenerateNode(RS, C.getState(), true); + + if (!N) + return; + + if (!BT) + BT = new BuiltinBug("Garbage return value", + "Undefined or garbage value returned to caller"); + + EnhancedBugReport *report = + new EnhancedBugReport(*BT, BT->getDescription().c_str(), N); + + report->addVisitorCreator(bugreporter::registerTrackNullOrUndefValue, RetE); + + C.EmitReport(report); +} -- 2.40.0