[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

llvm-svn: 342832

1 files changed, 34 insertions, 10 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
index 8b4aa857e77..b486443ac06 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ b/clang/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());
     }
   }