From: David Carlier <devnexen@gmail.com>
Date: Thu, 19 Jul 2018 21:50:03 +0000 (+0000)
Subject: [CStringSyntaxChecker] Check strlcpy sizeof syntax
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=73ce27a67980d4bb3d56c34a86d362de70d6fe45;p=clang

[CStringSyntaxChecker] Check strlcpy sizeof syntax

The last argument is expected to be the destination buffer size (or less).

    Detects if it points to destination buffer size directly or via a variable.
    Detects if it is an integral, try to detect if the destination buffer can receive the source length.

Updating bsd-string.c unit tests as it make it fails now.

Reviewers: george.karpenpov, NoQ

Reviewed By: george.karpenkov

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


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

diff --git a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
index 4b5e97b692..633e9724b6 100644
--- a/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -80,6 +80,17 @@ class WalkAST: public StmtVisitor<WalkAST> {
   /// of bytes to copy.
   bool containsBadStrncatPattern(const CallExpr *CE);
 
+  /// Identify erroneous patterns in the last argument to strlcpy - the number
+  /// of bytes to copy.
+  /// The bad pattern checked is when the size is known
+  /// to be larger than the destination can handle.
+  ///   char dst[2];
+  ///   size_t cpy = 4;
+  ///   strlcpy(dst, "abcd", sizeof("abcd") - 1);
+  ///   strlcpy(dst, "abcd", 4);
+  ///   strlcpy(dst, "abcd", cpy);
+  bool containsBadStrlcpyPattern(const CallExpr *CE);
+
 public:
   WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
       : Checker(Checker), BR(BR), AC(AC) {}
@@ -130,6 +141,38 @@ bool WalkAST::containsBadStrncatPattern(const CallExpr *CE) {
   return false;
 }
 
+bool WalkAST::containsBadStrlcpyPattern(const CallExpr *CE) {
+  if (CE->getNumArgs() != 3)
+    return false;
+  const Expr *DstArg = CE->getArg(0);
+  const Expr *LenArg = CE->getArg(2);
+
+  const auto *DstArgDecl = dyn_cast<DeclRefExpr>(DstArg->IgnoreParenCasts());
+  const auto *LenArgDecl = dyn_cast<DeclRefExpr>(LenArg->IgnoreParenLValueCasts());
+  // - size_t dstlen = sizeof(dst)
+  if (LenArgDecl) {
+    const auto *LenArgVal = dyn_cast<VarDecl>(LenArgDecl->getDecl());
+    if (LenArgVal->getInit())
+	    LenArg = LenArgVal->getInit();
+  }
+
+  // - integral value
+  // We try to figure out if the last argument is possibly longer
+  // than the destination can possibly handle if its size can be defined
+  if (const auto *IL = dyn_cast<IntegerLiteral>(LenArg->IgnoreParenCasts())) {
+    uint64_t ILRawVal = IL->getValue().getZExtValue();
+    if (const auto *Buffer = dyn_cast<ConstantArrayType>(DstArgDecl->getType())) {
+      ASTContext &C = BR.getContext();
+      uint64_t Usize = C.getTypeSizeInChars(DstArg->getType()).getQuantity();
+      uint64_t BufferLen = BR.getContext().getTypeSize(Buffer) / Usize;
+      if (BufferLen < ILRawVal)
+        return true;
+    }
+  }
+
+  return false;
+}
+
 void WalkAST::VisitCallExpr(CallExpr *CE) {
   const FunctionDecl *FD = CE->getDirectCallee();
   if (!FD)
@@ -155,6 +198,25 @@ void WalkAST::VisitCallExpr(CallExpr *CE) {
         os << "U";
       os << "se a safer 'strlcat' API";
 
+      BR.EmitBasicReport(FD, Checker, "Anti-pattern in the argument",
+                         "C String API", os.str(), Loc,
+                         LenArg->getSourceRange());
+    }
+  } else if (CheckerContext::isCLibraryFunction(FD, "strlcpy")) {
+    if (containsBadStrlcpyPattern(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);
+
+      SmallString<256> S;
+      llvm::raw_svector_ostream os(S);
+      os << "The third argument is larger than the size of the input buffer. ";
+      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());
diff --git a/test/Analysis/bsd-string.c b/test/Analysis/bsd-string.c
index 14e1b00fc0..bca42ca896 100644
--- a/test/Analysis/bsd-string.c
+++ b/test/Analysis/bsd-string.c
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring.NullArg,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -verify %s
 
 #define NULL ((void *)0)
 
diff --git a/test/Analysis/cstring-syntax.c b/test/Analysis/cstring-syntax.c
index 313ac54495..d7df3f082c 100644
--- a/test/Analysis/cstring-syntax.c
+++ b/test/Analysis/cstring-syntax.c
@@ -3,6 +3,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);
 
 void testStrncat(const char *src) {
   char dest[10];
@@ -13,3 +14,17 @@ void testStrncat(const char *src) {
   // Should not crash when sizeof has a type argument.
   strncat(dest, "AAAAAAAAAAAAAAAAAAAAAAAAAAA", sizeof(char));
 }
+
+void testStrlcpy(const char *src) {
+  char dest[10];
+  size_t destlen = sizeof(dest);
+  size_t srclen = sizeof(src);
+  size_t badlen = 20;
+  size_t ulen;
+  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, ulen);
+}