[PATCH] D57981: [analyzer] strlcat() syntax check: Fix an off-by-one error.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, george.karpenkov, xazax.hun, a_sidorin, 
rnkovacs, mikhail.ramalho, Szelethus, baloghadamsoftware, devnexen.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, a.sidorin, szepet.
Herald added a project: clang.

That's a fix on top of D49722 <https://reviews.llvm.org/D49722>.

Both `strlcat` and `strlcpy` cut off their safe bound argument value at 
`sizeof(destination)`. There's no need to subtract 1 in one of these cases.


Repository:
  rC Clang

https://reviews.llvm.org/D57981

Files:
  lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
  test/Analysis/cstring-syntax.c


Index: test/Analysis/cstring-syntax.c
===================================================================
--- test/Analysis/cstring-syntax.c
+++ test/Analysis/cstring-syntax.c
@@ -33,6 +33,7 @@
   strlcpy(dest, src, ulen);
   strlcpy(dest + 5, src, 5);
   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}}
+  strlcpy(dest, "aaaaaaaaaaaaaaa", 10); // no-warning
 }
 
 void testStrlcat(const char *src) {
@@ -51,4 +52,5 @@
   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}}
+  strlcat(dest, "aaaaaaaaaaaaaaa", 10); // no-warning
 }
Index: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -153,8 +153,6 @@
 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);
 
@@ -194,13 +192,8 @@
         ASTContext &C = BR.getContext();
         uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
         auto RemainingBufferLen = BufferLen - DstOff;
-        if (Append) {
-          if (RemainingBufferLen <= ILRawVal)
-            return true;
-        } else {
-          if (RemainingBufferLen < ILRawVal)
-            return true;
-        }
+        if (RemainingBufferLen < ILRawVal)
+          return true;
       }
     }
   }



Index: test/Analysis/cstring-syntax.c
===================================================================
--- test/Analysis/cstring-syntax.c
+++ test/Analysis/cstring-syntax.c
@@ -33,6 +33,7 @@
   strlcpy(dest, src, ulen);
   strlcpy(dest + 5, src, 5);
   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}}
+  strlcpy(dest, "aaaaaaaaaaaaaaa", 10); // no-warning
 }
 
 void testStrlcat(const char *src) {
@@ -51,4 +52,5 @@
   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}}
+  strlcat(dest, "aaaaaaaaaaaaaaa", 10); // no-warning
 }
Index: lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringSyntaxChecker.cpp
@@ -153,8 +153,6 @@
 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);
 
@@ -194,13 +192,8 @@
         ASTContext &C = BR.getContext();
         uint64_t BufferLen = C.getTypeSize(Buffer) / 8;
         auto RemainingBufferLen = BufferLen - DstOff;
-        if (Append) {
-          if (RemainingBufferLen <= ILRawVal)
-            return true;
-        } else {
-          if (RemainingBufferLen < ILRawVal)
-            return true;
-        }
+        if (RemainingBufferLen < ILRawVal)
+          return true;
       }
     }
   }

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits