[CStringSyntaxChecker] Check strlcat sizeof check

- Assuming strlcat is used with strlcpy we check as we can if the last argument does not equal os not larger than the buffer.
- Advising the proper usual pattern.

Reviewers: NoQ, george.karpenkov

Reviewed By: george.karpenkov

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

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@339641 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
index 8b4aa857e7752e9b26ddada3fb6f48de81c9f704..2763408facea7fe122efb578d4bf53ee2c5df033 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -90,7 +90,16 @@ class WalkAST: public StmtVisitor<WalkAST> {
   ///   strlcpy(dst, "abcd", 4);
   ///   strlcpy(dst + 3, "abcd", 2);
   ///   strlcpy(dst, "abcd", cpy);
-  bool containsBadStrlcpyPattern(const CallExpr *CE);
+  /// Identify erroneous patterns in the last argument to strlcat - the number
+  /// of bytes to copy.
+  /// The bad pattern checked is when the last argument is basically
+  /// pointing to the destination buffer size or argument larger or
+  /// equal to.  
+  ///   char dst[2];
+  ///   strlcat(dst, src2, sizeof(dst));
+  ///   strlcat(dst, src2, 2);
+  ///   strlcat(dst, src2, 10);
+  bool containsBadStrlcpyStrlcatPattern(const CallExpr *CE);
 
 public:
   WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
@@ -142,15 +151,21 @@ bool WalkAST::containsBadStrncatPattern(const CallExpr *CE) {
   return false;
 }
 
-bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
+bool WalkAST::containsBadStrlcpyStrlcatPattern(const CallExpr *CE) {
   if (CE->getNumArgs() != 3)
     return false;
+  const FunctionDecl *FD = CE->getDirectCallee();
+  bool Append = CheckerContext::isCLibraryFunction(FD, "strlcat");
   const Expr *DstArg = CE->getArg(0);
   const Expr *LenArg = CE->getArg(2);
 
   const auto *DstArgDecl = dyn_cast<DeclRefExpr>(DstArg->IgnoreParenImpCasts());
   const auto *LenArgDecl = dyn_cast<DeclRefExpr>(LenArg->IgnoreParenLValueCasts());
   uint64_t DstOff = 0;
+  // - sizeof(dst)
+  // strlcat appends at most size - strlen(dst) - 1
+  if (Append && isSizeof(LenArg, DstArg))
+    return true;
   // - size_t dstlen = sizeof(dst)
   if (LenArgDecl) {
     const auto *LenArgVal = dyn_cast<VarDecl>(LenArgDecl->getDecl());
@@ -181,7 +196,10 @@ bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
       if (const auto *Buffer = dyn_cast<ConstantArrayType>(DstArgDecl->getType())) {
         ASTContext &C = BR.getContext();
         uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
-        if ((BufferLen - DstOff) < ILRawVal)
+        auto RemainingBufferLen = BufferLen - DstOff;
+        if (Append)
+          RemainingBufferLen -= 1;
+        if (RemainingBufferLen < ILRawVal)
           return true;
       }
     }
@@ -220,7 +238,7 @@ void WalkAST::VisitCallExpr(CallExpr *CE) {
                          LenArg->getSourceRange());
     }
   } else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) {
-    if (containsBadStrlcpyPattern(CE)) {
+    if (containsBadStrlcpyStrlcatPattern(CE)) {
       const Expr *DstArg = CE->getArg(0);
       const Expr *LenArg = CE->getArg(2);
       PathDiagnosticLocation Loc =
@@ -234,6 +252,34 @@ void WalkAST::VisitCallExpr(CallExpr *CE) {
       if (!DstName.empty())
         os << "Replace with the value 'sizeof(" << DstName << ")` or lower";
 
+      BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument",
+                         "C String API", os.str(), Loc,
+                         LenArg->getSourceRange());
+    }
+  } else if (CheckerContext::isCLibraryFunction(FD, "strlcat")) {
+    if (containsBadStrlcpyStrlcatPattern(CE)) {
+      const Expr *DstArg = CE->getArg(0);
+      const Expr *LenArg = CE->getArg(2);
+      PathDiagnosticLocation Loc =
+        PathDiagnosticLocation::createBegin(LenArg, BR.getSourceManager(), AC);
+
+      StringRef DstName = getPrintableName(DstArg);
+      StringRef LenName = getPrintableName(LenArg);
+
+      SmallString<256> S;
+      llvm::raw_svector_ostream os(S);
+      os << "The third argument allows to potentially copy more bytes than it should. ";
+      os << "Replace with the value ";
+      if (!LenName.empty())
+        os << "'" << LenName << "'";
+      else
+        os << " <size> ";
+      if (!DstName.empty())
+        os << " - strlen(" << DstName << ")";
+      else
+        os << " - strlen(<destination buffer>)";
+      os << " - 1 or lower";
+
       BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument",
                          "C String API", os.str(), Loc,
                          LenArg->getSourceRange());

diff --git a/test/Analysis/cstring-syntax.c b/test/Analysis/cstring-syntax.c
index fe1253bedba3cd4ac315c7cfd07c0a218cbad270..c6d36940605626343ade5947a3eac56f629ad2e1 100644 (file)
--- a/test/Analysis/cstring-syntax.c
+++ b/test/Analysis/cstring-syntax.c
@@ -7,6 +7,7 @@ typedef __SIZE_TYPE__ size_t;
 char  *strncat(char *, const char *, size_t);
 size_t strlen (const char *s);
 size_t strlcpy(char *, const char *, size_t);
+size_t strlcat(char *, const char *, size_t);
 
 void testStrncat(const char *src) {
   char dest[10];
@@ -33,3 +34,19 @@ void testStrlcpy(const char *src) {
   strlcpy(dest + 5, src, 5);
   strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}}
 }
+
+void testStrlcat(const char *src) {
+  char dest[10];
+  size_t badlen = 10;
+  size_t ulen;
+  strlcpy(dest, "aaaaa", sizeof("aaaaa") - 1);
+  strlcat(dest, "bbbb", (sizeof("bbbb") - 1) - sizeof(dest) - 1);
+  strlcpy(dest, "012345678", sizeof(dest));
+  strlcat(dest, "910", sizeof(dest)); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value  <size>  - strlen(dest) - 1 or lower}}
+  strlcpy(dest, "0123456789", sizeof(dest));
+  strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen(dest) - 1 or lower}}
+  strlcat(dest, "0123456789", badlen - strlen(dest) - 1);
+  strlcat(dest, src, ulen);
+  strlcpy(dest, src, 5);
+  strlcat(dest + 5, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value 'badlen' - strlen(<destination buffer>) - 1 or lower}}
+}