From: Jordy Rose Date: Wed, 7 Jul 2010 07:48:06 +0000 (+0000) Subject: Cleanup on CStringChecker and its associated tests. Also check for null arguments... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=a6b808c6ba57723b997da2ef7a4a8cf48fbc2ba8;p=clang Cleanup on CStringChecker and its associated tests. Also check for null arguments...which are allowed if the access length is 0! git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@107759 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Checker/CStringChecker.cpp b/lib/Checker/CStringChecker.cpp index 26e83dfa23..cb30c1ba05 100644 --- a/lib/Checker/CStringChecker.cpp +++ b/lib/Checker/CStringChecker.cpp @@ -38,6 +38,8 @@ public: 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 @@ void clang::RegisterCStringChecker(GRExprEngine &Eng) { 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(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(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 @@ const GRState *CStringChecker::CheckLocation(CheckerContext &C, if (!R) return state; - R = R->StripCasts(); - const ElementRegion *ER = dyn_cast(R); if (!ER) return state; @@ -131,13 +173,38 @@ const GRState *CStringChecker::CheckBufferAccess(CheckerContext &C, 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(VM.makeIntVal(1, SizeTy)); NonLoc LastOffset = cast(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(SV.EvalCast(BufVal, PtrTy, FirstBuf->getType())); SVal BufEnd = SV.EvalBinOpLN(state, BinaryOperator::Add, BufStart, LastOffset, PtrTy); @@ -150,6 +217,10 @@ const GRState *CStringChecker::CheckBufferAccess(CheckerContext &C, // 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(SV.EvalCast(BufVal, PtrTy, SecondBuf->getType())); BufEnd = SV.EvalBinOpLN(state, BinaryOperator::Add, BufStart, LastOffset, PtrTy); @@ -339,10 +410,8 @@ bool CStringChecker::EvalCallExpr(CheckerContext &C, const CallExpr *CE) { Name = Name.substr(10); FnCheck EvalFunction = llvm::StringSwitch(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); diff --git a/test/Analysis/bstring.c b/test/Analysis/bstring.c index ac8646d8db..467b87b878 100644 --- a/test/Analysis/bstring.c +++ b/test/Analysis/bstring.c @@ -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 @@ typedef typeof(sizeof(int)) size_t; // 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 @@ void *__memcpy_chk(void *restrict s1, const void *restrict s2, size_t n, #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 @@ void memcpy9() { 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 () {