Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/CStringChecker.cpp +++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp @@ -97,14 +97,17 @@ void evalStrcpy(CheckerContext &C, const CallExpr *CE) const; void evalStrncpy(CheckerContext &C, const CallExpr *CE) const; void evalStpcpy(CheckerContext &C, const CallExpr *CE) const; + void evalStrlcpy(CheckerContext &C, const CallExpr *CE) const; void evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, bool returnEnd, bool isBounded, - bool isAppending) const; + bool isAppending, + bool returnPtr = true) const; void evalStrcat(CheckerContext &C, const CallExpr *CE) const; void evalStrncat(CheckerContext &C, const CallExpr *CE) const; + void evalStrlcat(CheckerContext &C, const CallExpr *CE) const; void evalStrcmp(CheckerContext &C, const CallExpr *CE) const; void evalStrncmp(CheckerContext &C, const CallExpr *CE) const; @@ -1393,6 +1396,18 @@ /* isAppending = */ false); } +void CStringChecker::evalStrlcpy(CheckerContext &C, const CallExpr *CE) const { + if (CE->getNumArgs() < 3) + return; + + // char *strlcpy(char *dst, const char *src, size_t n); + evalStrcpyCommon(C, CE, + /* returnEnd = */ true, + /* isBounded = */ true, + /* isAppending = */ false, + /* returnPtr = */ false); +} + void CStringChecker::evalStrcat(CheckerContext &C, const CallExpr *CE) const { if (CE->getNumArgs() < 2) return; @@ -1415,9 +1430,21 @@ /* isAppending = */ true); } +void CStringChecker::evalStrlcat(CheckerContext &C, const CallExpr *CE) const { + if (CE->getNumArgs() < 3) + return; + + //char *strlcat(char *s1, const char *s2, size_t n); + evalStrcpyCommon(C, CE, + /* returnEnd = */ false, + /* isBounded = */ true, + /* isAppending = */ true, + /* returnPtr = */ false); +} + void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, bool returnEnd, bool isBounded, - bool isAppending) const { + bool isAppending, bool returnPtr) const { CurrentFunctionDescription = "string copy function"; ProgramStateRef state = C.getState(); const LocationContext *LCtx = C.getLocationContext(); @@ -1455,6 +1482,11 @@ SVal maxLastElementIndex = UnknownVal(); const char *boundWarning = nullptr; + state = CheckOverlap(C, state, isBounded ? CE->getArg(2) : CE->getArg(1), Dst, srcExpr); + + if (!state) + return; + // If the function is strncpy, strncat, etc... it is bounded. if (isBounded) { // Get the max number of characters to copy. @@ -1658,16 +1690,22 @@ finalStrLength = amountCopied; } - // The final result of the function will either be a pointer past the last - // copied element, or a pointer to the start of the destination buffer. - SVal Result = (returnEnd ? UnknownVal() : DstVal); + SVal Result; + + if (returnPtr) { + // The final result of the function will either be a pointer past the last + // copied element, or a pointer to the start of the destination buffer. + Result = (returnEnd ? UnknownVal() : DstVal); + } else { + Result = finalStrLength; + } assert(state); // If the destination is a MemRegion, try to check for a buffer overflow and // record the new string length. if (Optional dstRegVal = - DstVal.getAs()) { + DstVal.getAs()) { QualType ptrTy = Dst->getType(); // If we have an exact value on a bounded copy, use that to check for @@ -1675,9 +1713,9 @@ if (boundWarning) { if (Optional maxLastNL = maxLastElementIndex.getAs()) { SVal maxLastElement = svalBuilder.evalBinOpLN(state, BO_Add, *dstRegVal, - *maxLastNL, ptrTy); + *maxLastNL, ptrTy); state = CheckLocation(C, state, CE->getArg(2), maxLastElement, - boundWarning); + boundWarning); if (!state) return; } @@ -1686,7 +1724,7 @@ // Then, if the final length is known... if (Optional knownStrLength = finalStrLength.getAs()) { SVal lastElement = svalBuilder.evalBinOpLN(state, BO_Add, *dstRegVal, - *knownStrLength, ptrTy); + *knownStrLength, ptrTy); // ...and we haven't checked the bound, we'll check the actual copy. if (!boundWarning) { @@ -1698,7 +1736,7 @@ } // If this is a stpcpy-style copy, the last element is the return value. - if (returnEnd) + if (returnPtr && returnEnd) Result = lastElement; } @@ -1710,12 +1748,12 @@ // This would probably remove any existing bindings past the end of the // string, but that's still an improvement over blank invalidation. state = InvalidateBuffer(C, state, Dst, *dstRegVal, - /*IsSourceBuffer*/false, nullptr); + /*IsSourceBuffer*/false, nullptr); // Invalidate the source (const-invalidation without const-pointer-escaping // the address of the top-level region). state = InvalidateBuffer(C, state, srcExpr, srcVal, /*IsSourceBuffer*/true, - nullptr); + nullptr); // Set the C string length of the destination, if we know it. if (isBounded && !isAppending) { @@ -1731,12 +1769,13 @@ assert(state); - // If this is a stpcpy-style copy, but we were unable to check for a buffer - // overflow, we still need a result. Conjure a return value. - if (returnEnd && Result.isUnknown()) { - Result = svalBuilder.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount()); + if (returnPtr) { + // If this is a stpcpy-style copy, but we were unable to check for a buffer + // overflow, we still need a result. Conjure a return value. + if (returnEnd && Result.isUnknown()) { + Result = svalBuilder.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount()); + } } - // Set the return value. state = state->BindExpr(CE, LCtx, Result); C.addTransition(state); @@ -1759,7 +1798,7 @@ } void CStringChecker::evalStrcasecmp(CheckerContext &C, - const CallExpr *CE) const { + const CallExpr *CE) const { if (CE->getNumArgs() < 2) return; @@ -1768,7 +1807,7 @@ } void CStringChecker::evalStrncasecmp(CheckerContext &C, - const CallExpr *CE) const { + const CallExpr *CE) const { if (CE->getNumArgs() < 3) return; @@ -1777,7 +1816,7 @@ } void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE, - bool isBounded, bool ignoreCase) const { + bool isBounded, bool ignoreCase) const { CurrentFunctionDescription = "string comparison function"; ProgramStateRef state = C.getState(); const LocationContext *LCtx = C.getLocationContext(); @@ -1822,7 +1861,7 @@ // and we only need to check one size. if (StSameBuf) { StSameBuf = StSameBuf->BindExpr(CE, LCtx, - svalBuilder.makeZeroVal(CE->getType())); + svalBuilder.makeZeroVal(CE->getType())); C.addTransition(StSameBuf); // If the two arguments are GUARANTEED to be the same, we're done! @@ -1841,7 +1880,7 @@ const StringLiteral *s2StrLiteral = getCStringLiteral(C, state, s2, s2Val); bool canComputeResult = false; SVal resultVal = svalBuilder.conjureSymbolVal(nullptr, CE, LCtx, - C.blockCount()); + C.blockCount()); if (s1StrLiteral && s2StrLiteral) { StringRef s1StrRef = s1StrLiteral->getString(); @@ -1876,7 +1915,7 @@ // Use StringRef's comparison methods to compute the actual result. int compareRes = ignoreCase ? s1StrRef.compare_lower(s2StrRef) - : s1StrRef.compare(s2StrRef); + : s1StrRef.compare(s2StrRef); // The strcmp function returns an integer greater than, equal to, or less // than zero, [c11, p7.24.4.2]. @@ -1890,7 +1929,7 @@ BinaryOperatorKind op = (compareRes == 1) ? BO_GT : BO_LT; SVal compareWithZero = svalBuilder.evalBinOp(state, op, resultVal, zeroVal, - svalBuilder.getConditionType()); + svalBuilder.getConditionType()); DefinedSVal compareWithZeroVal = compareWithZero.castAs(); state = state->assume(compareWithZeroVal, true); } @@ -1942,17 +1981,17 @@ // Invalidate the search string, representing the change of one delimiter // character to NUL. State = InvalidateBuffer(C, State, SearchStrPtr, Result, - /*IsSourceBuffer*/false, nullptr); + /*IsSourceBuffer*/false, nullptr); // Overwrite the search string pointer. The new value is either an address // further along in the same string, or NULL if there are no more tokens. State = State->bindLoc(*SearchStrLoc, - SVB.conjureSymbolVal(getTag(), - CE, - LCtx, - CharPtrTy, - C.blockCount()), - LCtx); + SVB.conjureSymbolVal(getTag(), + CE, + LCtx, + CharPtrTy, + C.blockCount()), + LCtx); } else { assert(SearchStrVal.isUnknown()); // Conjure a symbolic value. It's the best we can do. @@ -1970,12 +2009,12 @@ } void CStringChecker::evalStdCopyBackward(CheckerContext &C, - const CallExpr *CE) const { + const CallExpr *CE) const { evalStdCopyCommon(C, CE); } void CStringChecker::evalStdCopyCommon(CheckerContext &C, - const CallExpr *CE) const { + const CallExpr *CE) const { if (CE->getNumArgs() < 3) return; @@ -1992,7 +2031,7 @@ const Expr *Dst = CE->getArg(2); SVal DstVal = State->getSVal(Dst, LCtx); State = InvalidateBuffer(C, State, Dst, DstVal, /*IsSource=*/false, - /*Size=*/nullptr); + /*Size=*/nullptr); SValBuilder &SVB = C.getSValBuilder(); @@ -2042,7 +2081,7 @@ if (!State) return; State = InvalidateBuffer(C, State, Mem, C.getSVal(Mem), - /*IsSourceBuffer*/false, Size); + /*IsSourceBuffer*/false, Size); if (!State) return; @@ -2091,10 +2130,14 @@ evalFunction = &CStringChecker::evalStrncpy; else if (C.isCLibraryFunction(FDecl, "stpcpy")) evalFunction = &CStringChecker::evalStpcpy; + else if (C.isCLibraryFunction(FDecl, "strlcpy")) + evalFunction = &CStringChecker::evalStrlcpy; else if (C.isCLibraryFunction(FDecl, "strcat")) evalFunction = &CStringChecker::evalStrcat; else if (C.isCLibraryFunction(FDecl, "strncat")) evalFunction = &CStringChecker::evalStrncat; + else if (C.isCLibraryFunction(FDecl, "strlcat")) + evalFunction = &CStringChecker::evalStrlcat; else if (C.isCLibraryFunction(FDecl, "strlen")) evalFunction = &CStringChecker::evalstrLength; else if (C.isCLibraryFunction(FDecl, "strnlen")) @@ -2161,7 +2204,7 @@ SVal StrVal = C.getSVal(Init); assert(StrVal.isValid() && "Initializer string is unknown or undefined"); DefinedOrUnknownSVal strLength = - getCStringLength(C, state, Init, StrVal).castAs(); + getCStringLength(C, state, Init, StrVal).castAs(); state = state->set(MR, strLength); } @@ -2171,11 +2214,11 @@ ProgramStateRef CStringChecker::checkRegionChanges(ProgramStateRef state, - const InvalidatedSymbols *, - ArrayRef ExplicitRegions, - ArrayRef Regions, - const LocationContext *LCtx, - const CallEvent *Call) const { + const InvalidatedSymbols *, + ArrayRef ExplicitRegions, + ArrayRef Regions, + const LocationContext *LCtx, + const CallEvent *Call) const { CStringLengthTy Entries = state->get(); if (Entries.isEmpty()) return state; @@ -2185,7 +2228,7 @@ // First build sets for the changed regions and their super-regions. for (ArrayRef::iterator - I = Regions.begin(), E = Regions.end(); I != E; ++I) { + I = Regions.begin(), E = Regions.end(); I != E; ++I) { const MemRegion *MR = *I; Invalidated.insert(MR); @@ -2200,7 +2243,7 @@ // Then loop over the entries in the current state. for (CStringLengthTy::iterator I = Entries.begin(), - E = Entries.end(); I != E; ++I) { + E = Entries.end(); I != E; ++I) { const MemRegion *MR = I.getKey(); // Is this entry for a super-region of a changed region? @@ -2224,22 +2267,22 @@ } void CStringChecker::checkLiveSymbols(ProgramStateRef state, - SymbolReaper &SR) const { + SymbolReaper &SR) const { // Mark all symbols in our string length map as valid. CStringLengthTy Entries = state->get(); for (CStringLengthTy::iterator I = Entries.begin(), E = Entries.end(); - I != E; ++I) { + I != E; ++I) { SVal Len = I.getData(); for (SymExpr::symbol_iterator si = Len.symbol_begin(), - se = Len.symbol_end(); si != se; ++si) + se = Len.symbol_end(); si != se; ++si) SR.markInUse(*si); } } void CStringChecker::checkDeadSymbols(SymbolReaper &SR, - CheckerContext &C) const { + CheckerContext &C) const { if (!SR.hasDeadSymbols()) return; @@ -2250,7 +2293,7 @@ CStringLengthTy::Factory &F = state->get_context(); for (CStringLengthTy::iterator I = Entries.begin(), E = Entries.end(); - I != E; ++I) { + I != E; ++I) { SVal Len = I.getData(); if (SymbolRef Sym = Len.getAsSymbol()) { if (SR.isDead(Sym)) @@ -2269,11 +2312,11 @@ checker->Filter.CheckName##name = mgr.getCurrentCheckName(); \ } -REGISTER_CHECKER(CStringNullArg) -REGISTER_CHECKER(CStringOutOfBounds) -REGISTER_CHECKER(CStringBufferOverlap) + REGISTER_CHECKER(CStringNullArg) + REGISTER_CHECKER(CStringOutOfBounds) + REGISTER_CHECKER(CStringBufferOverlap) REGISTER_CHECKER(CStringNotNullTerm) -void ento::registerCStringCheckerBasic(CheckerManager &Mgr) { - registerCStringNullArg(Mgr); -} + void ento::registerCStringCheckerBasic(CheckerManager &Mgr) { + registerCStringNullArg(Mgr); + } Index: test/Analysis/bsd-string.c =================================================================== --- test/Analysis/bsd-string.c +++ test/Analysis/bsd-string.c @@ -0,0 +1,40 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -verify %s + +#define NULL ((void *)0) + +typedef __typeof(sizeof(int)) size_t; +size_t strlcpy(char *dst, const char *src, size_t n); +size_t strlcat(char *dst, const char *src, size_t n); +void clang_analyzer_eval(int); + +void f1() { + char overlap[] = "123456789"; + strlcpy(overlap, overlap + 1, 3); // expected-warning{{Arguments must not be overlapping buffers}} +} + +void f2() { + char buf[5]; + strlcpy(buf, "abcd", sizeof(buf)); // expected-no-warning + strlcat(buf, "efgh", sizeof(buf)); // expected-warning{{Size argument is greater than the free space in the destination buffer}} +} + +void f3() { + char dst[2]; + const char *src = "abdef"; + strlcpy(dst, src, 5); // expected-warning{{Size argument is greater than the length of the destination buffer}} +} + +void f4() { + strlcpy(NULL, "abcdef", 6); // expected-warning{{Null pointer argument in call to string copy function}} +} + +void f5() { + strlcat(NULL, "abcdef", 6); // expected-warning{{Null pointer argument in call to string copy function}} +} + +void f6() { + char buf[8]; + strlcpy(buf, "abc", 3); + size_t len = strlcat(buf, "defg", 4); + clang_analyzer_eval(len == 7); // expected-warning{{TRUE}} +}