CStringChecker, check strlcpy/strlcat

[devnexen]

Summary

Strlcpy strlcat checks with possible overlaps.

[devnexen]

ping

[devnexen]

ping

[devnexen]

If anybody can land for me, I would appreciate. Thanks regardless :-)

[NoQ]

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

106

The fact that the regular strcpy/strcat isn't checked for overlaps looks like an omission to me. The difference between strcpy/strcat and strlcpy/strlcat is not in how it handles overlaps.

[NoQ]

Whoops - this isn't quite correct because there's one more difference between strlcpy/strlcat and the standard strcpy/strcat/strncpy/strncat: the return value. After this patch the new functions are modeled as if they return a pointer into the string, which is incorrect and in fact causes crashes.

One of the crashes is on the following code:

int foo(char *d) {
  char e[1];
  return strlcpy(e, d, sizeof(e)) >= sizeof(e);
}

...when analyzed as clang -cc1 -w -analyze -analyzer-checker=core,unix repro.c.

David, would you be willing to have a look at this problem?

Also I forgot to add the tests before committing. Sorry!

[NoQ]

I reverted this for now. I'm sorry - i would have looked into this myself, but unfortunately there are other important fixes that distract me at the moment.

[devnexen]

Sure ! looking into it.

[devnexen]

The returned value is the number of character copied to the dst buffer.

[NoQ]

Yay that was fast, thanks!

Could we have tests for the return value? Like, concatenate a couple of concrete string literals and verify the return value via clang_analyzer_eval().

[devnexen]

New test to check the length

[NoQ]

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

1485

This crashes on the old tests for the checker. I guess that's because the normal strcpy() doesn't have three arguments (it counts from 0).

[devnexen]

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

1485

True I forgot those cases.

[devnexen]

ping

[rja]

LG

[devnexen]

Thanks ! I would be grateful if anybody could land it for me.

[NoQ]

Thanks! I'll commit again.

[NoQ]

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

1560–1566

One more cornercase where the return value needs to be corrected. It'd be great to de-duplicate this code to avoid similar problems in the future.

Test case:

int foo(char *dst, const char *src) {
  return strlcpy(dst, src, 0); // no-crash
}

[devnexen]

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

1560–1566

Thanks for the hint ! will do a separate "PR".

[alexfh]

This patch seems to cause an assertion failure:

assert.h assertion failed at clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:427 in clang::ento::SVal clang::ento::SValBuilder::evalBinOp(clang::ento::ProgramStateRef, BinaryOperator::Opcode, clang::ento::SVal, clang::ento::SVal, clang::QualType): op == BO_Add

The stack trace is:
__assert_fail
clang::ento::SValBuilder::evalBinOp
clang::ento::SValBuilder::evalEQ
clang::ento::SValBuilder::evalEQ
::CStringChecker::assumeZero
::CStringChecker::checkNonNull
::CStringChecker::evalStrcpyCommon
::CStringChecker::evalStrcpy
::CStringChecker::evalCall
clang::ento::eval::Call::_evalCall
clang::ento::CheckerFn::operator()
clang::ento::CheckerManager::runCheckersForEvalCall
clang::ento::ExprEngine::evalCall
clang::ento::ExprEngine::VisitCallExpr
clang::ento::ExprEngine::Visit
clang::ento::ExprEngine::ProcessStmt
clang::ento::ExprEngine::processCFGElement
clang::ento::CoreEngine::HandlePostStmt
clang::ento::CoreEngine::dispatchWorkItem
clang::ento::CoreEngine::ExecuteWorkList
clang::ento::ExprEngine::ExecuteWorkList
::AnalysisConsumer::ActionExprEngine
::AnalysisConsumer::HandleCode
::AnalysisConsumer::HandleDeclsCallGraph
::AnalysisConsumer::runAnalysisOnTranslationUnit
::AnalysisConsumer::HandleTranslationUnit

I'm trying to reduce a test case.

[alexfh]

This is reproducible in r332425.

[devnexen]

This comment was removed by devnexen.

[alexfh]

See https://bugs.llvm.org/show_bug.cgi?id=37503 for a test case.

[devnexen]

In D45177#1103162, @alexfh wrote:

See https://bugs.llvm.org/show_bug.cgi?id=37503 for a test case.

I was unable to reproduce both FreeBSD and Linux. Plus my changes come after checkNonNull.

[alexfh]

In D45177#1103774, @devnexen wrote:

In D45177#1103162, @alexfh wrote:

See https://bugs.llvm.org/show_bug.cgi?id=37503 for a test case.

I was unable to reproduce both FreeBSD and Linux. Plus my changes come after checkNonNull.

I'm not 100% sure this was caused by your patch, but the stack trace looks suspiciously similar to what was changed here. As for not being able to reproduce: do you build Clang with assertions enabled?

[devnexen]

I admit I do not due to much longer compilation time, I ll recompile all with and will see tomorrow if I can reproduce.

[devnexen]

In D45177#1103781, @alexfh wrote:

In D45177#1103774, @devnexen wrote:

In D45177#1103162, @alexfh wrote:

See https://bugs.llvm.org/show_bug.cgi?id=37503 for a test case.

I was unable to reproduce both FreeBSD and Linux. Plus my changes come after checkNonNull.

I'm not 100% sure this was caused by your patch, but the stack trace looks suspiciously similar to what was changed here. As for not being able to reproduce: do you build Clang with assertions enabled?

I was able to reproduce but also with the revision before when it has been reverted (https://github.com/llvm-mirror/clang/commit/262d2570ebab81867b46caa763a79940768d7faa#diff-b75d57d7da174f60bb0c32783975665a)

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

public:
   void evalstrnLength(CheckerContext &C, const CallExpr *CE) const;
   void evalstrLengthCommon(CheckerContext &C,
                            const CallExpr *CE,
                            bool IsStrnlen = false) const;
 
   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;
   void evalStrcasecmp(CheckerContext &C, const CallExpr *CE) const;
   void evalStrncasecmp(CheckerContext &C, const CallExpr *CE) const;
   void evalStrcmpCommon(CheckerContext &C,
                         const CallExpr *CE,
                         bool isBounded = false,
void CStringChecker::evalStpcpy(CheckerContext &C, const CallExpr *CE) const {
 
   // char *stpcpy(char *restrict dst, const char *restrict src);
   evalStrcpyCommon(C, CE,
                    /* returnEnd = */ true,
                    /* isBounded = */ false,
                    /* 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;
 
   //char *strcat(char *restrict s1, const char *restrict s2);
   evalStrcpyCommon(C, CE,
                    /* returnEnd = */ false,
                    /* isBounded = */ false,
                    /* isAppending = */ true);
 }
 
 void CStringChecker::evalStrncat(CheckerContext &C, const CallExpr *CE) const {
   if (CE->getNumArgs() < 3)
     return;
 
   //char *strncat(char *restrict s1, const char *restrict s2, size_t n);
   evalStrcpyCommon(C, CE,
                    /* returnEnd = */ false,
                    /* isBounded = */ true,
                    /* 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();
 
   // Check that the destination is non-null.
   const Expr *Dst = CE->getArg(0);
   SVal DstVal = state->getSVal(Dst, LCtx);
 
void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
 
   // These two values allow checking two kinds of errors:
   // - actual overflows caused by a source that doesn't fit in the destination
   // - potential overflows caused by a bound that could exceed the destination
   SVal amountCopied = UnknownVal();
   SVal maxLastElementIndex = UnknownVal();
   const char *boundWarning = nullptr;
 
+  state = CheckOverlap(C, state, CE->getArg(2), 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.
     const Expr *lenExpr = CE->getArg(2);
     SVal lenVal = state->getSVal(lenExpr, LCtx);
 
     // Protect against misdeclared strncpy().
     lenVal = svalBuilder.evalCast(lenVal, sizeTy, lenExpr->getType());
if (lenValNL) {
 
         // We need a special case for when the copy size is zero, in which
         // case strncpy will do no work at all. Our bounds check uses n-1
         // as the last element accessed, so n == 0 is problematic.
         ProgramStateRef StateZeroSize, StateNonZeroSize;
         std::tie(StateZeroSize, StateNonZeroSize) =
           assumeZero(C, state, *lenValNL, sizeTy);
 
         // If the size is known to be zero, we're done.
         if (StateZeroSize && !StateNonZeroSize) {
           StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, DstVal);
           C.addTransition(StateZeroSize);
           return;
         }
 
         // Otherwise, go ahead and figure out the last element we'll touch.
         // We don't record the non-zero assumption here because we can't
         // be sure. We won't warn on a possible zero.
         NonLoc one = svalBuilder.makeIntVal(1, sizeTy).castAs<NonLoc>();
         maxLastElementIndex = svalBuilder.evalBinOpNN(state, BO_Sub, *lenValNL,
                                                       one, sizeTy);
         boundWarning = "Size argument is greater than the length of the "
                        "destination buffer";
if (isAppending) {
     }
 
   } else {
     // Otherwise, this is a copy-over function (strcpy, strncpy, ...), and
     // the final string length will match the input string length.
     finalStrLength = amountCopied;
   }
 
+  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.
-  SVal Result = (returnEnd ? UnknownVal() : DstVal);
+    Result = (returnEnd ? UnknownVal() : DstVal);
+  } else {
+    Result = amountCopied;
+  }
 
   assert(state);
 
   // If the destination is a MemRegion, try to check for a buffer overflow and
   // record the new string length.
   if (Optional<loc::MemRegionVal> dstRegVal =
-          DstVal.getAs<loc::MemRegionVal>()) {
     QualType ptrTy = Dst->getType();
 
     // If we have an exact value on a bounded copy, use that to check for
     // overflows, rather than our estimate about how much is actually copied.
     if (boundWarning) {
       if (Optional<NonLoc> maxLastNL = maxLastElementIndex.getAs<NonLoc>()) {
         SVal maxLastElement = svalBuilder.evalBinOpLN(state, BO_Add, *dstRegVal,
-                                                      *maxLastNL, ptrTy);
         state = CheckLocation(C, state, CE->getArg(2), maxLastElement,
-                              boundWarning);
         if (!state)
           return;
       }
     }
 
     // Then, if the final length is known...
     if (Optional<NonLoc> knownStrLength = finalStrLength.getAs<NonLoc>()) {
       SVal lastElement = svalBuilder.evalBinOpLN(state, BO_Add, *dstRegVal,
-                                                 *knownStrLength, ptrTy);
 
       // ...and we haven't checked the bound, we'll check the actual copy.
       if (!boundWarning) {
         const char * const warningMsg =
           "String copy function overflows destination buffer";
         state = CheckLocation(C, state, Dst, lastElement, warningMsg);
         if (!state)
           return;
       }
 
       // If this is a stpcpy-style copy, the last element is the return value.
-      if (returnEnd)
+      if (returnPtr && returnEnd)
         Result = lastElement;
     }
 
     // Invalidate the destination (regular invalidation without pointer-escaping
     // the address of the top-level region). This must happen before we set the
     // C string length because invalidation will clear the length.
     // FIXME: Even if we can't perfectly model the copy, we should see if we
     // can use LazyCompoundVals to copy the source values into the destination.
     // 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);
 
     // 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);
 
     // Set the C string length of the destination, if we know it.
     if (isBounded && !isAppending) {
       // strncpy is annoying in that it doesn't guarantee to null-terminate
       // the result string. If the original string didn't fit entirely inside
       // the bound (including the null-terminator), we don't know how long the
       // result is.
       if (amountCopied != strLength)
         finalStrLength = UnknownVal();
     }
     state = setCStringLength(state, dstRegVal->getRegion(), finalStrLength);
   }
 
   assert(state);
 
+  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);
 }
 
 void CStringChecker::evalStrcmp(CheckerContext &C, const CallExpr *CE) const {
   if (CE->getNumArgs() < 2)
     return;
 
   //int strcmp(const char *s1, const char *s2);
   evalStrcmpCommon(C, CE, /* isBounded = */ false, /* ignoreCase = */ false);
 }
 
 void CStringChecker::evalStrncmp(CheckerContext &C, const CallExpr *CE) const {
   if (CE->getNumArgs() < 3)
     return;
 
   //int strncmp(const char *s1, const char *s2, size_t n);
   evalStrcmpCommon(C, CE, /* isBounded = */ true, /* ignoreCase = */ false);
 }
 
 void CStringChecker::evalStrcasecmp(CheckerContext &C,
-                                    const CallExpr *CE) const {
   if (CE->getNumArgs() < 2)
     return;
 
   //int strcasecmp(const char *s1, const char *s2);
   evalStrcmpCommon(C, CE, /* isBounded = */ false, /* ignoreCase = */ true);
 }
 
 void CStringChecker::evalStrncasecmp(CheckerContext &C,
-                                     const CallExpr *CE) const {
   if (CE->getNumArgs() < 3)
     return;
 
   //int strncasecmp(const char *s1, const char *s2, size_t n);
   evalStrcmpCommon(C, CE, /* isBounded = */ true, /* ignoreCase = */ true);
 }
 
 void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE,
-                                      bool isBounded, bool ignoreCase) const {
   CurrentFunctionDescription = "string comparison function";
   ProgramStateRef state = C.getState();
   const LocationContext *LCtx = C.getLocationContext();
 
   // Check that the first string is non-null
   const Expr *s1 = CE->getArg(0);
   SVal s1Val = state->getSVal(s1, LCtx);
   state = checkNonNull(C, state, s1, s1Val);
void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE,
   DefinedOrUnknownSVal SameBuf = svalBuilder.evalEQ(state, LV, RV);
   ProgramStateRef StSameBuf, StNotSameBuf;
   std::tie(StSameBuf, StNotSameBuf) = state->assume(SameBuf);
 
   // If the two arguments might be the same buffer, we know the result is 0,
   // and we only need to check one size.
   if (StSameBuf) {
     StSameBuf = StSameBuf->BindExpr(CE, LCtx,
-                                    svalBuilder.makeZeroVal(CE->getType()));
     C.addTransition(StSameBuf);
 
     // If the two arguments are GUARANTEED to be the same, we're done!
     if (!StNotSameBuf)
       return;
   }
 
   assert(StNotSameBuf);
   state = StNotSameBuf;
 
   // At this point we can go about comparing the two buffers.
   // For now, we only do this if they're both known string literals.
 
   // Attempt to extract string literals from both expressions.
   const StringLiteral *s1StrLiteral = getCStringLiteral(C, state, s1, s1Val);
   const StringLiteral *s2StrLiteral = getCStringLiteral(C, state, s2, s2Val);
   bool canComputeResult = false;
   SVal resultVal = svalBuilder.conjureSymbolVal(nullptr, CE, LCtx,
-                                                C.blockCount());
 
   if (s1StrLiteral && s2StrLiteral) {
     StringRef s1StrRef = s1StrLiteral->getString();
     StringRef s2StrRef = s2StrLiteral->getString();
 
     if (isBounded) {
       // Get the max number of characters to compare.
       const Expr *lenExpr = CE->getArg(2);
if (canComputeResult) {
         s1StrRef = s1StrRef.substr(0, s1Term);
 
       size_t s2Term = s2StrRef.find('\0');
       if (s2Term != StringRef::npos)
         s2StrRef = s2StrRef.substr(0, s2Term);
 
       // Use StringRef's comparison methods to compute the actual result.
       int compareRes = ignoreCase ? s1StrRef.compare_lower(s2StrRef)
-                                  : s1StrRef.compare(s2StrRef);
 
       // The strcmp function returns an integer greater than, equal to, or less
       // than zero, [c11, p7.24.4.2].
       if (compareRes == 0) {
         resultVal = svalBuilder.makeIntVal(compareRes, CE->getType());
       }
       else {
         DefinedSVal zeroVal = svalBuilder.makeIntVal(0, CE->getType());
         // Constrain strcmp's result range based on the result of StringRef's
         // comparison methods.
         BinaryOperatorKind op = (compareRes == 1) ? BO_GT : BO_LT;
         SVal compareWithZero =
           svalBuilder.evalBinOp(state, op, resultVal, zeroVal,
-                                svalBuilder.getConditionType());
         DefinedSVal compareWithZeroVal = compareWithZero.castAs<DefinedSVal>();
         state = state->assume(compareWithZeroVal, true);
       }
     }
   }
 
   state = state->BindExpr(CE, LCtx, resultVal);
 
void CStringChecker::evalStrsep(CheckerContext &C, const CallExpr *CE) const {
   SVal Result;
   if (Optional<Loc> SearchStrLoc = SearchStrVal.getAs<Loc>()) {
     // Get the current value of the search string pointer, as a char*.
     Result = State->getSVal(*SearchStrLoc, CharPtrTy);
 
     // Invalidate the search string, representing the change of one delimiter
     // character to NUL.
     State = InvalidateBuffer(C, State, SearchStrPtr, Result,
-                             /*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);
   } else {
     assert(SearchStrVal.isUnknown());
     // Conjure a symbolic value. It's the best we can do.
     Result = SVB.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
   }
 
   // Set the return value, and finish.
   State = State->BindExpr(CE, LCtx, Result);
   C.addTransition(State);
 }
 
 // These should probably be moved into a C++ standard library checker.
 void CStringChecker::evalStdCopy(CheckerContext &C, const CallExpr *CE) const {
   evalStdCopyCommon(C, CE);
 }
 
 void CStringChecker::evalStdCopyBackward(CheckerContext &C,
-                                         const CallExpr *CE) const {
   evalStdCopyCommon(C, CE);
 }
 
 void CStringChecker::evalStdCopyCommon(CheckerContext &C,
-                                       const CallExpr *CE) const {
   if (CE->getNumArgs() < 3)
     return;
 
   ProgramStateRef State = C.getState();
 
   const LocationContext *LCtx = C.getLocationContext();
 
   // template <class _InputIterator, class _OutputIterator>
   // _OutputIterator
   // copy(_InputIterator __first, _InputIterator __last,
   //        _OutputIterator __result)
 
   // Invalidate the destination buffer
   const Expr *Dst = CE->getArg(2);
   SVal DstVal = State->getSVal(Dst, LCtx);
   State = InvalidateBuffer(C, State, Dst, DstVal, /*IsSource=*/false,
-                           /*Size=*/nullptr);
 
   SValBuilder &SVB = C.getSValBuilder();
 
   SVal ResultVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount());
   State = State->BindExpr(CE, LCtx, ResultVal);
 
   C.addTransition(State);
 }
void CStringChecker::evalMemset(CheckerContext &C, const CallExpr *CE) const {
   State = checkNonNull(C, StateNonZeroSize, Mem, MemVal);
   if (!State)
     return;
 
   State = CheckBufferAccess(C, State, Size, Mem);
   if (!State)
     return;
   State = InvalidateBuffer(C, State, Mem, C.getSVal(Mem),
-                           /*IsSourceBuffer*/false, Size);
   if (!State)
     return;
 
   State = State->BindExpr(CE, LCtx, MemVal);
   C.addTransition(State);
 }
 
 static bool isCPPStdLibraryFunction(const FunctionDecl *FD, StringRef Name) {
bool CStringChecker::evalCall(const CallExpr *CE, CheckerContext &C) const {
   else if (C.isCLibraryFunction(FDecl, "memset"))
     evalFunction =  &CStringChecker::evalMemset;
   else if (C.isCLibraryFunction(FDecl, "strcpy"))
     evalFunction =  &CStringChecker::evalStrcpy;
   else if (C.isCLibraryFunction(FDecl, "strncpy"))
     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"))
     evalFunction =  &CStringChecker::evalstrnLength;
   else if (C.isCLibraryFunction(FDecl, "strcmp"))
     evalFunction =  &CStringChecker::evalStrcmp;
   else if (C.isCLibraryFunction(FDecl, "strncmp"))
     evalFunction =  &CStringChecker::evalStrncmp;
for (const auto *I : DS->decls()) {
     Loc VarLoc = state->getLValue(D, C.getLocationContext());
     const MemRegion *MR = VarLoc.getAsRegion();
     if (!MR)
       continue;
 
     SVal StrVal = C.getSVal(Init);
     assert(StrVal.isValid() && "Initializer string is unknown or undefined");
     DefinedOrUnknownSVal strLength =
-        getCStringLength(C, state, Init, StrVal).castAs<DefinedOrUnknownSVal>();
 
     state = state->set<CStringLength>(MR, strLength);
   }
 
   C.addTransition(state);
 }
 
 ProgramStateRef
 CStringChecker::checkRegionChanges(ProgramStateRef state,
-                                   const InvalidatedSymbols *,
-                                   ArrayRef<const MemRegion *> ExplicitRegions,
-                                   ArrayRef<const MemRegion *> Regions,
-                                   const LocationContext *LCtx,
-                                   const CallEvent *Call) const {
   CStringLengthTy Entries = state->get<CStringLength>();
   if (Entries.isEmpty())
     return state;
 
   llvm::SmallPtrSet<const MemRegion *, 8> Invalidated;
   llvm::SmallPtrSet<const MemRegion *, 32> SuperRegions;
 
   // First build sets for the changed regions and their super-regions.
   for (ArrayRef<const MemRegion *>::iterator
-       I = Regions.begin(), E = Regions.end(); I != E; ++I) {
     const MemRegion *MR = *I;
     Invalidated.insert(MR);
 
     SuperRegions.insert(MR);
     while (const SubRegion *SR = dyn_cast<SubRegion>(MR)) {
       MR = SR->getSuperRegion();
       SuperRegions.insert(MR);
     }
   }
 
   CStringLengthTy::Factory &F = state->get_context<CStringLength>();
 
   // Then loop over the entries in the current state.
   for (CStringLengthTy::iterator I = Entries.begin(),
-       E = Entries.end(); I != E; ++I) {
     const MemRegion *MR = I.getKey();
 
     // Is this entry for a super-region of a changed region?
     if (SuperRegions.count(MR)) {
       Entries = F.remove(Entries, MR);
       continue;
     }
 
     // Is this entry for a sub-region of a changed region?
     const MemRegion *Super = MR;
     while (const SubRegion *SR = dyn_cast<SubRegion>(Super)) {
       Super = SR->getSuperRegion();
       if (Invalidated.count(Super)) {
         Entries = F.remove(Entries, MR);
         break;
       }
     }
   }
 
   return state->set<CStringLength>(Entries);
 }
 
 void CStringChecker::checkLiveSymbols(ProgramStateRef state,
-                                      SymbolReaper &SR) const {
   // Mark all symbols in our string length map as valid.
   CStringLengthTy Entries = state->get<CStringLength>();
 
   for (CStringLengthTy::iterator I = Entries.begin(), E = Entries.end();
-       I != E; ++I) {
     SVal Len = I.getData();
 
     for (SymExpr::symbol_iterator si = Len.symbol_begin(),
-                                  se = Len.symbol_end(); si != se; ++si)
       SR.markInUse(*si);
   }
 }
 
 void CStringChecker::checkDeadSymbols(SymbolReaper &SR,
-                                      CheckerContext &C) const {
   if (!SR.hasDeadSymbols())
     return;
 
   ProgramStateRef state = C.getState();
   CStringLengthTy Entries = state->get<CStringLength>();
   if (Entries.isEmpty())
     return;
 
   CStringLengthTy::Factory &F = state->get_context<CStringLength>();
   for (CStringLengthTy::iterator I = Entries.begin(), E = Entries.end();
-       I != E; ++I) {
     SVal Len = I.getData();
     if (SymbolRef Sym = Len.getAsSymbol()) {
       if (SR.isDead(Sym))
         Entries = F.remove(Entries, I.getKey());
     }
   }
 
   state = state->set<CStringLength>(Entries);
   C.addTransition(state);
 }
 
 #define REGISTER_CHECKER(name)                                                 \
   void ento::register##name(CheckerManager &mgr) {                             \
     CStringChecker *checker = mgr.registerChecker<CStringChecker>();           \
     checker->Filter.Check##name = true;                                        \
     checker->Filter.CheckName##name = mgr.getCurrentCheckName();               \
   }
 
   REGISTER_CHECKER(CStringNullArg)
   REGISTER_CHECKER(CStringOutOfBounds)
   REGISTER_CHECKER(CStringBufferOverlap)
 REGISTER_CHECKER(CStringNotNullTerm)
 
   void ento::registerCStringCheckerBasic(CheckerManager &Mgr) {
     registerCStringNullArg(Mgr);
   }

test/Analysis/bsd-string.c

+// 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 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}}
+}