From: Jordy Rose Date: Mon, 20 Jun 2011 03:49:16 +0000 (+0000) Subject: [analyzer] Re-enable checking for strncpy, along with a new validation of the size... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=5e5f15062bcf4b62fda9062b453178f8b9bd0c2d;p=clang [analyzer] Re-enable checking for strncpy, along with a new validation of the size argument. strncat is not yet up-to-date, but I'm leaving it enabled for now (there shouldn't be any false positives, at least...) git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@133408 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index 9b6bcc8b88..82f2855d17 100644 --- a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -123,12 +123,15 @@ public: const Expr *FirstBuf, const Expr *SecondBuf, const char *firstMessage = NULL, - const char *secondMessage = NULL) const; + const char *secondMessage = NULL, + bool WarnAboutSize = false) const; const GRState *CheckBufferAccess(CheckerContext &C, const GRState *state, const Expr *Size, const Expr *Buf, - const char *message = NULL) const { + const char *message = NULL, + bool WarnAboutSize = false) const { // This is a convenience override. - return CheckBufferAccess(C, state, Size, Buf, NULL, message, NULL); + return CheckBufferAccess(C, state, Size, Buf, NULL, message, NULL, + WarnAboutSize); } const GRState *CheckOverlap(CheckerContext &C, const GRState *state, const Expr *Size, const Expr *First, @@ -290,7 +293,8 @@ const GRState *CStringChecker::CheckBufferAccess(CheckerContext &C, const Expr *FirstBuf, const Expr *SecondBuf, const char *firstMessage, - const char *secondMessage) const { + const char *secondMessage, + bool WarnAboutSize) const { // If a previous check has failed, propagate the failure. if (!state) return NULL; @@ -323,9 +327,11 @@ const GRState *CStringChecker::CheckBufferAccess(CheckerContext &C, // Check that the first buffer is sufficiently long. SVal BufStart = svalBuilder.evalCast(BufVal, PtrTy, FirstBuf->getType()); if (Loc *BufLoc = dyn_cast(&BufStart)) { + const Expr *warningExpr = (WarnAboutSize ? Size : FirstBuf); + SVal BufEnd = svalBuilder.evalBinOpLN(state, BO_Add, *BufLoc, LastOffset, PtrTy); - state = CheckLocation(C, state, FirstBuf, BufEnd, firstMessage); + state = CheckLocation(C, state, warningExpr, BufEnd, firstMessage); // If the buffer isn't large enough, abort. if (!state) @@ -341,9 +347,11 @@ const GRState *CStringChecker::CheckBufferAccess(CheckerContext &C, BufStart = svalBuilder.evalCast(BufVal, PtrTy, SecondBuf->getType()); if (Loc *BufLoc = dyn_cast(&BufStart)) { + const Expr *warningExpr = (WarnAboutSize ? Size : SecondBuf); + SVal BufEnd = svalBuilder.evalBinOpLN(state, BO_Add, *BufLoc, LastOffset, PtrTy); - state = CheckLocation(C, state, SecondBuf, BufEnd, secondMessage); + state = CheckLocation(C, state, warningExpr, BufEnd, secondMessage); } } @@ -1251,6 +1259,18 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, } } + // We still want to know if the bound is known to be too large. + const char * const warningMsg = + "Size argument is greater than the length of the destination buffer"; + state = CheckBufferAccess(C, state, lenExpr, Dst, warningMsg, + /* WarnAboutSize = */ true); + // FIXME: It'd be nice for this not to be a hard error, so we can warn + // about more than one thing, but the multiple calls to CheckLocation + // result in the same ExplodedNode, which means we don't keep emitting + // bug reports. + if (!state) + return; + // If we couldn't pin down the copy length, at least bound it. if (amountCopied.isUnknown()) { // Try to get a "hypothetical" string length symbol, which we can later @@ -1399,8 +1419,11 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, // string, but that's still an improvement over blank invalidation. state = InvalidateBuffer(C, state, Dst, *dstRegVal); - // Set the C string length of the destination. - state = setCStringLength(state, dstRegVal->getRegion(), finalStrLength); + // Set the C string length of the destination, if we know it. + if (!isBounded || (amountCopied == strLength)) + state = setCStringLength(state, dstRegVal->getRegion(), finalStrLength); + else + state = setCStringLength(state, dstRegVal->getRegion(), UnknownVal()); } assert(state); @@ -1589,7 +1612,7 @@ bool CStringChecker::evalCall(const CallExpr *CE, CheckerContext &C) const { .Cases("memcmp", "bcmp", &CStringChecker::evalMemcmp) .Cases("memmove", "__memmove_chk", &CStringChecker::evalMemmove) .Cases("strcpy", "__strcpy_chk", &CStringChecker::evalStrcpy) - //.Cases("strncpy", "__strncpy_chk", &CStringChecker::evalStrncpy) + .Cases("strncpy", "__strncpy_chk", &CStringChecker::evalStrncpy) .Cases("stpcpy", "__stpcpy_chk", &CStringChecker::evalStpcpy) .Cases("strcat", "__strcat_chk", &CStringChecker::evalStrcat) .Cases("strncat", "__strncat_chk", &CStringChecker::evalStrncat) diff --git a/test/Analysis/string.c b/test/Analysis/string.c index c50eca2f35..c755d5ef12 100644 --- a/test/Analysis/string.c +++ b/test/Analysis/string.c @@ -456,6 +456,105 @@ void strcat_too_big(char *dst, char *src) { } +//===----------------------------------------------------------------------=== +// strncpy() +//===----------------------------------------------------------------------=== + +#ifdef VARIANT + +#define __strncpy_chk BUILTIN(__strncpy_chk) +char *__strncpy_chk(char *restrict s1, const char *restrict s2, size_t n, size_t destlen); + +#define strncpy(a,b,n) __strncpy_chk(a,b,n,(size_t)-1) + +#else /* VARIANT */ + +#define strncpy BUILTIN(strncpy) +char *strncpy(char *restrict s1, const char *restrict s2, size_t n); + +#endif /* VARIANT */ + + +void strncpy_null_dst(char *x) { + strncpy(NULL, x, 5); // expected-warning{{Null pointer argument in call to string copy function}} +} + +void strncpy_null_src(char *x) { + strncpy(x, NULL, 5); // expected-warning{{Null pointer argument in call to string copy function}} +} + +void strncpy_fn(char *x) { + strncpy(x, (char*)&strcpy_fn, 5); // expected-warning{{Argument to string copy function is the address of the function 'strcpy_fn', which is not a null-terminated string}} +} + +void strncpy_effects(char *x, char *y) { + char a = x[0]; + + if (strncpy(x, y, 5) != x) + (void)*(char*)0; // no-warning + + if (strlen(x) != strlen(y)) + (void)*(char*)0; // expected-warning{{null}} + + if (a != x[0]) + (void)*(char*)0; // expected-warning{{null}} +} + +void strncpy_overflow(char *y) { + char x[4]; + if (strlen(y) == 4) + strncpy(x, y, 5); // expected-warning{{Size argument is greater than the length of the destination buffer}} +} + +void strncpy_no_overflow(char *y) { + char x[4]; + if (strlen(y) == 3) + strncpy(x, y, 5); // expected-warning{{Size argument is greater than the length of the destination buffer}} +} + +void strncpy_no_overflow2(char *y, int n) { + if (n <= 4) + return; + + char x[4]; + if (strlen(y) == 3) + strncpy(x, y, n); // expected-warning{{Size argument is greater than the length of the destination buffer}} +} + +void strncpy_truncate(char *y) { + char x[4]; + if (strlen(y) == 4) + strncpy(x, y, 3); // no-warning +} + +void strncpy_no_truncate(char *y) { + char x[4]; + if (strlen(y) == 3) + strncpy(x, y, 3); // no-warning +} + +void strncpy_exactly_matching_buffer(char *y) { + char x[4]; + strncpy(x, y, 4); // no-warning + + // strncpy does not null-terminate, so we have no idea what the strlen is + // after this. + if (strlen(x) > 4) + (void)*(int*)0; // expected-warning{{null}} +} + +void strncpy_exactly_matching_buffer2(char *y) { + if (strlen(y) >= 4) + return; + + char x[4]; + strncpy(x, y, 4); // no-warning + + // This time, we know that y fits in x anyway. + if (strlen(x) > 3) + (void)*(int*)0; // no-warning +} + //===----------------------------------------------------------------------=== // strncat() //===----------------------------------------------------------------------===