[analyzer] PR41729: Fix some false positives and improve strlcat and strlcpy modeling

[dkrupp]

Summary

Fixes Bug 41729 (https://bugs.llvm.org/show_bug.cgi?id=41729)
and the following errors:

• Fixes false positive reports of strlcat
• The return value of strlcat and strlcpy is now correctly calculated
• The resulting string length of strlcat and strlcpy is now correctly calculated

[NoQ]

Oh, these false positives! Thanks!!

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

1627 ↗

(On Diff #214512)

Please handle the situation when freeSpaceNL is None before dereferencing. I suspect that this might actually happen when you overwhelm the symbol complexity by subtracting 1. I'm not really super sure this can happen, given that symbols we're working with are very specific as of now, but this is something that should be made possible if we ever go far enough in our attempts to improve this checker.

1637 ↗

(On Diff #214512)

Strange whitespace here and in the next comment (:

1647 ↗

(On Diff #214512)

Great job not introducing the state split here!

test/Analysis/cstring-syntax.c

57 ↗

(On Diff #214512)

I think this test doesn't really demonstrate that bug 41729 is fixed, as the offending checker isn't enabled on this test.

[dkrupp]

Fix comments from @NoQ

[dkrupp]

Thanks for the comments @NoQ , all of them addressed.

[gamesh411]

Nice improvements! It was a good catch that a substantial part of the checker could be cut out. Accurate modelling of CString and related API-s is really important, and this is a good step in that direction.
I would test this on a larger codebase as well before committing, but LGTM otherwise.

[Szelethus]

I agree with @gamesh411 here, would it be much trouble to see how this behaves on a bigger codebase?

[Szelethus]

The logic of the patch is fine in my eyes, but I dislike the formatting. Could you please make the code obey the LLVM coding style, and after that use clang-format-diff.py?
https://llvm.org/docs/CodingStandards.html
https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

31 ↗

(On Diff #214817)

Please capitalize these. Also, ConcatFnKind might be more descriptive? Ad absurdum, we might as well pass the CallDescription that is responsible for the concatenation. That maaay just be a little over engineered though.

133–134 ↗

(On Diff #214817)

CaPitALizeE :D

1556 ↗

(On Diff #214817)

Please terminate this sentence.

1580 ↗

(On Diff #214817)

Ah, okay, so the assumption is that bounded functions' third argument is always a numerical size parameter. Why isn't that enforced at all?

1596 ↗

(On Diff #214817)

Can we just do a switch-case? If someone were to add a new concat type, it would even give a warning in case it was left unhandled.

1628 ↗

(On Diff #214817)

Please put comments like these before the branch. I like the following format better:

// While unlikely, it is possible that the subtraction is too complexity
// to complex to compute, let's check whether it succeeded.
if (Optional<NonLoc> freeSpaceNL = freeSpace.getAs<NonLoc>())

1630 ↗

(On Diff #214817)

hasEnoughSpace

1637–1640 ↗

(On Diff #214817)

All of these too, and omit braces too:

// srcStrLength <= size - dstStrLength -1
if (TrueState && !FalseState)
  amountCopied = strLength;

1657 ↗

(On Diff #214817)

Would prefer a switch case here too.

[dkrupp]

@Szelethus thanks for your review.
I fixed your suggestions.

[dkrupp]

I also analyzed openssl with the baseline and this version, but did not find any new warnings.
See:
http://codechecker-demo.eastus.cloudapp.azure.com/Default/#run=D66049_baseline&newcheck=D66049_improved&review-status=Unreviewed&review-status=Confirmed&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=D66049_baseline_diff_D66049_improved

lib/StaticAnalyzer/Checkers/CStringChecker.cpp

1580 ↗

(On Diff #214817)

How should we enforce this?

[dkrupp]

Fixing minor capitalization issue and removing an extra newline.

[Szelethus]

LGTM!

In D66049#1701730, @dkrupp wrote:

I also analyzed openssl with the baseline and this version, but did not find any new warnings.
See:
http://codechecker-demo.eastus.cloudapp.azure.com/Default/#run=D66049_baseline&newcheck=D66049_improved&review-status=Unreviewed&review-status=Confirmed&detection-status=New&detection-status=Reopened&detection-status=Unresolved&tab=D66049_baseline_diff_D66049_improved

This requires a username/pw, but if there isn't any noticeable change on open source code, this change mustn't hurt much.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

1652–1654

We usually omit braces when the branch consists of a single line.

[dkrupp]

Thanks for the reviews!
Could you pls commit this for me?

[dkrupp]

If this is good to go, could you please commit this? Thanks!

[NoQ]

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

1710–1711

rGf450dd63a14d

Please use the normal evalBinOp without suffixes, it just works!

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
+enum class ConcatFnKind { none = 0, strcat = 1, strlcat = 2 };
 class CStringChecker : public Checker< eval::Call,
                                          check::PreStmt<DeclStmt>,
                                          check::LiveSymbols,
                                          check::DeadSymbols,
                                          check::RegionChanges
                                          > {
   mutable std::unique_ptr<BugType> BT_Null, BT_Bounds, BT_Overlap,
       BT_NotCString, BT_AdditionOverflow;
public:
   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,
+  void evalStrcpyCommon(CheckerContext &C, const CallExpr *CE, bool ReturnEnd,
+                        bool IsBounded, ConcatFnKind appendK,
                         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,
-                        bool ignoreCase = false) const;
+                        bool IsBounded = false,
+                        bool IgnoreCase = false) const;
 
   void evalStrsep(CheckerContext &C, const CallExpr *CE) const;
 
   void evalStdCopy(CheckerContext &C, const CallExpr *CE) const;
   void evalStdCopyBackward(CheckerContext &C, const CallExpr *CE) const;
   void evalStdCopyCommon(CheckerContext &C, const CallExpr *CE) const;
   void evalMemset(CheckerContext &C, const CallExpr *CE) const;
   void evalBzero(CheckerContext &C, const CallExpr *CE) const;
void CStringChecker::evalstrLengthCommon(CheckerContext &C, const CallExpr *CE,
   assert(!result.isUnknown() && "Should have conjured a value by now");
   state = state->BindExpr(CE, LCtx, result);
   C.addTransition(state);
 }
 
 void CStringChecker::evalStrcpy(CheckerContext &C, const CallExpr *CE) const {
   // char *strcpy(char *restrict dst, const char *restrict src);
   evalStrcpyCommon(C, CE,
-                   /* returnEnd = */ false,
-                   /* isBounded = */ false,
-                   /* isAppending = */ false);
+                   /* ReturnEnd = */ false,
+                   /* IsBounded = */ false,
+                   /* appendK = */ ConcatFnKind::none);
 }
 
 void CStringChecker::evalStrncpy(CheckerContext &C, const CallExpr *CE) const {
   // char *strncpy(char *restrict dst, const char *restrict src, size_t n);
   evalStrcpyCommon(C, CE,
-                   /* returnEnd = */ false,
-                   /* isBounded = */ true,
-                   /* isAppending = */ false);
+                   /* ReturnEnd = */ false,
+                   /* IsBounded = */ true,
+                   /* appendK = */ ConcatFnKind::none);
 }
 
 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);
+                   /* ReturnEnd = */ true,
+                   /* IsBounded = */ false,
+                   /* appendK = */ ConcatFnKind::none);
 }
 
 void CStringChecker::evalStrlcpy(CheckerContext &C, const CallExpr *CE) const {
-  // char *strlcpy(char *dst, const char *src, size_t n);
+  // size_t strlcpy(char *dest, const char *src, size_t size);
   evalStrcpyCommon(C, CE,
-                   /* returnEnd = */ true,
-                   /* isBounded = */ true,
-                   /* isAppending = */ false,
+                   /* ReturnEnd = */ true,
+                   /* IsBounded = */ true,
+                   /* appendK = */ ConcatFnKind::none,
                    /* returnPtr = */ false);
 }
 
 void CStringChecker::evalStrcat(CheckerContext &C, const CallExpr *CE) const {
-  //char *strcat(char *restrict s1, const char *restrict s2);
+  // char *strcat(char *restrict s1, const char *restrict s2);
   evalStrcpyCommon(C, CE,
-                   /* returnEnd = */ false,
-                   /* isBounded = */ false,
-                   /* isAppending = */ true);
+                   /* ReturnEnd = */ false,
+                   /* IsBounded = */ false,
+                   /* appendK = */ ConcatFnKind::strcat);
 }
 
 void CStringChecker::evalStrncat(CheckerContext &C, const CallExpr *CE) const {
   //char *strncat(char *restrict s1, const char *restrict s2, size_t n);
   evalStrcpyCommon(C, CE,
-                   /* returnEnd = */ false,
-                   /* isBounded = */ true,
-                   /* isAppending = */ true);
+                   /* ReturnEnd = */ false,
+                   /* IsBounded = */ true,
+                   /* appendK = */ ConcatFnKind::strcat);
 }
 
 void CStringChecker::evalStrlcat(CheckerContext &C, const CallExpr *CE) const {
-  // FIXME: strlcat() uses a different rule for bound checking, i.e. 'n' means
-  // a different thing as compared to strncat(). This currently causes
-  // false positives in the alpha string bound checker.
-
-  //char *strlcat(char *s1, const char *s2, size_t n);
+  // size_t strlcat(char *dst, const char *src, size_t size);
+  // It will append at most size - strlen(dst) - 1 bytes,
+  // NULL-terminating the result.
   evalStrcpyCommon(C, CE,
-                   /* returnEnd = */ false,
-                   /* isBounded = */ true,
-                   /* isAppending = */ true,
+                   /* ReturnEnd = */ false,
+                   /* IsBounded = */ true,
+                   /* appendK = */ ConcatFnKind::strlcat,
                    /* returnPtr = */ false);
 }
 
 void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
-                                      bool returnEnd, bool isBounded,
-                                      bool isAppending, bool returnPtr) const {
+                                      bool ReturnEnd, bool IsBounded,
+                                      ConcatFnKind appendK,
+                                      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);
 
   state = checkNonNull(C, state, Dst, DstVal, 1);
   if (!state)
     return;
 
   // Check that the source is non-null.
   const Expr *srcExpr = CE->getArg(1);
   SVal srcVal = state->getSVal(srcExpr, LCtx);
   state = checkNonNull(C, state, srcExpr, srcVal, 2);
   if (!state)
     return;
 
   // Get the string length of the source.
   SVal strLength = getCStringLength(C, state, srcExpr, srcVal);
+  Optional<NonLoc> strLengthNL = strLength.getAs<NonLoc>();
+
+  // Get the string length of the destination buffer.
+  SVal dstStrLength = getCStringLength(C, state, Dst, DstVal);
+  Optional<NonLoc> dstStrLengthNL = dstStrLength.getAs<NonLoc>();
 
   // If the source isn't a valid C string, give up.
   if (strLength.isUndef())
     return;
 
   SValBuilder &svalBuilder = C.getSValBuilder();
   QualType cmpTy = svalBuilder.getConditionType();
   QualType sizeTy = svalBuilder.getContext().getSizeType();
 
   // 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, isBounded ? CE->getArg(2) : CE->getArg(1), Dst, srcExpr);
+  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) {
+  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());
 
-    Optional<NonLoc> strLengthNL = strLength.getAs<NonLoc>();
     Optional<NonLoc> lenValNL = lenVal.getAs<NonLoc>();
 
     // If we know both values, we might be able to figure out how much
     // we're copying.
     if (strLengthNL && lenValNL) {
+      switch (appendK) {
+      case ConcatFnKind::none:
+      case ConcatFnKind::strcat: {
         ProgramStateRef stateSourceTooLong, stateSourceNotTooLong;
-
         // Check if the max number to copy is less than the length of the src.
         // If the bound is equal to the source length, strncpy won't null-
         // terminate the result!
         std::tie(stateSourceTooLong, stateSourceNotTooLong) = state->assume(
-          svalBuilder.evalBinOpNN(state, BO_GE, *strLengthNL, *lenValNL, cmpTy)
+            svalBuilder
+                .evalBinOpNN(state, BO_GE, *strLengthNL, *lenValNL, cmpTy)
                 .castAs<DefinedOrUnknownSVal>());
 
         if (stateSourceTooLong && !stateSourceNotTooLong) {
-        // Max number to copy is less than the length of the src, so the actual
-        // strLength copied is the max number arg.
+          // Max number to copy is less than the length of the src, so the
+          // actual strLength copied is the max number arg.
           state = stateSourceTooLong;
           amountCopied = lenVal;
 
         } else if (!stateSourceTooLong && stateSourceNotTooLong) {
           // The source buffer entirely fits in the bound.
           state = stateSourceNotTooLong;
           amountCopied = strLength;
         }
+        break;
+      }
+      case ConcatFnKind::strlcat:
+        if (!dstStrLengthNL)
+          return;
+
+        // amountCopied = min (size - dstLen - 1 , srcLen)
+        SVal freeSpace = svalBuilder.evalBinOpNN(state, BO_Sub, *lenValNL,
+                                                 *dstStrLengthNL, sizeTy);
+        if (!freeSpace.getAs<NonLoc>())
+          return;
+        freeSpace =
+            svalBuilder.evalBinOp(state, BO_Sub, freeSpace,
+                                  svalBuilder.makeIntVal(1, sizeTy), sizeTy);
+        Optional<NonLoc> freeSpaceNL = freeSpace.getAs<NonLoc>();
+
+        // While unlikely, it is possible that the subtraction is
+        // too complex to compute, let's check whether it succeeded.
+        if (!freeSpaceNL)
+          return;
+        SVal hasEnoughSpace = svalBuilder.evalBinOpNN(
+            state, BO_LE, *strLengthNL, *freeSpaceNL, cmpTy);
+
+        ProgramStateRef TrueState, FalseState;
+        std::tie(TrueState, FalseState) =
+            state->assume(hasEnoughSpace.castAs<DefinedOrUnknownSVal>());
+
+        // srcStrLength <= size - dstStrLength -1
+        if (TrueState && !FalseState) {
+          amountCopied = strLength;
+        }
+
+        // srcStrLength > size - dstStrLength -1
+        if (!TrueState && FalseState) {
+          amountCopied = freeSpace;
         }
 
+        if (TrueState && FalseState)
+          amountCopied = UnknownVal();
+        break;
+      }
+    }
     // We still want to know if the bound is known to be too large.
     if (lenValNL) {
-      if (isAppending) {
+      switch (appendK) {
+      case ConcatFnKind::strcat:
         // For strncat, the check is strlen(dst) + lenVal < sizeof(dst)
 
         // Get the string length of the destination. If the destination is
         // memory that can't have a string length, we shouldn't be copying
         // into it anyway.
-        SVal dstStrLength = getCStringLength(C, state, Dst, DstVal);
         if (dstStrLength.isUndef())
           return;
 
-        if (Optional<NonLoc> dstStrLengthNL = dstStrLength.getAs<NonLoc>()) {
-          maxLastElementIndex = svalBuilder.evalBinOpNN(state, BO_Add,
-                                                        *lenValNL,
-                                                        *dstStrLengthNL,
-                                                        sizeTy);
+        if (dstStrLengthNL) {
+          maxLastElementIndex = svalBuilder.evalBinOpNN(
+              state, BO_Add, *lenValNL, *dstStrLengthNL, sizeTy);
+
           boundWarning = "Size argument is greater than the free space in the "
                          "destination buffer";
         }
-
-      } else {
-        // For strncpy, this is just checking that lenVal <= sizeof(dst)
+        break;
+      case ConcatFnKind::none:
+      case ConcatFnKind::strlcat:
+        // For strncpy and strlcat, this is just checking
+        //  that lenVal <= sizeof(dst).
         // (Yes, strncpy and strncat differ in how they treat termination.
         // strncat ALWAYS terminates, but strncpy doesn't.)
 
         // 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) {
           if (returnPtr) {
             StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, DstVal);
           } else {
-            StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, *lenValNL);
+            if (appendK == ConcatFnKind::none) {
+              // strlcpy returns strlen(src)
+              StateZeroSize = StateZeroSize->BindExpr(CE, LCtx, *strLengthNL);
+            } else if (dstStrLengthNL) {
+              // strlcat returns strlen(src) + strlen(dst)
+              SVal retSize = svalBuilder.evalBinOpNN(
+                  state, BO_Add, *strLengthNL, *dstStrLengthNL, sizeTy);
+              StateZeroSize =
+                  StateZeroSize->BindExpr(CE, LCtx, *(retSize.getAs<NonLoc>()));
+            }
           }
           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);
+        maxLastElementIndex =
+            svalBuilder.evalBinOpNN(state, BO_Sub, *lenValNL, one, sizeTy);
         boundWarning = "Size argument is greater than the length of the "
                        "destination buffer";
+        break;
       }
     }
-
-    // If we couldn't pin down the copy length, at least bound it.
-    // FIXME: We should actually run this code path for append as well, but
-    // right now it creates problems with constraints (since we can end up
-    // trying to pass constraints from symbol to symbol).
-    if (amountCopied.isUnknown() && !isAppending) {
-      // Try to get a "hypothetical" string length symbol, which we can later
-      // set as a real value if that turns out to be the case.
-      amountCopied = getCStringLength(C, state, lenExpr, srcVal, true);
-      assert(!amountCopied.isUndef());
-
-      if (Optional<NonLoc> amountCopiedNL = amountCopied.getAs<NonLoc>()) {
-        if (lenValNL) {
-          // amountCopied <= lenVal
-          SVal copiedLessThanBound = svalBuilder.evalBinOpNN(state, BO_LE,
-                                                             *amountCopiedNL,
-                                                             *lenValNL,
-                                                             cmpTy);
-          state = state->assume(
-              copiedLessThanBound.castAs<DefinedOrUnknownSVal>(), true);
-          if (!state)
-            return;
-        }
-
-        if (strLengthNL) {
-          // amountCopied <= strlen(source)
-          SVal copiedLessThanSrc = svalBuilder.evalBinOpNN(state, BO_LE,
-                                                           *amountCopiedNL,
-                                                           *strLengthNL,
-                                                           cmpTy);
-          state = state->assume(
-              copiedLessThanSrc.castAs<DefinedOrUnknownSVal>(), true);
-          if (!state)
-            return;
-        }
-      }
-    }
-
   } else {
     // The function isn't bounded. The amount copied should match the length
     // of the source buffer.
     amountCopied = strLength;
   }
 
   assert(state);
 
   // This represents the number of characters copied into the destination
   // buffer. (It may not actually be the strlen if the destination buffer
   // is not terminated.)
   SVal finalStrLength = UnknownVal();
+  SVal strlRetVal = UnknownVal();
+
+  if (appendK == ConcatFnKind::none && !returnPtr) {
+    // strlcpy returns the sizeof(src)
+    strlRetVal = strLength;
+  }
 
   // If this is an appending function (strcat, strncat...) then set the
   // string length to strlen(src) + strlen(dst) since the buffer will
   // ultimately contain both.
-  if (isAppending) {
+  if (appendK != ConcatFnKind::none) {
     // Get the string length of the destination. If the destination is memory
     // that can't have a string length, we shouldn't be copying into it anyway.
-    SVal dstStrLength = getCStringLength(C, state, Dst, DstVal);
     if (dstStrLength.isUndef())
       return;
 
-    Optional<NonLoc> srcStrLengthNL = amountCopied.getAs<NonLoc>();
-    Optional<NonLoc> dstStrLengthNL = dstStrLength.getAs<NonLoc>();
+    if (appendK == ConcatFnKind::strlcat && dstStrLengthNL && strLengthNL) {
+      strlRetVal = svalBuilder.evalBinOpNN(state, BO_Add, *strLengthNL,
+                                           *dstStrLengthNL, sizeTy);
+    }
+
+    Optional<NonLoc> amountCopiedNL = amountCopied.getAs<NonLoc>();
 
     // If we know both string lengths, we might know the final string length.
-    if (srcStrLengthNL && dstStrLengthNL) {
+    if (amountCopiedNL && dstStrLengthNL) {
       // Make sure the two lengths together don't overflow a size_t.
-      state = checkAdditionOverflow(C, state, *srcStrLengthNL, *dstStrLengthNL);
+      state = checkAdditionOverflow(C, state, *amountCopiedNL, *dstStrLengthNL);
       if (!state)
         return;
 
-      finalStrLength = svalBuilder.evalBinOpNN(state, BO_Add, *srcStrLengthNL,
+      finalStrLength = svalBuilder.evalBinOpNN(state, BO_Add, *amountCopiedNL,
                                                *dstStrLengthNL, sizeTy);
     }
 
     // If we couldn't get a single value for the final string length,
     // we can at least bound it by the individual lengths.
     if (finalStrLength.isUnknown()) {
       // Try to get a "hypothetical" string length symbol, which we can later
       // set as a real value if that turns out to be the case.
       finalStrLength = getCStringLength(C, state, CE, DstVal, true);
       assert(!finalStrLength.isUndef());
 
       if (Optional<NonLoc> finalStrLengthNL = finalStrLength.getAs<NonLoc>()) {
-        if (srcStrLengthNL) {
+        if (amountCopiedNL && appendK == ConcatFnKind::none) {
+          // we overwrite dst string with the src
           // finalStrLength >= srcStrLength
-          SVal sourceInResult = svalBuilder.evalBinOpNN(state, BO_GE,
-                                                        *finalStrLengthNL,
-                                                        *srcStrLengthNL,
-                                                        cmpTy);
+          SVal sourceInResult = svalBuilder.evalBinOpNN(
+              state, BO_GE, *finalStrLengthNL, *amountCopiedNL, cmpTy);
           state = state->assume(sourceInResult.castAs<DefinedOrUnknownSVal>(),
                                 true);
           if (!state)
             return;
         }
 
-        if (dstStrLengthNL) {
+        if (dstStrLengthNL && appendK != ConcatFnKind::none) {
+          // we extend the dst string with the src
           // finalStrLength >= dstStrLength
           SVal destInResult = svalBuilder.evalBinOpNN(state, BO_GE,
                                                       *finalStrLengthNL,
                                                       *dstStrLengthNL,
                                                       cmpTy);
           state =
               state->assume(destInResult.castAs<DefinedOrUnknownSVal>(), true);
           if (!state)
             return;
         }
       }
     }
 
   } 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.
-    Result = (returnEnd ? UnknownVal() : DstVal);
+    Result = (ReturnEnd ? UnknownVal() : DstVal);
   } else {
+    if (appendK == ConcatFnKind::strlcat || appendK == ConcatFnKind::none)
+      //strlcpy, strlcat
+      Result = strlRetVal;
+    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<loc::MemRegionVal> dstRegVal =
       DstVal.getAs<loc::MemRegionVal>()) {
if (Optional<NonLoc> knownStrLength = finalStrLength.getAs<NonLoc>()) {
         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 (returnPtr && 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) {
+    if (IsBounded && (appendK == ConcatFnKind::none)) {
       // 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()) {
+    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 {
   //int strcmp(const char *s1, const char *s2);
-  evalStrcmpCommon(C, CE, /* isBounded = */ false, /* ignoreCase = */ false);
+  evalStrcmpCommon(C, CE, /* IsBounded = */ false, /* IgnoreCase = */ false);
 }
 
 void CStringChecker::evalStrncmp(CheckerContext &C, const CallExpr *CE) const {
   //int strncmp(const char *s1, const char *s2, size_t n);
-  evalStrcmpCommon(C, CE, /* isBounded = */ true, /* ignoreCase = */ false);
+  evalStrcmpCommon(C, CE, /* IsBounded = */ true, /* IgnoreCase = */ false);
 }
 
 void CStringChecker::evalStrcasecmp(CheckerContext &C,
     const CallExpr *CE) const {
   //int strcasecmp(const char *s1, const char *s2);
-  evalStrcmpCommon(C, CE, /* isBounded = */ false, /* ignoreCase = */ true);
+  evalStrcmpCommon(C, CE, /* IsBounded = */ false, /* IgnoreCase = */ true);
 }
 
 void CStringChecker::evalStrncasecmp(CheckerContext &C,
     const CallExpr *CE) const {
   //int strncasecmp(const char *s1, const char *s2, size_t n);
-  evalStrcmpCommon(C, CE, /* isBounded = */ true, /* ignoreCase = */ true);
+  evalStrcmpCommon(C, CE, /* IsBounded = */ true, /* IgnoreCase = */ true);
 }
 
 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();
 
   // 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, 1);
void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE,
   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) {
+    if (IsBounded) {
       // Get the max number of characters to compare.
       const Expr *lenExpr = CE->getArg(2);
       SVal lenVal = state->getSVal(lenExpr, LCtx);
 
       // If the length is known, we can get the right substrings.
       if (const llvm::APSInt *len = svalBuilder.getKnownValue(state, lenVal)) {
         // Create substrings of each to compare the prefix.
         s1StrRef = s1StrRef.substr(0, (size_t)len->getZExtValue());
if (canComputeResult) {
       if (s1Term != StringRef::npos)
         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)
+      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 {

 void CStringChecker::evalBzero(CheckerContext &C, const CallExpr *CE) const {
   CurrentFunctionDescription = "memory clearance function";
 
   const Expr *Mem = CE->getArg(0);
   const Expr *Size = CE->getArg(1);
   SVal Zero = C.getSValBuilder().makeZeroVal(C.getASTContext().IntTy);
 
   ProgramStateRef State = C.getState();
-  
   // See if the size argument is zero.
   SVal SizeVal = C.getSVal(Size);
   QualType SizeTy = Size->getType();
 
   ProgramStateRef StateZeroSize, StateNonZeroSize;
   std::tie(StateZeroSize, StateNonZeroSize) =
     assumeZero(C, State, SizeVal, SizeTy);
 

clang/test/Analysis/bsd-string.c

 // RUN: %clang_analyze_cc1 -verify %s \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=unix.cstring.NullArg \
 // RUN:   -analyzer-checker=alpha.unix.cstring \
 // RUN:   -analyzer-checker=debug.ExprInspection
 
 #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);
+size_t strlen(const char *s);
 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
-  // FIXME: This should not warn. The string is safely truncated.
-  strlcat(buf, "efgh", sizeof(buf)); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
+  size_t len;
+  len = strlcpy(buf, "abcd", sizeof(buf)); // expected-no-warning
+  clang_analyzer_eval(len == 4); // expected-warning{{TRUE}}
+  len = strlcat(buf, "efgh", sizeof(buf)); // expected-no-warning
+  clang_analyzer_eval(len == 8); // expected-warning{{TRUE}}
 }
 
 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 f6() {
   size_t len = strlcat(buf, "defg", 4);
   clang_analyzer_eval(len == 7); // expected-warning{{TRUE}}
 }
 
 int f7() {
   char buf[8];
   return strlcpy(buf, "1234567", 0); // no-crash
 }
+
+void f8(){
+  char buf[5];
+  size_t len;
+
+  // basic strlcpy
+  len = strlcpy(buf,"123", sizeof(buf));
+  clang_analyzer_eval(len==3);// expected-warning{{TRUE}}
+  len = strlen(buf);
+  clang_analyzer_eval(len==3);// expected-warning{{TRUE}}
+
+  // testing bounded strlcat
+  len = strlcat(buf,"456", sizeof(buf));
+  clang_analyzer_eval(len==6);// expected-warning{{TRUE}}
+  len = strlen(buf);
+  clang_analyzer_eval(len==4);// expected-warning{{TRUE}}
+
+  // testing strlcat with size==0
+  len = strlcat(buf,"789", 0);
+  clang_analyzer_eval(len==7);// expected-warning{{TRUE}}
+  len = strlen(buf);
+  clang_analyzer_eval(len==4);// expected-warning{{TRUE}}
+
+  // testing strlcpy with size==0
+  len = strlcpy(buf,"123",0);
+  clang_analyzer_eval(len==3);// expected-warning{{TRUE}}
+  len = strlen(buf);
+  clang_analyzer_eval(len==4);// expected-warning{{TRUE}}
+
+}
+
+void f9(int unknown_size, char* unknown_src, char* unknown_dst){
+  char buf[8];
+  size_t len;
+
+  len = strlcpy(buf,"abba",sizeof(buf));
+
+  clang_analyzer_eval(len==4);// expected-warning{{TRUE}}
+  clang_analyzer_eval(strlen(buf)==4);// expected-warning{{TRUE}}
+
+  //size is unknown
+  len = strlcat(buf,"cd", unknown_size);
+  clang_analyzer_eval(len==6);// expected-warning{{TRUE}}
+  clang_analyzer_eval(strlen(buf)>=4);// expected-warning{{TRUE}}
+
+  //dst is unknown
+  len = strlcpy(unknown_dst,"abbc",unknown_size);
+  clang_analyzer_eval(len==4);// expected-warning{{TRUE}}
+  clang_analyzer_eval(strlen(unknown_dst));// expected-warning{{UNKNOWN}}
+
+  //src is unknown
+  len = strlcpy(buf,unknown_src, sizeof(buf));
+  clang_analyzer_eval(len);// expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(strlen(buf));// expected-warning{{UNKNOWN}}
+
+  //src, dst is unknown
+  len = strlcpy(unknown_dst, unknown_src, unknown_size);
+  clang_analyzer_eval(len);// expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(strlen(unknown_dst));// expected-warning{{UNKNOWN}}
+
+  //size is unknown
+  len = strlcat(buf+2,unknown_src+1, sizeof(buf));// expected-warning{{Size argument is greater than the length of the destination buffer}};
+}
+
+void f10(){
+  char buf[8];
+  size_t len;
+
+  len = strlcpy(buf,"abba",sizeof(buf));
+  clang_analyzer_eval(len==4);// expected-warning{{TRUE}}
+  strlcat(buf, "efghi",9);// expected-warning{{Size argument is greater than the length of the destination buffer}}
+}
+
+void f11() {
+  //test for Bug 41729
+  char a[256], b[256];
+  strlcpy(a, "world", sizeof(a));
+  strlcpy(b, "hello ", sizeof(b));
+  strlcat(b, a, sizeof(b)); // no-warning
+}