[cfe-commits] r107935 - in /cfe/trunk: lib/Checker/CStringChecker.cpp test/Analysis/bstring.c
Jordy Rose
jediknil at belkadan.com
Thu Jul 8 16:57:29 PDT 2010
Author: jrose
Date: Thu Jul 8 18:57:29 2010
New Revision: 107935
URL: http://llvm.org/viewvc/llvm-project?rev=107935&view=rev
Log:
Cleanup in CStringChecker. Now properly bifurcates the state for zero/nonzero sizes.
Modified:
cfe/trunk/lib/Checker/CStringChecker.cpp
cfe/trunk/test/Analysis/bstring.c
Modified: cfe/trunk/lib/Checker/CStringChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Checker/CStringChecker.cpp?rev=107935&r1=107934&r2=107935&view=diff
==============================================================================
--- cfe/trunk/lib/Checker/CStringChecker.cpp (original)
+++ cfe/trunk/lib/Checker/CStringChecker.cpp Thu Jul 8 18:57:29 2010
@@ -21,35 +21,40 @@
namespace {
class CStringChecker : public CheckerVisitor<CStringChecker> {
- BugType *BT_Bounds;
- BugType *BT_Overlap;
+ BugType *BT_Null, *BT_Bounds, *BT_Overlap;
public:
CStringChecker()
- : BT_Bounds(0), BT_Overlap(0) {}
+ : BT_Null(0), BT_Bounds(0), BT_Overlap(0) {}
static void *getTag() { static int tag; return &tag; }
bool EvalCallExpr(CheckerContext &C, const CallExpr *CE);
- typedef const GRState *(CStringChecker::*FnCheck)(CheckerContext &,
- const CallExpr *);
+ typedef void (CStringChecker::*FnCheck)(CheckerContext &, const CallExpr *);
- const GRState *EvalMemcpy(CheckerContext &C, const CallExpr *CE);
- const GRState *EvalMemmove(CheckerContext &C, const CallExpr *CE);
- const GRState *EvalMemcmp(CheckerContext &C, const CallExpr *CE);
- const GRState *EvalBcopy(CheckerContext &C, const CallExpr *CE);
+ void EvalMemcpy(CheckerContext &C, const CallExpr *CE);
+ void EvalMemmove(CheckerContext &C, const CallExpr *CE);
+ void EvalBcopy(CheckerContext &C, const CallExpr *CE);
+ void EvalCopyCommon(CheckerContext &C, const GRState *state,
+ const Expr *Size, const Expr *Source, const Expr *Dest,
+ bool Restricted = false);
+
+ void EvalMemcmp(CheckerContext &C, const CallExpr *CE);
// Utility methods
+ std::pair<const GRState*, const GRState*>
+ AssumeZero(CheckerContext &C, const GRState *state, SVal V, QualType Ty);
+
const GRState *CheckNonNull(CheckerContext &C, const GRState *state,
- const Stmt *S, SVal l);
+ const Expr *S, SVal l);
const GRState *CheckLocation(CheckerContext &C, const GRState *state,
- const Stmt *S, SVal l);
+ const Expr *S, SVal l);
const GRState *CheckBufferAccess(CheckerContext &C, const GRState *state,
const Expr *Size,
const Expr *FirstBuf,
const Expr *SecondBuf = NULL);
const GRState *CheckOverlap(CheckerContext &C, const GRState *state,
- const Expr *First, const Expr *Second,
- const Expr *Size);
+ const Expr *Size, const Expr *First,
+ const Expr *Second);
void EmitOverlapBug(CheckerContext &C, const GRState *state,
const Stmt *First, const Stmt *Second);
};
@@ -59,34 +64,47 @@
Eng.registerCheck(new CStringChecker());
}
-const GRState *CStringChecker::CheckNonNull(CheckerContext &C,
- const GRState *state,
- const Stmt *S, SVal l) {
- // FIXME: This method just checks, of course, that the value is non-null.
- // It should maybe be refactored and combined with AttrNonNullChecker.
- if (l.isUnknownOrUndef())
- return state;
+//===----------------------------------------------------------------------===//
+// Individual checks and utility methods.
+//===----------------------------------------------------------------------===//
+
+std::pair<const GRState*, const GRState*>
+CStringChecker::AssumeZero(CheckerContext &C, const GRState *state, SVal V,
+ QualType Ty) {
+ DefinedSVal *Val = dyn_cast<DefinedSVal>(&V);
+ if (!Val)
+ return std::pair<const GRState*, const GRState *>(state, state);
ValueManager &ValMgr = C.getValueManager();
SValuator &SV = ValMgr.getSValuator();
- Loc Null = ValMgr.makeNull();
- DefinedOrUnknownSVal LocIsNull = SV.EvalEQ(state, cast<Loc>(l), Null);
+ DefinedOrUnknownSVal Zero = ValMgr.makeZeroVal(Ty);
+ DefinedOrUnknownSVal ValIsZero = SV.EvalEQ(state, *Val, Zero);
- const GRState *stateIsNull, *stateIsNonNull;
- llvm::tie(stateIsNull, stateIsNonNull) = state->Assume(LocIsNull);
+ return state->Assume(ValIsZero);
+}
- if (stateIsNull && !stateIsNonNull) {
- ExplodedNode *N = C.GenerateSink(stateIsNull);
+const GRState *CStringChecker::CheckNonNull(CheckerContext &C,
+ const GRState *state,
+ const Expr *S, SVal l) {
+ // If a previous check has failed, propagate the failure.
+ if (!state)
+ return NULL;
+
+ const GRState *stateNull, *stateNonNull;
+ llvm::tie(stateNull, stateNonNull) = AssumeZero(C, state, l, S->getType());
+
+ if (stateNull && !stateNonNull) {
+ ExplodedNode *N = C.GenerateSink(stateNull);
if (!N)
return NULL;
- if (!BT_Bounds)
- BT_Bounds = new BuiltinBug("API",
+ if (!BT_Null)
+ BT_Null = new BuiltinBug("API",
"Null pointer argument in call to byte string function");
// Generate a report for this bug.
- BuiltinBug *BT = static_cast<BuiltinBug*>(BT_Bounds);
+ BuiltinBug *BT = static_cast<BuiltinBug*>(BT_Null);
EnhancedBugReport *report = new EnhancedBugReport(*BT,
BT->getDescription(), N);
@@ -97,14 +115,18 @@
}
// From here on, assume that the value is non-null.
- assert(stateIsNonNull);
- return stateIsNonNull;
+ assert(stateNonNull);
+ return stateNonNull;
}
// FIXME: This was originally copied from ArrayBoundChecker.cpp. Refactor?
const GRState *CStringChecker::CheckLocation(CheckerContext &C,
const GRState *state,
- const Stmt *S, SVal l) {
+ const Expr *S, SVal l) {
+ // If a previous check has failed, propagate the failure.
+ if (!state)
+ return NULL;
+
// Check for out of bound array element access.
const MemRegion *R = l.getAsRegion();
if (!R)
@@ -161,6 +183,10 @@
const Expr *Size,
const Expr *FirstBuf,
const Expr *SecondBuf) {
+ // If a previous check has failed, propagate the failure.
+ if (!state)
+ return NULL;
+
ValueManager &VM = C.getValueManager();
SValuator &SV = VM.getSValuator();
ASTContext &Ctx = C.getASTContext();
@@ -168,38 +194,18 @@
QualType SizeTy = Ctx.getSizeType();
QualType PtrTy = Ctx.getPointerType(Ctx.CharTy);
- // Get the access length and make sure it is known.
- SVal LengthVal = state->getSVal(Size);
- NonLoc *Length = dyn_cast<NonLoc>(&LengthVal);
- if (!Length)
- return state;
-
- // If the length is zero, it doesn't matter what the two buffers are.
- DefinedOrUnknownSVal Zero = VM.makeZeroVal(SizeTy);
- DefinedOrUnknownSVal LengthIsZero = SV.EvalEQ(state, *Length, Zero);
-
- const GRState *stateZeroLength, *stateNonZeroLength;
- llvm::tie(stateZeroLength, stateNonZeroLength) = state->Assume(LengthIsZero);
- if (stateZeroLength && !stateNonZeroLength)
- return stateZeroLength;
-
- // FIXME: At this point all we know is it's *possible* for the length to be
- // nonzero; we don't know it for sure. Unfortunately, that means the next few
- // tests are incorrect for the edge cases in which a buffer is null or invalid
- // but the size argument was set to zero in some way that we couldn't track.
- // What we should really do is bifurcate the state here, but that doesn't
- // match the way CheckBufferAccess is being used.
-
- // From here on, we're going to pretend that even if the length is zero, the
- // buffer access rules still apply. That means the buffer must be non-NULL,
- // and the value at buffer[size-1] must be valid.
-
// Check that the first buffer is non-null.
SVal BufVal = state->getSVal(FirstBuf);
state = CheckNonNull(C, state, FirstBuf, BufVal);
if (!state)
return NULL;
+ // Get the access length and make sure it is known.
+ SVal LengthVal = state->getSVal(Size);
+ NonLoc *Length = dyn_cast<NonLoc>(&LengthVal);
+ if (!Length)
+ return state;
+
// Compute the offset of the last element to be accessed: size-1.
NonLoc One = cast<NonLoc>(VM.makeIntVal(1, SizeTy));
NonLoc LastOffset = cast<NonLoc>(SV.EvalBinOpNN(state, BinaryOperator::Sub,
@@ -234,13 +240,17 @@
const GRState *CStringChecker::CheckOverlap(CheckerContext &C,
const GRState *state,
+ const Expr *Size,
const Expr *First,
- const Expr *Second,
- const Expr *Size) {
+ const Expr *Second) {
// Do a simple check for overlap: if the two arguments are from the same
// buffer, see if the end of the first is greater than the start of the second
// or vice versa.
+ // If a previous check has failed, propagate the failure.
+ if (!state)
+ return NULL;
+
ValueManager &VM = state->getStateManager().getValueManager();
SValuator &SV = VM.getSValuator();
ASTContext &Ctx = VM.getContext();
@@ -359,37 +369,60 @@
C.EmitReport(report);
}
-const GRState *
-CStringChecker::EvalMemcpy(CheckerContext &C, const CallExpr *CE) {
- // void *memcpy(void *restrict dst, const void *restrict src, size_t n);
- // memcpy() is like memmove(), but with the extra requirement that the buffers
- // not overlap.
- const GRState *state = EvalMemmove(C, CE);
- if (!state)
- return NULL;
+//===----------------------------------------------------------------------===//
+// Evaluation of individual function calls.
+//===----------------------------------------------------------------------===//
+
+void CStringChecker::EvalCopyCommon(CheckerContext &C, const GRState *state,
+ const Expr *Size, const Expr *Dest,
+ const Expr *Source, bool Restricted) {
+ // See if the size argument is zero.
+ SVal SizeVal = state->getSVal(Size);
+ QualType SizeTy = Size->getType();
+
+ const GRState *StZeroSize, *StNonZeroSize;
+ llvm::tie(StZeroSize, StNonZeroSize) = AssumeZero(C, state, SizeVal, SizeTy);
+
+ // If the size is zero, there won't be any actual memory access.
+ if (StZeroSize)
+ C.addTransition(StZeroSize);
+
+ // If the size can be nonzero, we have to check the other arguments.
+ if (StNonZeroSize) {
+ state = StNonZeroSize;
+ state = CheckBufferAccess(C, state, Size, Dest, Source);
+ if (Restricted)
+ state = CheckOverlap(C, state, Size, Dest, Source);
+ if (state)
+ C.addTransition(state);
+ }
+}
- return CheckOverlap(C, state, CE->getArg(0), CE->getArg(1), CE->getArg(2));
+
+void CStringChecker::EvalMemcpy(CheckerContext &C, const CallExpr *CE) {
+ // void *memcpy(void *restrict dst, const void *restrict src, size_t n);
+ // The return value is the address of the destination buffer.
+ const Expr *Dest = CE->getArg(0);
+ const GRState *state = C.getState();
+ state = state->BindExpr(CE, state->getSVal(Dest));
+ EvalCopyCommon(C, state, CE->getArg(2), Dest, CE->getArg(1), true);
}
-const GRState *
-CStringChecker::EvalMemmove(CheckerContext &C, const CallExpr *CE) {
+void CStringChecker::EvalMemmove(CheckerContext &C, const CallExpr *CE) {
// void *memmove(void *dst, const void *src, size_t n);
+ // The return value is the address of the destination buffer.
const Expr *Dest = CE->getArg(0);
- const Expr *Source = CE->getArg(1);
- const Expr *Size = CE->getArg(2);
-
- // Check that the accesses will stay in bounds.
const GRState *state = C.getState();
- state = CheckBufferAccess(C, state, Size, Dest, Source);
- if (!state)
- return NULL;
+ state = state->BindExpr(CE, state->getSVal(Dest));
+ EvalCopyCommon(C, state, CE->getArg(2), Dest, CE->getArg(1));
+}
- // The return value is the address of the destination buffer.
- return state->BindExpr(CE, state->getSVal(Dest));
+void CStringChecker::EvalBcopy(CheckerContext &C, const CallExpr *CE) {
+ // void bcopy(const void *src, void *dst, size_t n);
+ EvalCopyCommon(C, C.getState(), CE->getArg(2), CE->getArg(1), CE->getArg(0));
}
-const GRState *
-CStringChecker::EvalMemcmp(CheckerContext &C, const CallExpr *CE) {
+void CStringChecker::EvalMemcmp(CheckerContext &C, const CallExpr *CE) {
// int memcmp(const void *s1, const void *s2, size_t n);
const Expr *Left = CE->getArg(0);
const Expr *Right = CE->getArg(1);
@@ -398,66 +431,67 @@
const GRState *state = C.getState();
ValueManager &ValMgr = C.getValueManager();
SValuator &SV = ValMgr.getSValuator();
- const GRState *stateTrue, *stateFalse;
-
- // If we know the size argument is 0, we know the result is 0, and we don't
- // have to check either of the buffers. (Another checker will have already
- // made sure the size isn't undefined, so we can cast it safely.)
- DefinedOrUnknownSVal SizeV = cast<DefinedOrUnknownSVal>(state->getSVal(Size));
- DefinedOrUnknownSVal Zero = ValMgr.makeZeroVal(Size->getType());
-
- DefinedOrUnknownSVal SizeIsZero = SV.EvalEQ(state, SizeV, Zero);
- llvm::tie(stateTrue, stateFalse) = state->Assume(SizeIsZero);
-
- // FIXME: This should really cause a bifurcation of the state, but that would
- // require changing the contract to allow the various Eval* methods to add
- // transitions themselves. Currently that isn't the case because some of these
- // functions are "basically" like another function, but with one or two
- // additional restrictions (like memcpy and memmove).
-
- if (stateTrue && !stateFalse)
- return stateTrue->BindExpr(CE, ValMgr.makeZeroVal(CE->getType()));
-
- // At this point, we still don't know that the size is nonzero, only that it
- // might be.
-
- // If we know the two buffers are the same, we know the result is 0.
- // First, get the two buffers' addresses. Another checker will have already
- // made sure they're not undefined.
- DefinedOrUnknownSVal LBuf = cast<DefinedOrUnknownSVal>(state->getSVal(Left));
- DefinedOrUnknownSVal RBuf = cast<DefinedOrUnknownSVal>(state->getSVal(Right));
-
- // See if they are the same.
- DefinedOrUnknownSVal SameBuf = SV.EvalEQ(state, LBuf, RBuf);
- llvm::tie(stateTrue, stateFalse) = state->Assume(SameBuf);
-
- // FIXME: This should also bifurcate the state (as above).
- // If the two arguments are known to be the same buffer, we know the result is
- // zero, and we only need to check one size.
- if (stateTrue && !stateFalse) {
- state = CheckBufferAccess(C, stateTrue, Size, Left);
- return state->BindExpr(CE, ValMgr.makeZeroVal(CE->getType()));
+ // See if the size argument is zero.
+ SVal SizeVal = state->getSVal(Size);
+ QualType SizeTy = Size->getType();
+
+ const GRState *StZeroSize, *StNonZeroSize;
+ llvm::tie(StZeroSize, StNonZeroSize) = AssumeZero(C, state, SizeVal, SizeTy);
+
+ // If the size can be zero, the result will be 0 in that case, and we don't
+ // have to check either of the buffers.
+ if (StZeroSize) {
+ state = StZeroSize;
+ state = state->BindExpr(CE, ValMgr.makeZeroVal(CE->getType()));
+ C.addTransition(state);
}
- // At this point, we don't know if the arguments are the same or not -- we
- // only know that they *might* be different. We can't make any assumptions.
-
- // The return value is the comparison result, which we don't know.
- unsigned Count = C.getNodeBuilder().getCurrentBlockCount();
- SVal RetVal = ValMgr.getConjuredSymbolVal(NULL, CE, CE->getType(), Count);
- state = state->BindExpr(CE, RetVal);
+ // If the size can be nonzero, we have to check the other arguments.
+ if (StNonZeroSize) {
+ state = StNonZeroSize;
+
+ // If we know the two buffers are the same, we know the result is 0.
+ // First, get the two buffers' addresses. Another checker will have already
+ // made sure they're not undefined.
+ DefinedOrUnknownSVal LV = cast<DefinedOrUnknownSVal>(state->getSVal(Left));
+ DefinedOrUnknownSVal RV = cast<DefinedOrUnknownSVal>(state->getSVal(Right));
+
+ // See if they are the same.
+ DefinedOrUnknownSVal SameBuf = SV.EvalEQ(state, LV, RV);
+ const GRState *StSameBuf, *StNotSameBuf;
+ llvm::tie(StSameBuf, StNotSameBuf) = state->Assume(SameBuf);
+
+ // If the two arguments might be the same buffer, we know the result is zero,
+ // and we only need to check one size.
+ if (StSameBuf) {
+ state = StSameBuf;
+ state = CheckBufferAccess(C, state, Size, Left);
+ if (state) {
+ state = StSameBuf->BindExpr(CE, ValMgr.makeZeroVal(CE->getType()));
+ C.addTransition(state);
+ }
+ }
- // Check that the accesses will stay in bounds.
- return CheckBufferAccess(C, state, Size, Left, Right);
+ // If the two arguments might be different buffers, we have to check the
+ // size of both of them.
+ if (StNotSameBuf) {
+ state = StNotSameBuf;
+ state = CheckBufferAccess(C, state, Size, Left, Right);
+ if (state) {
+ // The return value is the comparison result, which we don't know.
+ unsigned Count = C.getNodeBuilder().getCurrentBlockCount();
+ SVal CmpV = ValMgr.getConjuredSymbolVal(NULL, CE, CE->getType(), Count);
+ state = state->BindExpr(CE, CmpV);
+ C.addTransition(state);
+ }
+ }
+ }
}
-const GRState *
-CStringChecker::EvalBcopy(CheckerContext &C, const CallExpr *CE) {
- // void bcopy(const void *src, void *dst, size_t n);
- return CheckBufferAccess(C, C.getState(),
- CE->getArg(2), CE->getArg(0), CE->getArg(1));
-}
+//===----------------------------------------------------------------------===//
+// The driver method.
+//===----------------------------------------------------------------------===//
bool CStringChecker::EvalCallExpr(CheckerContext &C, const CallExpr *CE) {
// Get the callee. All the functions we care about are C functions
@@ -481,13 +515,11 @@
.Case("bcopy", &CStringChecker::EvalBcopy)
.Default(NULL);
+ // If the callee isn't a string function, let another checker handle it.
if (!EvalFunction)
- // The callee isn't a string function. Let another checker handle it.
return false;
- const GRState *NewState = (this->*EvalFunction)(C, CE);
-
- if (NewState)
- C.addTransition(NewState);
+ // Check and evaluate the call.
+ (this->*EvalFunction)(C, CE);
return true;
}
Modified: cfe/trunk/test/Analysis/bstring.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/bstring.c?rev=107935&r1=107934&r2=107935&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/bstring.c (original)
+++ cfe/trunk/test/Analysis/bstring.c Thu Jul 8 18:57:29 2010
@@ -238,6 +238,14 @@
(void)*(char*)0; // no-warning
}
+void memcmp6 (char *a, char *b, size_t n) {
+ int result = memcmp(a, b, n);
+ if (result != 0)
+ return;
+ if (n == 0)
+ (void)*(char*)0; // expected-warning{{null}}
+}
+
//===----------------------------------------------------------------------===
// bcopy()
//===----------------------------------------------------------------------===
More information about the cfe-commits
mailing list