[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: george.karpenkov, NoQ, MaskRay

Reviewed By: MaskRay

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

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

diff --git a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
index 8b4aa857e7752e9b26ddada3fb6f48de81c9f704..b486443ac06d3bcfbd290945f97301e332b75df5 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,19 @@ 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;
+  if (isSizeof(LenArg, DstArg))
+    return false;
   // - size_t dstlen = sizeof(dst)
   if (LenArgDecl) {
     const auto *LenArgVal = dyn_cast<VarDecl>(LenArgDecl->getDecl());
@@ -181,8 +194,14 @@ 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)
-          return true;
+        auto RemainingBufferLen = BufferLen - DstOff;
+        if (Append) {
+          if (RemainingBufferLen <= ILRawVal)
+            return true;
+        } else {
+          if (RemainingBufferLen < ILRawVal)
+            return true;
+        }
       }
     }
   }
@@ -219,8 +238,9 @@ void WalkAST::VisitCallExpr(CallExpr *CE) {
                          "C String API", os.str(), Loc,
                          LenArg->getSourceRange());
     }
-  } else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) {
-    if (containsBadStrlcpyPattern(CE)) {
+  } else if (CheckerContext::isCLibraryFunction(FD, "strlcpy") ||
+             CheckerContext::isCLibraryFunction(FD, "strlcat")) {
+    if (containsBadStrlcpyStrlcatPattern(CE)) {
       const Expr *DstArg = CE->getArg(0);
       const Expr *LenArg = CE->getArg(2);
       PathDiagnosticLocation Loc =
@@ -230,13 +250,17 @@ void WalkAST::VisitCallExpr(CallExpr *CE) {
 
       SmallString<256> S;
       llvm::raw_svector_ostream os(S);
-      os << "The third argument is larger than the size of the input buffer. ";
+      os << "The third argument allows to potentially copy more bytes than it should. ";
+      os << "Replace with the value ";
       if (!DstName.empty())
-        os << "Replace with the value 'sizeof(" << DstName << ")` or lower";
+          os << "sizeof(" << DstName << ")";
+      else
+          os << "sizeof(<destination buffer>)";
+      os << " or lower";
 
       BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument",
-                         "C String API", os.str(), Loc,
-                         LenArg->getSourceRange());
+              "C String API", os.str(), Loc,
+              LenArg->getSourceRange());
     }
   }
 

diff --git a/test/Analysis/cstring-syntax.c b/test/Analysis/cstring-syntax.c
index fe1253bedba3cd4ac315c7cfd07c0a218cbad270..d2e12e8303b15ce440eb42919b0d9de9beab018f 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];
@@ -27,9 +28,27 @@ void testStrlcpy(const char *src) {
   strlcpy(dest, src, sizeof(dest));
   strlcpy(dest, src, destlen);
   strlcpy(dest, src, 10);
-  strlcpy(dest, src, 20); // expected-warning {{The third argument is larger than the size of the input buffer. Replace with the value 'sizeof(dest)` or lower}}
-  strlcpy(dest, src, badlen); // expected-warning {{The third argument is larger than the size of the input buffer. Replace with the value 'sizeof(dest)` or lower}}
+  strlcpy(dest, src, 20); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) or lower}}
+  strlcpy(dest, src, badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) or lower}}
   strlcpy(dest, src, ulen);
   strlcpy(dest + 5, src, 5);
-  strlcpy(dest + 5, src, 10); // expected-warning {{The third argument is larger than the size of the input buffer.}}
+  strlcpy(dest + 5, src, 10); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(<destination buffer>) or lower}}
+}
+
+void testStrlcat(const char *src) {
+  char dest[10];
+  size_t badlen = 20;
+  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));
+  strlcpy(dest, "0123456789", sizeof(dest));
+  strlcpy(dest, "0123456789", sizeof(dest));
+  strlcat(dest, "0123456789", badlen / 2);
+  strlcat(dest, "0123456789", badlen); // expected-warning {{The third argument allows to potentially copy more bytes than it should. Replace with the value sizeof(dest) 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 sizeof(<destination buffer>) or lower}}
 }