[cfe-commits] r107759 - in /cfe/trunk: lib/Checker/CStringChecker.cpp test/Analysis/bstring.c
Jordy Rose
jediknil at belkadan.com
Wed Jul 7 00:48:06 PDT 2010
Author: jrose
Date: Wed Jul 7 02:48:06 2010
New Revision: 107759
URL: http://llvm.org/viewvc/llvm-project?rev=107759&view=rev
Log:
Cleanup on CStringChecker and its associated tests. Also check for null arguments...which are allowed if the access length is 0!
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=107759&r1=107758&r2=107759&view=diff
==============================================================================
--- cfe/trunk/lib/Checker/CStringChecker.cpp (original)
+++ cfe/trunk/lib/Checker/CStringChecker.cpp Wed Jul 7 02:48:06 2010
@@ -38,6 +38,8 @@
const GRState *EvalBcopy(CheckerContext &C, const CallExpr *CE);
// Utility methods
+ const GRState *CheckNonNull(CheckerContext &C, const GRState *state,
+ const Stmt *S, SVal l);
const GRState *CheckLocation(CheckerContext &C, const GRState *state,
const Stmt *S, SVal l);
const GRState *CheckBufferAccess(CheckerContext &C, const GRState *state,
@@ -56,6 +58,48 @@
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;
+
+ ValueManager &ValMgr = C.getValueManager();
+ SValuator &SV = ValMgr.getSValuator();
+
+ Loc Null = ValMgr.makeNull();
+ DefinedOrUnknownSVal LocIsNull = SV.EvalEQ(state, cast<Loc>(l), Null);
+
+ const GRState *stateIsNull, *stateIsNonNull;
+ llvm::tie(stateIsNull, stateIsNonNull) = state->Assume(LocIsNull);
+
+ if (stateIsNull && !stateIsNonNull) {
+ ExplodedNode *N = C.GenerateSink(stateIsNull);
+ if (!N)
+ return NULL;
+
+ if (!BT_Bounds)
+ BT_Bounds = 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);
+ EnhancedBugReport *report = new EnhancedBugReport(*BT,
+ BT->getDescription(), N);
+
+ report->addRange(S->getSourceRange());
+ report->addVisitorCreator(bugreporter::registerTrackNullOrUndefValue, S);
+ C.EmitReport(report);
+ return NULL;
+ }
+
+ // From here on, assume that the value is non-null.
+ assert(stateIsNonNull);
+ return stateIsNonNull;
+}
+
// FIXME: This was originally copied from ArrayBoundChecker.cpp. Refactor?
const GRState *CStringChecker::CheckLocation(CheckerContext &C,
const GRState *state,
@@ -65,8 +109,6 @@
if (!R)
return state;
- R = R->StripCasts();
-
const ElementRegion *ER = dyn_cast<ElementRegion>(R);
if (!ER)
return state;
@@ -131,13 +173,38 @@
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;
+
// 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,
*Length, One, SizeTy));
- // Check that the first buffer is sufficiently long.
- SVal BufVal = state->getSVal(FirstBuf);
+ // Check that the first buffer is sufficently long.
Loc BufStart = cast<Loc>(SV.EvalCast(BufVal, PtrTy, FirstBuf->getType()));
SVal BufEnd
= SV.EvalBinOpLN(state, BinaryOperator::Add, BufStart, LastOffset, PtrTy);
@@ -150,6 +217,10 @@
// If there's a second buffer, check it as well.
if (SecondBuf) {
BufVal = state->getSVal(SecondBuf);
+ state = CheckNonNull(C, state, SecondBuf, BufVal);
+ if (!state)
+ return NULL;
+
BufStart = cast<Loc>(SV.EvalCast(BufVal, PtrTy, SecondBuf->getType()));
BufEnd
= SV.EvalBinOpLN(state, BinaryOperator::Add, BufStart, LastOffset, PtrTy);
@@ -339,10 +410,8 @@
Name = Name.substr(10);
FnCheck EvalFunction = llvm::StringSwitch<FnCheck>(Name)
- .Case("memcpy", &CStringChecker::EvalMemcpy)
- .Case("__memcpy_chk", &CStringChecker::EvalMemcpy)
- .Case("memmove", &CStringChecker::EvalMemmove)
- .Case("__memmove_chk", &CStringChecker::EvalMemmove)
+ .Cases("memcpy", "__memcpy_chk", &CStringChecker::EvalMemcpy)
+ .Cases("memmove", "__memmove_chk", &CStringChecker::EvalMemmove)
.Case("bcopy", &CStringChecker::EvalBcopy)
.Default(NULL);
Modified: cfe/trunk/test/Analysis/bstring.c
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/bstring.c?rev=107759&r1=107758&r2=107759&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/bstring.c (original)
+++ cfe/trunk/test/Analysis/bstring.c Wed Jul 7 02:48:06 2010
@@ -1,21 +1,21 @@
// RUN: %clang_cc1 -analyze -analyzer-experimental-internal-checks -analyzer-check-objc-mem -analyzer-store=region -analyzer-experimental-checks -verify %s
// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -analyzer-experimental-internal-checks -analyzer-check-objc-mem -analyzer-store=region -analyzer-experimental-checks -verify %s
-// RUN: %clang_cc1 -analyze -DCHECK -analyzer-experimental-internal-checks -analyzer-check-objc-mem -analyzer-store=region -analyzer-experimental-checks -verify %s
-// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -DCHECK -analyzer-experimental-internal-checks -analyzer-check-objc-mem -analyzer-store=region -analyzer-experimental-checks -verify %s
+// RUN: %clang_cc1 -analyze -DVARIANT -analyzer-experimental-internal-checks -analyzer-check-objc-mem -analyzer-store=region -analyzer-experimental-checks -verify %s
+// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -DVARIANT -analyzer-experimental-internal-checks -analyzer-check-objc-mem -analyzer-store=region -analyzer-experimental-checks -verify %s
//===----------------------------------------------------------------------===
// Declarations
//===----------------------------------------------------------------------===
-// Some functions having a checking variant, which checks if there is overflow
-// using a flow-insensitive calculation of the buffer size. If CHECK is defined,
-// use those instead to make sure they are still checked by the analyzer.
+// Some functions are so similar to each other that they follow the same code
+// path, such as memcpy and __memcpy_chk. If VARIANT is defined, make sure to
+// use the variants instead to make sure they are still checked by the analyzer.
// Some functions are implemented as builtins. These should be #defined as
// BUILTIN(f), which will prepend "__builtin_" if USE_BUILTINS is defined.
-// Functions that have both checking and builtin variants should be declared
-// carefully! See memcpy() for an example.
+// Functions that have variants and are also availabe as builtins should be
+// declared carefully! See memcpy() for an example.
#ifdef USE_BUILTINS
# define BUILTIN(f) __builtin_ ## f
@@ -29,7 +29,7 @@
// memcpy()
//===----------------------------------------------------------------------===
-#ifdef CHECK
+#ifdef VARIANT
#define __memcpy_chk BUILTIN(__memcpy_chk)
void *__memcpy_chk(void *restrict s1, const void *restrict s2, size_t n,
@@ -37,12 +37,12 @@
#define memcpy(a,b,c) __memcpy_chk(a,b,c,(size_t)-1)
-#else /* CHECK */
+#else /* VARIANT */
#define memcpy BUILTIN(memcpy)
void *memcpy(void *restrict s1, const void *restrict s2, size_t n);
-#endif /* CHECK */
+#endif /* VARIANT */
void memcpy0 () {
@@ -112,23 +112,39 @@
memcpy(a+1, a+2, 4); // no-warning
}
+void memcpy10() {
+ char a[4] = {0};
+ memcpy(0, a, 4); // expected-warning{{Null pointer argument in call to byte string function}}
+}
+
+void memcpy11() {
+ char a[4] = {0};
+ memcpy(a, 0, 4); // expected-warning{{Null pointer argument in call to byte string function}}
+}
+
+void memcpy12() {
+ char a[4] = {0};
+ memcpy(0, a, 0); // no-warning
+ memcpy(a, 0, 0); // no-warning
+}
+
//===----------------------------------------------------------------------===
// memmove()
//===----------------------------------------------------------------------===
-#ifdef CHECK
+#ifdef VARIANT
#define __memmove_chk BUILTIN(__memmove_chk)
void *__memmove_chk(void *s1, const void *s2, size_t n, size_t destlen);
#define memmove(a,b,c) __memmove_chk(a,b,c,(size_t)-1)
-#else /* CHECK */
+#else /* VARIANT */
#define memmove BUILTIN(memmove)
void *memmove(void *s1, const void *s2, size_t n);
-#endif /* CHECK */
+#endif /* VARIANT */
void memmove0 () {
More information about the cfe-commits
mailing list