From cd8f6ac9b613e1fe962ebf9c87d822ce765275e6 Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Tue, 6 Oct 2009 01:39:48 +0000 Subject: [PATCH] Fix: Static analyzer warns about NULL pointer when adding assert This fix required a few changes: SimpleSValuator: - Eagerly replace a symbolic value with its constant value in EvalBinOpNN when it is constrained to a constant. This allows us to better constant fold values along a path. - Handle trivial case of '<', '>' comparison of pointers when the two pointers are exactly the same. RegionStoreManager: git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@83358 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Analysis/PathSensitive/GRExprEngine.h | 10 +++--- .../clang/Analysis/PathSensitive/GRState.h | 6 ++++ .../clang/Analysis/PathSensitive/SValuator.h | 4 +-- lib/Analysis/GRExprEngine.cpp | 2 +- lib/Analysis/RegionStore.cpp | 33 +++++++++++++------ lib/Analysis/SValuator.cpp | 2 +- lib/Analysis/SimpleSValuator.cpp | 26 ++++++++++++--- test/Analysis/misc-ps-region-store.m | 16 +++++++++ 8 files changed, 77 insertions(+), 22 deletions(-) diff --git a/include/clang/Analysis/PathSensitive/GRExprEngine.h b/include/clang/Analysis/PathSensitive/GRExprEngine.h index 60a59d4968..e5c61e68c7 100644 --- a/include/clang/Analysis/PathSensitive/GRExprEngine.h +++ b/include/clang/Analysis/PathSensitive/GRExprEngine.h @@ -548,12 +548,14 @@ protected: public: - SVal EvalBinOp(BinaryOperator::Opcode op, NonLoc L, NonLoc R, QualType T) { - return SVator.EvalBinOpNN(op, L, R, T); + SVal EvalBinOp(const GRState *state, BinaryOperator::Opcode op, + NonLoc L, NonLoc R, QualType T) { + return SVator.EvalBinOpNN(state, op, L, R, T); } - SVal EvalBinOp(BinaryOperator::Opcode op, NonLoc L, SVal R, QualType T) { - return R.isValid() ? SVator.EvalBinOpNN(op, L, cast(R), T) : R; + SVal EvalBinOp(const GRState *state, BinaryOperator::Opcode op, + NonLoc L, SVal R, QualType T) { + return R.isValid() ? SVator.EvalBinOpNN(state, op, L, cast(R), T) : R; } SVal EvalBinOp(const GRState *ST, BinaryOperator::Opcode Op, diff --git a/include/clang/Analysis/PathSensitive/GRState.h b/include/clang/Analysis/PathSensitive/GRState.h index b9d444540d..f4224e4b09 100644 --- a/include/clang/Analysis/PathSensitive/GRState.h +++ b/include/clang/Analysis/PathSensitive/GRState.h @@ -259,6 +259,8 @@ public: SVal getSVal(const MemRegion* R) const; SVal getSValAsScalarOrLoc(const MemRegion *R) const; + + const llvm::APSInt *getSymVal(SymbolRef sym); bool scanReachableSymbols(SVal val, SymbolVisitor& visitor) const; @@ -566,6 +568,10 @@ public: // Out-of-line method definitions for GRState. //===----------------------------------------------------------------------===// +inline const llvm::APSInt *GRState::getSymVal(SymbolRef sym) { + return getStateManager().getSymVal(this, sym); +} + inline const VarRegion* GRState::getRegion(const VarDecl *D, const LocationContext *LC) const { return getStateManager().getRegionManager().getVarRegion(D, LC); diff --git a/include/clang/Analysis/PathSensitive/SValuator.h b/include/clang/Analysis/PathSensitive/SValuator.h index 729aba78ad..e63eb12cf8 100644 --- a/include/clang/Analysis/PathSensitive/SValuator.h +++ b/include/clang/Analysis/PathSensitive/SValuator.h @@ -67,8 +67,8 @@ public: virtual SVal EvalComplement(NonLoc val) = 0; - virtual SVal EvalBinOpNN(BinaryOperator::Opcode Op, NonLoc lhs, - NonLoc rhs, QualType resultTy) = 0; + virtual SVal EvalBinOpNN(const GRState *state, BinaryOperator::Opcode Op, + NonLoc lhs, NonLoc rhs, QualType resultTy) = 0; virtual SVal EvalBinOpLL(BinaryOperator::Opcode Op, Loc lhs, Loc rhs, QualType resultTy) = 0; diff --git a/lib/Analysis/GRExprEngine.cpp b/lib/Analysis/GRExprEngine.cpp index dc39d8b041..8de200cb1e 100644 --- a/lib/Analysis/GRExprEngine.cpp +++ b/lib/Analysis/GRExprEngine.cpp @@ -2545,7 +2545,7 @@ void GRExprEngine::VisitUnaryOperator(UnaryOperator* U, ExplodedNode* Pred, } else { nonloc::ConcreteInt X(getBasicVals().getValue(0, Ex->getType())); - Result = EvalBinOp(BinaryOperator::EQ, cast(V), X, + Result = EvalBinOp(state, BinaryOperator::EQ, cast(V), X, U->getType()); } diff --git a/lib/Analysis/RegionStore.cpp b/lib/Analysis/RegionStore.cpp index 7a433dd148..46e1d12a3c 100644 --- a/lib/Analysis/RegionStore.cpp +++ b/lib/Analysis/RegionStore.cpp @@ -826,7 +826,10 @@ SVal RegionStoreManager::EvalBinOp(const GRState *state, // Not yet handled. case MemRegion::VarRegionKind: - case MemRegion::StringRegionKind: + case MemRegion::StringRegionKind: { + + } + // Fall-through. case MemRegion::CompoundLiteralRegionKind: case MemRegion::FieldRegionKind: case MemRegion::ObjCObjectRegionKind: @@ -851,17 +854,27 @@ SVal RegionStoreManager::EvalBinOp(const GRState *state, SVal Idx = ER->getIndex(); nonloc::ConcreteInt* Base = dyn_cast(&Idx); - nonloc::ConcreteInt* Offset = dyn_cast(&R); - // Only support concrete integer indexes for now. - if (Base && Offset) { - // FIXME: Should use SValuator here. - SVal NewIdx = Base->evalBinOp(ValMgr, Op, + // For now, only support: + // (a) concrete integer indices that can easily be resolved + // (b) 0 + symbolic index + if (Base) { + if (nonloc::ConcreteInt *Offset = dyn_cast(&R)) { + // FIXME: Should use SValuator here. + SVal NewIdx = + Base->evalBinOp(ValMgr, Op, cast(ValMgr.convertToArrayIndex(*Offset))); - const MemRegion* NewER = - MRMgr.getElementRegion(ER->getElementType(), NewIdx, ER->getSuperRegion(), - getContext()); - return ValMgr.makeLoc(NewER); + const MemRegion* NewER = + MRMgr.getElementRegion(ER->getElementType(), NewIdx, + ER->getSuperRegion(), getContext()); + return ValMgr.makeLoc(NewER); + } + if (0 == Base->getValue()) { + const MemRegion* NewER = + MRMgr.getElementRegion(ER->getElementType(), R, + ER->getSuperRegion(), getContext()); + return ValMgr.makeLoc(NewER); + } } return UnknownVal(); diff --git a/lib/Analysis/SValuator.cpp b/lib/Analysis/SValuator.cpp index 0551c7cdba..86006c3df6 100644 --- a/lib/Analysis/SValuator.cpp +++ b/lib/Analysis/SValuator.cpp @@ -43,7 +43,7 @@ SVal SValuator::EvalBinOp(const GRState *ST, BinaryOperator::Opcode Op, return EvalBinOpLN(ST, Op, cast(R), cast(L), T); } - return EvalBinOpNN(Op, cast(L), cast(R), T); + return EvalBinOpNN(ST, Op, cast(L), cast(R), T); } DefinedOrUnknownSVal SValuator::EvalEQ(const GRState *ST, diff --git a/lib/Analysis/SimpleSValuator.cpp b/lib/Analysis/SimpleSValuator.cpp index 52a591927d..2869fabea3 100644 --- a/lib/Analysis/SimpleSValuator.cpp +++ b/lib/Analysis/SimpleSValuator.cpp @@ -29,8 +29,8 @@ public: virtual SVal EvalMinus(NonLoc val); virtual SVal EvalComplement(NonLoc val); - virtual SVal EvalBinOpNN(BinaryOperator::Opcode op, NonLoc lhs, NonLoc rhs, - QualType resultTy); + virtual SVal EvalBinOpNN(const GRState *state, BinaryOperator::Opcode op, + NonLoc lhs, NonLoc rhs, QualType resultTy); virtual SVal EvalBinOpLL(BinaryOperator::Opcode op, Loc lhs, Loc rhs, QualType resultTy); virtual SVal EvalBinOpLN(const GRState *state, BinaryOperator::Opcode op, @@ -206,7 +206,8 @@ static SVal EvalEquality(ValueManager &ValMgr, Loc lhs, Loc rhs, bool isEqual, return ValMgr.makeTruthVal(isEqual ? lhs == rhs : lhs != rhs, resultTy); } -SVal SimpleSValuator::EvalBinOpNN(BinaryOperator::Opcode op, +SVal SimpleSValuator::EvalBinOpNN(const GRState *state, + BinaryOperator::Opcode op, NonLoc lhs, NonLoc rhs, QualType resultTy) { // Handle trivial case where left-side and right-side are the same. @@ -342,8 +343,18 @@ SVal SimpleSValuator::EvalBinOpNN(BinaryOperator::Opcode op, } } case nonloc::SymbolValKind: { + nonloc::SymbolVal *slhs = cast(&lhs); + SymbolRef Sym = slhs->getSymbol(); + + // Does the symbol simplify to a constant? + if (Sym->getType(ValMgr.getContext())->isIntegerType()) + if (const llvm::APSInt *Constant = state->getSymVal(Sym)) { + lhs = nonloc::ConcreteInt(*Constant); + continue; + } + if (isa(rhs)) { - return ValMgr.makeNonLoc(cast(lhs).getSymbol(), op, + return ValMgr.makeNonLoc(slhs->getSymbol(), op, cast(rhs).getValue(), resultTy); } @@ -362,6 +373,13 @@ SVal SimpleSValuator::EvalBinOpLL(BinaryOperator::Opcode op, Loc lhs, Loc rhs, case BinaryOperator::EQ: case BinaryOperator::NE: return EvalEquality(ValMgr, lhs, rhs, op == BinaryOperator::EQ, resultTy); + case BinaryOperator::LT: + case BinaryOperator::GT: + // FIXME: Generalize. For now, just handle the trivial case where + // the two locations are identical. + if (lhs == rhs) + return ValMgr.makeTruthVal(false, resultTy); + return UnknownVal(); } } diff --git a/test/Analysis/misc-ps-region-store.m b/test/Analysis/misc-ps-region-store.m index 24ebe5b236..e849042b3d 100644 --- a/test/Analysis/misc-ps-region-store.m +++ b/test/Analysis/misc-ps-region-store.m @@ -287,3 +287,19 @@ int rdar_7261075(void) { return 0; } +// false path due to limited pointer +// arithmetic constraints +void rdar_7275774(void *data, unsigned n) { + if (!(data || n == 0)) + return; + + unsigned short *p = (unsigned short*) data; + unsigned short *q = p + (n / 2); + + if (p < q) { + // If we reach here, 'p' cannot be null. If 'p' is null, then 'n' must + // be '0', meaning that this branch is not feasible. + *p = *q; // no-warning + } +} + -- 2.40.0