[analyzer] Re-apply r331096 "CStringChecker: Add support for BSD strlcpy()...".

Fixed after revert in r331401.

Patch by David Carlier!

Differential Revision: https://reviews.llvm.org/D45177

llvm-svn: 332303

diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
index 284d7deda878dfad7da35928619332624f249aa5..f97c513f371a6525a6bc8acefdb16ca7465f0afb 100644 (file)
--- a/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -97,14 +97,17 @@ public:
   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 @@ void CStringChecker::evalStpcpy(CheckerContext &C, const CallExpr *CE) const {
                    /* 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 @@ void CStringChecker::evalStrncat(CheckerContext &C, const CallExpr *CE) const {
                    /* 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 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
   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 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
     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<loc::MemRegionVal> dstRegVal =
-          DstVal.getAs<loc::MemRegionVal>()) {
+      DstVal.getAs<loc::MemRegionVal>()) {
     QualType ptrTy = Dst->getType();
 
     // If we have an exact value on a bounded copy, use that to check for
@@ -1675,9 +1713,9 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
     if (boundWarning) {
       if (Optional<NonLoc> maxLastNL = maxLastElementIndex.getAs<NonLoc>()) {
         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 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
     // Then, if the final length is known...
     if (Optional<NonLoc> knownStrLength = finalStrLength.getAs<NonLoc>()) {
       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 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
       }
 
       // 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 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
     // 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 @@ void CStringChecker::evalStrcpyCommon(CheckerContext &C, const CallExpr *CE,
 
   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::evalStrncmp(CheckerContext &C, const CallExpr *CE) const {
 }
 
 void CStringChecker::evalStrcasecmp(CheckerContext &C,
-                                    const CallExpr *CE) const {
+    const CallExpr *CE) const {
   if (CE->getNumArgs() < 2)
     return;
 
@@ -1768,7 +1807,7 @@ void CStringChecker::evalStrcasecmp(CheckerContext &C,
 }
 
 void CStringChecker::evalStrncasecmp(CheckerContext &C,
-                                     const CallExpr *CE) const {
+    const CallExpr *CE) const {
   if (CE->getNumArgs() < 3)
     return;
 
@@ -1777,7 +1816,7 @@ void CStringChecker::evalStrncasecmp(CheckerContext &C,
 }
 
 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 @@ void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE,
   // 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 @@ void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE,
   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 @@ void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE,
 
       // 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 @@ void CStringChecker::evalStrcmpCommon(CheckerContext &C, const CallExpr *CE,
         BinaryOperatorKind op = (compareRes == 1) ? BO_GT : BO_LT;
         SVal compareWithZero =
           svalBuilder.evalBinOp(state, op, resultVal, zeroVal,
-                                svalBuilder.getConditionType());
+              svalBuilder.getConditionType());
         DefinedSVal compareWithZeroVal = compareWithZero.castAs<DefinedSVal>();
         state = state->assume(compareWithZeroVal, true);
       }
@@ -1942,17 +1981,17 @@ void CStringChecker::evalStrsep(CheckerContext &C, const CallExpr *CE) const {
     // 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::evalStdCopy(CheckerContext &C, const CallExpr *CE) const {
 }
 
 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 @@ void CStringChecker::evalStdCopyCommon(CheckerContext &C,
   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 @@ void CStringChecker::evalMemset(CheckerContext &C, const CallExpr *CE) const {
   if (!State)
     return;
   State = InvalidateBuffer(C, State, Mem, C.getSVal(Mem),
-                           /*IsSourceBuffer*/false, Size);
+      /*IsSourceBuffer*/false, Size);
   if (!State)
     return;
 
@@ -2091,10 +2130,14 @@ bool CStringChecker::evalCall(const CallExpr *CE, CheckerContext &C) const {
     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 @@ void CStringChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
     SVal StrVal = C.getSVal(Init);
     assert(StrVal.isValid() && "Initializer string is unknown or undefined");
     DefinedOrUnknownSVal strLength =
-        getCStringLength(C, state, Init, StrVal).castAs<DefinedOrUnknownSVal>();
+      getCStringLength(C, state, Init, StrVal).castAs<DefinedOrUnknownSVal>();
 
     state = state->set<CStringLength>(MR, strLength);
   }
@@ -2171,11 +2214,11 @@ void CStringChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
 
 ProgramStateRef
 CStringChecker::checkRegionChanges(ProgramStateRef state,
-                                   const InvalidatedSymbols *,
-                                   ArrayRef<const MemRegion *> ExplicitRegions,
-                                   ArrayRef<const MemRegion *> Regions,
-                                   const LocationContext *LCtx,
-                                   const CallEvent *Call) const {
+    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;
@@ -2185,7 +2228,7 @@ CStringChecker::checkRegionChanges(ProgramStateRef state,
 
   // 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) {
+      I = Regions.begin(), E = Regions.end(); I != E; ++I) {
     const MemRegion *MR = *I;
     Invalidated.insert(MR);
 
@@ -2200,7 +2243,7 @@ CStringChecker::checkRegionChanges(ProgramStateRef state,
 
   // 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 @@ CStringChecker::checkRegionChanges(ProgramStateRef state,
 }
 
 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<CStringLength>();
 
   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 @@ void CStringChecker::checkDeadSymbols(SymbolReaper &SR,
 
   CStringLengthTy::Factory &F = state->get_context<CStringLength>();
   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 @@ void CStringChecker::checkDeadSymbols(SymbolReaper &SR,
     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);
+  }

diff --git a/clang/test/Analysis/bsd-string.c b/clang/test/Analysis/bsd-string.c
new file mode 100644 (file)
index 0000000..4c57c86
--- /dev/null
+++ b/clang/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}}
+}