From: Ted Kremenek Date: Wed, 4 Nov 2009 04:24:16 +0000 (+0000) Subject: Catch uses of undefined values when they are used in assignment, thus catching such... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b107c4b7efb907d75620cd3c17f82fe27dc5b745;p=clang Catch uses of undefined values when they are used in assignment, thus catching such bugs closer to the source. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@86003 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Analysis/PathSensitive/Checker.h b/include/clang/Analysis/PathSensitive/Checker.h index 0134599511..3bef08d754 100644 --- a/include/clang/Analysis/PathSensitive/Checker.h +++ b/include/clang/Analysis/PathSensitive/Checker.h @@ -114,11 +114,19 @@ private: _PreVisit(C, stmt); } - + void GR_VisitBind(ExplodedNodeSet &Dst, + GRStmtNodeBuilder &Builder, GRExprEngine &Eng, + const Stmt *stmt, ExplodedNode *Pred, void *tag, + SVal location, SVal val, + bool isPrevisit) { + CheckerContext C(Dst, Builder, Eng, Pred, tag, isPrevisit); + assert(isPrevisit && "Only previsit supported for now."); + PreVisitBind(C, stmt, location, val); + } public: virtual ~Checker() {} - virtual void _PreVisit(CheckerContext &C, const Stmt *stmt) {} + virtual void _PreVisit(CheckerContext &C, const Stmt *ST) {} // This is a previsit which takes a node returns a node. virtual ExplodedNode *CheckLocation(const Stmt *S, ExplodedNode *Pred, @@ -126,6 +134,9 @@ public: GRExprEngine &Eng) { return Pred; } + + virtual void PreVisitBind(CheckerContext &C, const Stmt *ST, + SVal location, SVal val) {} virtual ExplodedNode *CheckType(QualType T, ExplodedNode *Pred, const GRState *state, Stmt *S, diff --git a/include/clang/Analysis/PathSensitive/Checkers/UndefinedAssignmentChecker.h b/include/clang/Analysis/PathSensitive/Checkers/UndefinedAssignmentChecker.h new file mode 100644 index 0000000000..7fee5011c4 --- /dev/null +++ b/include/clang/Analysis/PathSensitive/Checkers/UndefinedAssignmentChecker.h @@ -0,0 +1,32 @@ +//===--- UndefinedAssignmentChecker.h ---------------------------*- C++ -*--==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This defines UndefinedAssginmentChecker, a builtin check in GRExprEngine that +// checks for assigning undefined values. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_UNDEFASSIGNMENTCHECKER +#define LLVM_CLANG_UNDEFASSIGNMENTCHECKER + +#include "clang/Analysis/PathSensitive/CheckerVisitor.h" + +namespace clang { +class UndefinedAssignmentChecker + : public CheckerVisitor { + BugType *BT; +public: + UndefinedAssignmentChecker() : BT(0) {} + static void *getTag(); + virtual void PreVisitBind(CheckerContext &C, const Stmt *S, SVal location, + SVal val); +}; +} +#endif + diff --git a/include/clang/Analysis/PathSensitive/GRExprEngine.h b/include/clang/Analysis/PathSensitive/GRExprEngine.h index 1d2b08711c..2f2a11a83c 100644 --- a/include/clang/Analysis/PathSensitive/GRExprEngine.h +++ b/include/clang/Analysis/PathSensitive/GRExprEngine.h @@ -411,6 +411,10 @@ protected: /// at specific statements. void CheckerVisit(Stmt *S, ExplodedNodeSet &Dst, ExplodedNodeSet &Src, bool isPrevisit); + + void CheckerVisitBind(Stmt *S, ExplodedNodeSet &Dst, ExplodedNodeSet &Src, + SVal location, SVal val, bool isPrevisit); + /// Visit - Transfer function logic for all statements. Dispatches to /// other functions that handle specific kinds of statements. diff --git a/lib/Analysis/GRExprEngine.cpp b/lib/Analysis/GRExprEngine.cpp index bf1f6ab1e3..c71882e4d8 100644 --- a/lib/Analysis/GRExprEngine.cpp +++ b/lib/Analysis/GRExprEngine.cpp @@ -139,6 +139,41 @@ void GRExprEngine::CheckerVisit(Stmt *S, ExplodedNodeSet &Dst, // automatically. } +// FIXME: This is largely copy-paste from CheckerVisit(). Need to +// unify. +void GRExprEngine::CheckerVisitBind(Stmt *S, ExplodedNodeSet &Dst, + ExplodedNodeSet &Src, + SVal location, SVal val, bool isPrevisit) { + + if (Checkers.empty()) { + Dst = Src; + return; + } + + ExplodedNodeSet Tmp; + ExplodedNodeSet *PrevSet = &Src; + + for (CheckersOrdered::iterator I=Checkers.begin(),E=Checkers.end(); I!=E; ++I) + { + ExplodedNodeSet *CurrSet = (I+1 == E) ? &Dst + : (PrevSet == &Tmp) ? &Src : &Tmp; + + CurrSet->clear(); + void *tag = I->first; + Checker *checker = I->second; + + for (ExplodedNodeSet::iterator NI = PrevSet->begin(), NE = PrevSet->end(); + NI != NE; ++NI) + checker->GR_VisitBind(*CurrSet, *Builder, *this, S, *NI, tag, location, + val, isPrevisit); + + // Update which NodeSet is the current one. + PrevSet = CurrSet; + } + + // Don't autotransition. The CheckerContext objects should do this + // automatically. +} //===----------------------------------------------------------------------===// // Engine construction and deletion. //===----------------------------------------------------------------------===// @@ -1093,36 +1128,49 @@ void GRExprEngine::VisitMemberExpr(MemberExpr* M, ExplodedNode* Pred, void GRExprEngine::EvalBind(ExplodedNodeSet& Dst, Stmt* Ex, ExplodedNode* Pred, const GRState* state, SVal location, SVal Val, bool atDeclInit) { + + + // Do a previsit of the bind. + ExplodedNodeSet CheckedSet, Src; + Src.Add(Pred); + CheckerVisitBind(Ex, CheckedSet, Src, location, Val, true); + + for (ExplodedNodeSet::iterator I = CheckedSet.begin(), E = CheckedSet.end(); + I!=E; ++I) { + + if (Pred != *I) + state = GetState(*I); + + const GRState* newState = 0; - const GRState* newState = 0; - - if (atDeclInit) { - const VarRegion *VR = - cast(cast(location).getRegion()); + if (atDeclInit) { + const VarRegion *VR = + cast(cast(location).getRegion()); - newState = state->bindDecl(VR, Val); - } - else { - if (location.isUnknown()) { - // We know that the new state will be the same as the old state since - // the location of the binding is "unknown". Consequently, there - // is no reason to just create a new node. - newState = state; + newState = state->bindDecl(VR, Val); } else { - // We are binding to a value other than 'unknown'. Perform the binding - // using the StoreManager. - newState = state->bindLoc(cast(location), Val); + if (location.isUnknown()) { + // We know that the new state will be the same as the old state since + // the location of the binding is "unknown". Consequently, there + // is no reason to just create a new node. + newState = state; + } + else { + // We are binding to a value other than 'unknown'. Perform the binding + // using the StoreManager. + newState = state->bindLoc(cast(location), Val); + } } - } - // The next thing to do is check if the GRTransferFuncs object wants to - // update the state based on the new binding. If the GRTransferFunc object - // doesn't do anything, just auto-propagate the current state. - GRStmtNodeBuilderRef BuilderRef(Dst, *Builder, *this, Pred, newState, Ex, - newState != state); + // The next thing to do is check if the GRTransferFuncs object wants to + // update the state based on the new binding. If the GRTransferFunc object + // doesn't do anything, just auto-propagate the current state. + GRStmtNodeBuilderRef BuilderRef(Dst, *Builder, *this, *I, newState, Ex, + newState != state); - getTF().EvalBind(BuilderRef, location, Val); + getTF().EvalBind(BuilderRef, location, Val); + } } /// EvalStore - Handle the semantics of a store via an assignment. diff --git a/lib/Analysis/GRExprEngineInternalChecks.cpp b/lib/Analysis/GRExprEngineInternalChecks.cpp index e0112ee984..695f0b02e5 100644 --- a/lib/Analysis/GRExprEngineInternalChecks.cpp +++ b/lib/Analysis/GRExprEngineInternalChecks.cpp @@ -19,6 +19,7 @@ #include "clang/Analysis/PathSensitive/Checkers/DivZeroChecker.h" #include "clang/Analysis/PathSensitive/Checkers/BadCallChecker.h" #include "clang/Analysis/PathSensitive/Checkers/UndefinedArgChecker.h" +#include "clang/Analysis/PathSensitive/Checkers/UndefinedAssignmentChecker.h" #include "clang/Analysis/PathSensitive/Checkers/AttrNonNullChecker.h" #include "clang/Analysis/PathSensitive/Checkers/VLASizeChecker.h" #include "clang/Analysis/PathDiagnostic.h" @@ -476,12 +477,13 @@ void GRExprEngine::RegisterInternalChecks() { // their associated BugType will get registered with the BugReporter // automatically. Note that the check itself is owned by the GRExprEngine // object. - registerCheck(new AttrNonNullChecker()); - registerCheck(new UndefinedArgChecker()); - registerCheck(new BadCallChecker()); - registerCheck(new DivZeroChecker()); - registerCheck(new UndefDerefChecker()); - registerCheck(new NullDerefChecker()); - registerCheck(new UndefSizedVLAChecker()); - registerCheck(new ZeroSizedVLAChecker()); + registerCheck(new AttrNonNullChecker()); + registerCheck(new UndefinedArgChecker()); + registerCheck(new UndefinedAssignmentChecker()); + registerCheck(new BadCallChecker()); + registerCheck(new DivZeroChecker()); + registerCheck(new UndefDerefChecker()); + registerCheck(new NullDerefChecker()); + registerCheck(new UndefSizedVLAChecker()); + registerCheck(new ZeroSizedVLAChecker()); } diff --git a/lib/Analysis/UndefinedAssignmentChecker.cpp b/lib/Analysis/UndefinedAssignmentChecker.cpp new file mode 100644 index 0000000000..9df58844dc --- /dev/null +++ b/lib/Analysis/UndefinedAssignmentChecker.cpp @@ -0,0 +1,59 @@ +//===--- UndefinedAssignmentChecker.h ---------------------------*- C++ -*--==// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This defines UndefinedAssginmentChecker, a builtin check in GRExprEngine that +// checks for assigning undefined values. +// +//===----------------------------------------------------------------------===// + +#include "clang/Analysis/PathSensitive/Checkers/UndefinedAssignmentChecker.h" +#include "clang/Analysis/PathSensitive/BugReporter.h" + +using namespace clang; + +void *UndefinedAssignmentChecker::getTag() { + static int x = 0; + return &x; +} + +void UndefinedAssignmentChecker::PreVisitBind(CheckerContext &C, + const Stmt *S, + SVal location, + SVal val) { + if (!val.isUndef()) + return; + + ExplodedNode *N = C.GenerateNode(S, true); + + if (!N) + return; + + if (!BT) + BT = new BugType("Assigned value is garbage or undefined", + "Logic error"); + + // Generate a report for this bug. + EnhancedBugReport *R = new EnhancedBugReport(*BT, BT->getName().c_str(), N); + const Expr *ex = 0; + + if (const BinaryOperator *B = dyn_cast(S)) + ex = B->getRHS(); + else if (const DeclStmt *DS = dyn_cast(S)) { + const VarDecl* VD = dyn_cast(DS->getSingleDecl()); + ex = VD->getInit(); + } + + if (ex) { + R->addRange(ex->getSourceRange()); + R->addVisitorCreator(bugreporter::registerTrackNullOrUndefValue, ex); + } + + C.EmitReport(R); +} + diff --git a/test/Analysis/uninit-vals-ps-region.c b/test/Analysis/uninit-vals-ps-region.c index 1561f11c99..e927a92576 100644 --- a/test/Analysis/uninit-vals-ps-region.c +++ b/test/Analysis/uninit-vals-ps-region.c @@ -24,9 +24,20 @@ void test_uninit_pos() { struct TestUninit v1 = { 0, 0 }; struct TestUninit v2 = test_uninit_aux(); int z; - v1.y = z; - test_unit_aux2(v2.x + v1.y); // expected-warning{{The right operand of '+' is a garbage value}} + v1.y = z; // expected-warning{{Assigned value is garbage or undefined}} + test_unit_aux2(v2.x + v1.y); } +void test_uninit_pos_2() { + struct TestUninit v1 = { 0, 0 }; + struct TestUninit v2; + test_unit_aux2(v2.x + v1.y); // expected-warning{{The left operand of '+' is a garbage value}} +} +void test_uninit_pos_3() { + struct TestUninit v1 = { 0, 0 }; + struct TestUninit v2; + test_unit_aux2(v1.y + v2.x); // expected-warning{{The right operand of '+' is a garbage value}} +} + void test_uninit_neg() { struct TestUninit v1 = { 0, 0 }; struct TestUninit v2 = test_uninit_aux();