From: Artem Dergachev Date: Fri, 13 Oct 2017 20:11:00 +0000 (+0000) Subject: [analyzer] CStringChecker: pr34460: Avoid a crash when a cast is not modeled. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=89ba86c158ce787d14f0352b2bffc6bacd71d02e;p=clang [analyzer] CStringChecker: pr34460: Avoid a crash when a cast is not modeled. The checker used to crash when a mempcpy's length argument is symbolic. In this case the cast from 'void *' to 'char *' failed because the respective ElementRegion that represents cast is hard to add on top of the existing ElementRegion that represents the offset to the last copied byte, while preseving a sane memory region structure. Additionally, a few test cases are added (to casts.c) which demonstrate problems caused by existing sloppy work we do with multi-layer ElementRegions. If said cast would be modeled properly in the future, these tests would need to be taken into account. Differential Revision: https://reviews.llvm.org/D38797 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@315742 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 77c24629d7..58218df238 100644 --- a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -1050,31 +1050,22 @@ void CStringChecker::evalCopyCommon(CheckerContext &C, // If this is mempcpy, get the byte after the last byte copied and // bind the expr. if (IsMempcpy) { - loc::MemRegionVal destRegVal = destVal.castAs(); - - // Get the length to copy. - if (Optional lenValNonLoc = sizeVal.getAs()) { - // Get the byte after the last byte copied. - SValBuilder &SvalBuilder = C.getSValBuilder(); - ASTContext &Ctx = SvalBuilder.getContext(); - QualType CharPtrTy = Ctx.getPointerType(Ctx.CharTy); - loc::MemRegionVal DestRegCharVal = SvalBuilder.evalCast(destRegVal, - CharPtrTy, Dest->getType()).castAs(); - SVal lastElement = C.getSValBuilder().evalBinOpLN(state, BO_Add, - DestRegCharVal, - *lenValNonLoc, - Dest->getType()); - - // The byte after the last byte copied is the return value. - state = state->BindExpr(CE, LCtx, lastElement); - } else { - // If we don't know how much we copied, we can at least - // conjure a return value for later. - SVal result = C.getSValBuilder().conjureSymbolVal(nullptr, CE, LCtx, + // Get the byte after the last byte copied. + SValBuilder &SvalBuilder = C.getSValBuilder(); + ASTContext &Ctx = SvalBuilder.getContext(); + QualType CharPtrTy = Ctx.getPointerType(Ctx.CharTy); + SVal DestRegCharVal = + SvalBuilder.evalCast(destVal, CharPtrTy, Dest->getType()); + SVal lastElement = C.getSValBuilder().evalBinOp( + state, BO_Add, DestRegCharVal, sizeVal, Dest->getType()); + // If we don't know how much we copied, we can at least + // conjure a return value for later. + if (lastElement.isUnknown()) + lastElement = C.getSValBuilder().conjureSymbolVal(nullptr, CE, LCtx, C.blockCount()); - state = state->BindExpr(CE, LCtx, result); - } + // The byte after the last byte copied is the return value. + state = state->BindExpr(CE, LCtx, lastElement); } else { // All other copies return the destination buffer. // (Well, bcopy() has a void return type, but this won't hurt.) diff --git a/test/Analysis/bstring.cpp b/test/Analysis/bstring.cpp index a6d7b40162..fea76cc082 100644 --- a/test/Analysis/bstring.cpp +++ b/test/Analysis/bstring.cpp @@ -1,8 +1,35 @@ // RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -verify %s +// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -verify %s +// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -verify %s +// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -verify %s #include "Inputs/system-header-simulator-cxx.h" #include "Inputs/system-header-simulator-for-malloc.h" +// This provides us with four possible mempcpy() definitions. +// See also comments in bstring.c. + +#ifdef USE_BUILTINS +#define BUILTIN(f) __builtin_##f +#else /* USE_BUILTINS */ +#define BUILTIN(f) f +#endif /* USE_BUILTINS */ + +#ifdef VARIANT + +#define __mempcpy_chk BUILTIN(__mempcpy_chk) +void *__mempcpy_chk(void *__restrict__ s1, const void *__restrict__ s2, + size_t n, size_t destlen); + +#define mempcpy(a,b,c) __mempcpy_chk(a,b,c,(size_t)-1) + +#else /* VARIANT */ + +#define mempcpy BUILTIN(mempcpy) +void *mempcpy(void *__restrict__ s1, const void *__restrict__ s2, size_t n); + +#endif /* VARIANT */ + void clang_analyzer_eval(int); int *testStdCopyInvalidatesBuffer(std::vector v) { @@ -36,3 +63,17 @@ int *testStdCopyBackwardInvalidatesBuffer(std::vector v) { return buf; } + +namespace pr34460 { +short a; +class b { + int c; + long g; + void d() { + int e = c; + f += e; + mempcpy(f, &a, g); + } + unsigned *f; +}; +} diff --git a/test/Analysis/casts.c b/test/Analysis/casts.c index 3ba12e4318..24bba8a30a 100644 --- a/test/Analysis/casts.c +++ b/test/Analysis/casts.c @@ -123,3 +123,29 @@ void locAsIntegerCasts(void *p) { int x = (int) p; clang_analyzer_eval(++x < 10); // no-crash // expected-warning{{UNKNOWN}} } + +void multiDimensionalArrayPointerCasts() { + static int x[10][10]; + int *y1 = &(x[3][5]); + char *z = ((char *) y1) + 2; + int *y2 = (int *)(z - 2); + int *y3 = ((int *)x) + 35; // This is offset for [3][5]. + + clang_analyzer_eval(y1 == y2); // expected-warning{{TRUE}} + + // FIXME: should be FALSE (i.e. equal pointers). + clang_analyzer_eval(y1 - y2); // expected-warning{{UNKNOWN}} + // FIXME: should be TRUE (i.e. same symbol). + clang_analyzer_eval(*y1 == *y2); // expected-warning{{UNKNOWN}} + + clang_analyzer_eval(*((char *)y1) == *((char *) y2)); // expected-warning{{TRUE}} + + clang_analyzer_eval(y1 == y3); // expected-warning{{TRUE}} + + // FIXME: should be FALSE (i.e. equal pointers). + clang_analyzer_eval(y1 - y3); // expected-warning{{UNKNOWN}} + // FIXME: should be TRUE (i.e. same symbol). + clang_analyzer_eval(*y1 == *y3); // expected-warning{{UNKNOWN}} + + clang_analyzer_eval(*((char *)y1) == *((char *) y3)); // expected-warning{{TRUE}} +}