From: Jordy Rose Date: Tue, 14 Jun 2011 01:15:31 +0000 (+0000) Subject: [analyzer] Fix modeling of strnlen to be more conservative. Move tests we can't prope... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=793bff3fb7ca2a31e81aa7f4f3f21f921459010b;p=clang [analyzer] Fix modeling of strnlen to be more conservative. Move tests we can't properly model (yet?) to string-fail.c. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@132955 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp index f2f5c1ed44..1c9064e40d 100644 --- a/lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -903,10 +903,35 @@ void CStringChecker::evalstrnLength(CheckerContext &C, void CStringChecker::evalstrLengthCommon(CheckerContext &C, const CallExpr *CE, bool IsStrnlen) const { const GRState *state = C.getState(); + + if (IsStrnlen) { + const Expr *maxlenExpr = CE->getArg(1); + SVal maxlenVal = state->getSVal(maxlenExpr); + + const GRState *stateZeroSize, *stateNonZeroSize; + llvm::tie(stateZeroSize, stateNonZeroSize) = + assumeZero(C, state, maxlenVal, maxlenExpr->getType()); + + // If the size can be zero, the result will be 0 in that case, and we don't + // have to check the string itself. + if (stateZeroSize) { + SVal zero = C.getSValBuilder().makeZeroVal(CE->getType()); + stateZeroSize = stateZeroSize->BindExpr(CE, zero); + C.addTransition(stateZeroSize); + } + + // If the size is GUARANTEED to be zero, we're done! + if (!stateNonZeroSize) + return; + + // Otherwise, record the assumption that the size is nonzero. + state = stateNonZeroSize; + } + + // Check that the string argument is non-null. const Expr *Arg = CE->getArg(0); SVal ArgVal = state->getSVal(Arg); - // Check that the argument is non-null. state = checkNonNull(C, state, Arg, ArgVal); if (state) { @@ -917,41 +942,82 @@ void CStringChecker::evalstrLengthCommon(CheckerContext &C, const CallExpr *CE, if (strLength.isUndef()) return; + DefinedOrUnknownSVal result = UnknownVal(); + // If the check is for strnlen() then bind the return value to no more than // the maxlen value. if (IsStrnlen) { + QualType cmpTy = C.getSValBuilder().getContext().IntTy; + + // It's a little unfortunate to be getting this again, + // but it's not that expensive... const Expr *maxlenExpr = CE->getArg(1); SVal maxlenVal = state->getSVal(maxlenExpr); - + NonLoc *strLengthNL = dyn_cast(&strLength); NonLoc *maxlenValNL = dyn_cast(&maxlenVal); - QualType cmpTy = C.getSValBuilder().getContext().IntTy; - const GRState *stateTrue, *stateFalse; - - // Check if the strLength is greater than or equal to the maxlen - llvm::tie(stateTrue, stateFalse) = - state->assume(cast - (C.getSValBuilder().evalBinOpNN(state, BO_GE, - *strLengthNL, *maxlenValNL, - cmpTy))); - - // If the strLength is greater than or equal to the maxlen, set strLength - // to maxlen - if (stateTrue && !stateFalse) { - strLength = maxlenVal; + if (strLengthNL && maxlenValNL) { + const GRState *stateStringTooLong, *stateStringNotTooLong; + + // Check if the strLength is greater than the maxlen. + llvm::tie(stateStringTooLong, stateStringNotTooLong) = + state->assume(cast + (C.getSValBuilder().evalBinOpNN(state, BO_GT, + *strLengthNL, + *maxlenValNL, + cmpTy))); + + if (stateStringTooLong && !stateStringNotTooLong) { + // If the string is longer than maxlen, return maxlen. + result = *maxlenValNL; + } else if (stateStringNotTooLong && !stateStringTooLong) { + // If the string is shorter than maxlen, return its length. + result = *strLengthNL; + } } - } - // If getCStringLength couldn't figure out the length, conjure a return - // value, so it can be used in constraints, at least. - if (strLength.isUnknown()) { - unsigned Count = C.getNodeBuilder().getCurrentBlockCount(); - strLength = C.getSValBuilder().getConjuredSymbolVal(NULL, CE, Count); + if (result.isUnknown()) { + // If we don't have enough information for a comparison, there's + // no guarantee the full string length will actually be returned. + // All we know is the return value is the min of the string length + // and the limit. This is better than nothing. + unsigned Count = C.getNodeBuilder().getCurrentBlockCount(); + result = C.getSValBuilder().getConjuredSymbolVal(NULL, CE, Count); + NonLoc *resultNL = cast(&result); + + if (strLengthNL) { + state = state->assume(cast + (C.getSValBuilder().evalBinOpNN(state, BO_LE, + *resultNL, + *strLengthNL, + cmpTy)), true); + } + + if (maxlenValNL) { + state = state->assume(cast + (C.getSValBuilder().evalBinOpNN(state, BO_LE, + *resultNL, + *maxlenValNL, + cmpTy)), true); + } + } + + } else { + // This is a plain strlen(), not strnlen(). + result = cast(strLength); + + // If we don't know the length of the string, conjure a return + // value, so it can be used in constraints, at least. + if (result.isUnknown()) { + unsigned Count = C.getNodeBuilder().getCurrentBlockCount(); + result = C.getSValBuilder().getConjuredSymbolVal(NULL, CE, Count); + } } // Bind the return value. - state = state->BindExpr(CE, strLength); + assert(!result.isUnknown() && "Should have conjured a value by now"); + state = state->BindExpr(CE, result); C.addTransition(state); } } diff --git a/test/Analysis/string-fail.c b/test/Analysis/string-fail.c new file mode 100644 index 0000000000..bdd46642c5 --- /dev/null +++ b/test/Analysis/string-fail.c @@ -0,0 +1,113 @@ +// RUN: %clang_cc1 -analyze -analyzer-checker=core,cplusplus.experimental.CString,deadcode.experimental.UnreachableCode -analyzer-store=region -Wno-null-dereference -verify %s +// RUN: %clang_cc1 -analyze -DUSE_BUILTINS -analyzer-checker=core,cplusplus.experimental.CString,deadcode.experimental.UnreachableCode -analyzer-store=region -Wno-null-dereference -verify %s +// XFAIL: * + +// This file is for tests that may eventually go into string.c, or may be +// deleted outright. At one point these tests passed, but only because we +// weren't correctly modelling the behavior of the relevant string functions. +// The tests aren't incorrect, but require the analyzer to be smarter about +// conjured values than it currently is. + +//===----------------------------------------------------------------------=== +// Declarations +//===----------------------------------------------------------------------=== + +// Some functions are so similar to each other that they follow the same code +// path, such as memcpy and __memcpy_chk, or memcmp and bcmp. 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 variants and are also available as builtins should be +// declared carefully! See memcpy() for an example. + +#ifdef USE_BUILTINS +# define BUILTIN(f) __builtin_ ## f +#else /* USE_BUILTINS */ +# define BUILTIN(f) f +#endif /* USE_BUILTINS */ + +#define NULL 0 +typedef typeof(sizeof(int)) size_t; + + +//===----------------------------------------------------------------------=== +// strnlen() +//===----------------------------------------------------------------------=== + +#define strnlen BUILTIN(strnlen) +size_t strnlen(const char *s, size_t maxlen); + +void strnlen_liveness(const char *x) { + if (strnlen(x, 10) < 5) + return; + if (strnlen(x, 10) < 5) + (void)*(char*)0; // no-warning +} + +void strnlen_subregion() { + struct two_stringsn { char a[2], b[2]; }; + extern void use_two_stringsn(struct two_stringsn *); + + struct two_stringsn z; + use_two_stringsn(&z); + + size_t a = strnlen(z.a, 10); + z.b[0] = 5; + size_t b = strnlen(z.a, 10); + if (a == 0 && b != 0) + (void)*(char*)0; // expected-warning{{never executed}} + + use_two_stringsn(&z); + + size_t c = strnlen(z.a, 10); + if (a == 0 && c != 0) + (void)*(char*)0; // expected-warning{{null}} +} + +extern void use_stringn(char *); +void strnlen_argument(char *x) { + size_t a = strnlen(x, 10); + size_t b = strnlen(x, 10); + if (a == 0 && b != 0) + (void)*(char*)0; // expected-warning{{never executed}} + + use_stringn(x); + + size_t c = strnlen(x, 10); + if (a == 0 && c != 0) + (void)*(char*)0; // expected-warning{{null}} +} + +extern char global_strn[]; +void strnlen_global() { + size_t a = strnlen(global_strn, 10); + size_t b = strnlen(global_strn, 10); + if (a == 0 && b != 0) + (void)*(char*)0; // expected-warning{{never executed}} + + // Call a function with unknown effects, which should invalidate globals. + use_stringn(0); + + size_t c = strnlen(global_strn, 10); + if (a == 0 && c != 0) + (void)*(char*)0; // expected-warning{{null}} +} + +void strnlen_indirect(char *x) { + size_t a = strnlen(x, 10); + char *p = x; + char **p2 = &p; + size_t b = strnlen(x, 10); + if (a == 0 && b != 0) + (void)*(char*)0; // expected-warning{{never executed}} + + extern void use_stringn_ptr(char*const*); + use_stringn_ptr(p2); + + size_t c = strnlen(x, 10); + if (a == 0 && c != 0) + (void)*(char*)0; // expected-warning{{null}} +} diff --git a/test/Analysis/string.c b/test/Analysis/string.c index e6baf51c47..ac1026c414 100644 --- a/test/Analysis/string.c +++ b/test/Analysis/string.c @@ -199,76 +199,59 @@ label: return strnlen((char*)&&label, 3); // expected-warning{{Argument to byte string function is the address of the label 'label', which is not a null-terminated string}} } -void strnlen_subregion() { - struct two_stringsn { char a[2], b[2]; }; - extern void use_two_stringsn(struct two_stringsn *); - - struct two_stringsn z; - use_two_stringsn(&z); +void strnlen_zero() { + if (strnlen("abc", 0) != 0) + (void)*(char*)0; // no-warning + if (strnlen(NULL, 0) != 0) // no-warning + (void)*(char*)0; // no-warning +} - size_t a = strnlen(z.a, 10); - z.b[0] = 5; - size_t b = strnlen(z.a, 10); - if (a == 0 && b != 0) - (void)*(char*)0; // expected-warning{{never executed}} +size_t strnlen_compound_literal() { + // This used to crash because we don't model the string lengths of + // compound literals. + return strnlen((char[]) { 'a', 'b', 0 }, 1); +} - use_two_stringsn(&z); +size_t strnlen_unknown_limit(float f) { + // This used to crash because we don't model the integer values of floats. + return strnlen("abc", (int)f); +} - size_t c = strnlen(z.a, 10); - if (a == 0 && c != 0) +void strnlen_is_not_strlen(char *x) { + if (strnlen(x, 10) != strlen(x)) (void)*(char*)0; // expected-warning{{null}} } -extern void use_stringn(char *); -void strnlen_argument(char *x) { - size_t a = strnlen(x, 10); - size_t b = strnlen(x, 10); - if (a == 0 && b != 0) +void strnlen_at_limit(char *x) { + size_t len = strnlen(x, 10); + if (len > 10) (void)*(char*)0; // expected-warning{{never executed}} - - use_stringn(x); - - size_t c = strnlen(x, 10); - if (a == 0 && c != 0) - (void)*(char*)0; // expected-warning{{null}} + if (len == 10) + (void)*(char*)0; // expected-warning{{null}} } -extern char global_strn[]; -void strnlen_global() { - size_t a = strnlen(global_strn, 10); - size_t b = strnlen(global_strn, 10); - if (a == 0 && b != 0) +void strnlen_less_than_limit(char *x) { + size_t len = strnlen(x, 10); + if (len > 10) (void)*(char*)0; // expected-warning{{never executed}} - - // Call a function with unknown effects, which should invalidate globals. - use_stringn(0); - - size_t c = strnlen(global_str, 10); - if (a == 0 && c != 0) - (void)*(char*)0; // expected-warning{{null}} + if (len < 10) + (void)*(char*)0; // expected-warning{{null}} } -void strnlen_indirect(char *x) { - size_t a = strnlen(x, 10); - char *p = x; - char **p2 = &p; - size_t b = strnlen(x, 10); - if (a == 0 && b != 0) +void strnlen_at_actual(size_t limit) { + size_t len = strnlen("abc", limit); + if (len > 3) (void)*(char*)0; // expected-warning{{never executed}} - - extern void use_stringn_ptr(char*const*); - use_stringn_ptr(p2); - - size_t c = strnlen(x, 10); - if (a == 0 && c != 0) + if (len == 3) (void)*(char*)0; // expected-warning{{null}} } -void strnlen_liveness(const char *x) { - if (strnlen(x, 10) < 5) - return; - if (strnlen(x, 10) < 5) - (void)*(char*)0; // no-warning +void strnlen_less_than_actual(size_t limit) { + size_t len = strnlen("abc", limit); + if (len > 3) + (void)*(char*)0; // expected-warning{{never executed}} + if (len < 3) + (void)*(char*)0; // expected-warning{{null}} } //===----------------------------------------------------------------------===