Warn on mismatched parentheses in memcmp and friends.

Thisadds a new warning that warns on code like this:

  if (memcmp(a, b, sizeof(a) != 0))

The warning looks like:

test4.cc:5:30: warning: size argument in 'memcmp' call is a comparison [-Wmemsize-comparison]
  if (memcmp(a, b, sizeof(a) != 0))
                   ~~~~~~~~~~^~~~
test4.cc:5:7: note: did you mean to compare the result of 'memcmp' instead?
  if (memcmp(a, b, sizeof(a) != 0))
      ^                          ~
                            )
test4.cc:5:20: note: explicitly cast the argument to size_t to silence this warning
  if (memcmp(a, b, sizeof(a) != 0))
                   ^
                   (size_t)(     )
1 warning generated.

This found 2 bugs in chromium and has 0 false positives on both chromium and
llvm.

The idea of triggering this warning on a binop in the size argument is due to
rnk.

llvm-svn: 198063

3 files changed, 150 insertions, 4 deletions

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c3ac6fbd78e..461d46736ea 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -390,11 +390,18 @@ def warn_sizeof_pointer_type_memaccess : Warning<
   "%select{destination|source}2; expected %3 or an explicit length">,
   InGroup<SizeofPointerMemaccess>;
 def warn_strlcpycat_wrong_size : Warning<
-  "size argument in %0 call appears to be size of the source; expected the size of "
-  "the destination">,
+  "size argument in %0 call appears to be size of the source; "
+  "expected the size of the destination">,
   InGroup<DiagGroup<"strlcpy-strlcat-size">>;
 def note_strlcpycat_wrong_size : Note<
   "change size argument to be the size of the destination">;
+def warn_memsize_comparison : Warning<
+  "size argument in %0 call is a comparison">,
+  InGroup<DiagGroup<"memsize-comparison">>;
+def warn_memsize_comparison_paren_note : Note<
+  "did you mean to compare the result of %0 instead?">;
+def warn_memsize_comparison_cast_note : Note<
+  "explicitly cast the argument to size_t to silence this warning">;
   
 def warn_strncat_large_size : Warning<
   "the value of the size argument in 'strncat' is too large, might lead to a " 
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 97c041c9013..feee61ea5ec 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -622,7 +622,7 @@ bool Sema::CheckARMBuiltinFunctionCall(unsigned BuiltinID, CallExpr *TheCall) {
                                  RHS.get(), AA_Assigning))
       return true;
   }
-  
+
   // For NEON intrinsics which take an immediate value as part of the 
   // instruction, range check them here.
   unsigned i = 0, l = 0, u = 0;
@@ -3560,6 +3560,40 @@ void Sema::CheckFormatString(const StringLiteral *FExpr,
 
 //===--- CHECK: Standard memory functions ---------------------------------===//
 
+/// \brief Takes the expression passed to the size_t parameter of functions
+/// such as memcmp, strncat, etc and warns if it's a comparison.
+///
+/// This is to catch typos like `if (memcmp(&a, &b, sizeof(a) > 0))`.
+static bool CheckMemorySizeofForComparison(Sema &S, const Expr *E,
+                                           IdentifierInfo *FnName,
+                                           SourceLocation FnLoc,
+                                           SourceLocation RParenLoc) {
+  const BinaryOperator *Size = dyn_cast<BinaryOperator>(E);
+  if (!Size)
+    return false;
+
+  // if E is binop and op is >, <, >=, <=, ==, &&, ||:
+  if (!Size->isComparisonOp() && !Size->isEqualityOp() && !Size->isLogicalOp())
+    return false;
+
+  Preprocessor &PP = S.getPreprocessor();
+  SourceRange SizeRange = Size->getSourceRange();
+  S.Diag(Size->getOperatorLoc(), diag::warn_memsize_comparison)
+      << SizeRange << FnName;
+  S.Diag(FnLoc, diag::warn_memsize_comparison_paren_note)
+      << FnName
+      << FixItHint::CreateInsertion(
+             PP.getLocForEndOfToken(Size->getLHS()->getLocEnd()),
+             ")")
+      << FixItHint::CreateRemoval(RParenLoc);
+  S.Diag(SizeRange.getBegin(), diag::warn_memsize_comparison_cast_note)
+      << FixItHint::CreateInsertion(SizeRange.getBegin(), "(size_t)(")
+      << FixItHint::CreateInsertion(
+             PP.getLocForEndOfToken(SizeRange.getEnd()), ")");
+
+  return true;
+}
+
 /// \brief Determine whether the given type is a dynamic class type (e.g.,
 /// whether it has a vtable).
 static bool isDynamicClassType(QualType T) {
@@ -3615,6 +3649,10 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
   unsigned LenArg = (BId == Builtin::BIstrndup ? 1 : 2);
   const Expr *LenExpr = Call->getArg(LenArg)->IgnoreParenImpCasts();
 
+  if (CheckMemorySizeofForComparison(*this, LenExpr, FnName,
+                                     Call->getLocStart(), Call->getRParenLoc()))
+    return;
+
   // We have special checking when the length is a sizeof expression.
   QualType SizeOfArgTy = getSizeOfArgType(LenExpr);
   const Expr *SizeOfArg = getSizeOfExprArg(LenExpr);
@@ -3798,6 +3836,10 @@ void Sema::CheckStrlcpycatArguments(const CallExpr *Call,
   const Expr *SrcArg = ignoreLiteralAdditions(Call->getArg(1), Context);
   const Expr *SizeArg = ignoreLiteralAdditions(Call->getArg(2), Context);
   const Expr *CompareWithSrc = NULL;
+
+  if (CheckMemorySizeofForComparison(*this, SizeArg, FnName,
+                                     Call->getLocStart(), Call->getRParenLoc()))
+    return;
   
   // Look for 'strlcpy(dst, x, sizeof(x))'
   if (const Expr *Ex = getSizeOfExprArg(SizeArg))
@@ -3880,6 +3922,10 @@ void Sema::CheckStrncatArguments(const CallExpr *CE,
   const Expr *SrcArg = CE->getArg(1)->IgnoreParenCasts();
   const Expr *LenArg = CE->getArg(2)->IgnoreParenCasts();
 
+  if (CheckMemorySizeofForComparison(*this, LenArg, FnName, CE->getLocStart(),
+                                     CE->getRParenLoc()))
+    return;
+
   // Identify common expressions, which are wrongly used as the size argument
   // to strncat and may lead to buffer overflows.
   unsigned PatternType = 0;
@@ -5235,7 +5281,7 @@ void CheckImplicitConversion(Sema &S, Expr *E, QualType T,
   if (Target->isSpecificBuiltinType(BuiltinType::Bool)) {
     if (isa<StringLiteral>(E))
       // Warn on string literal to bool.  Checks for string literals in logical
-      // expressions, for instances, assert(0 && "error here"), is prevented
+      // expressions, for instances, assert(0 && "error here"), are prevented
       // by a check in AnalyzeImplicitConversions().
       return DiagnoseImpCast(S, E, T, CC,
                              diag::warn_impcast_string_literal_to_bool);
diff --git a/clang/test/SemaCXX/warn-memsize-comparison.cpp b/clang/test/SemaCXX/warn-memsize-comparison.cpp
new file mode 100644
index 00000000000..54c410e3dc0
--- /dev/null
+++ b/clang/test/SemaCXX/warn-memsize-comparison.cpp
@@ -0,0 +1,93 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+//
+
+typedef __SIZE_TYPE__ size_t;
+extern "C" void *memset(void *, int, size_t);
+extern "C" void *memmove(void *s1, const void *s2, size_t n);
+extern "C" void *memcpy(void *s1, const void *s2, size_t n);
+extern "C" void *memcmp(void *s1, const void *s2, size_t n);
+extern "C" int strncmp(const char *s1, const char *s2, size_t n);
+extern "C" int strncasecmp(const char *s1, const char *s2, size_t n);
+extern "C" char *strncpy(char *dst, const char *src, size_t n);
+extern "C" char *strncat(char *dst, const char *src, size_t n);
+extern "C" char *strndup(const  char *src, size_t n);
+extern "C" size_t strlcpy(char *dst, const char *src, size_t size);
+extern "C" size_t strlcat(char *dst, const char *src, size_t size);
+
+void f() {
+  char b1[80], b2[80];
+  if (memset(b1, 0, sizeof(b1) != 0)) {} // \
+    expected-warning{{size argument in 'memset' call is a comparison}} \
+    expected-note {{did you mean to compare}} \
+    expected-note {{explicitly cast the argument}}
+  if (memset(b1, 0, sizeof(b1)) != 0) {}
+
+  if (memmove(b1, b2, sizeof(b1) == 0)) {} // \
+    expected-warning{{size argument in 'memmove' call is a comparison}} \
+    expected-note {{did you mean to compare}} \
+    expected-note {{explicitly cast the argument}}
+  if (memmove(b1, b2, sizeof(b1)) == 0) {}
+
+  if (memcpy(b1, b2, sizeof(b1) < 0)) {} // \
+    expected-warning{{size argument in 'memcpy' call is a comparison}} \
+    expected-note {{did you mean to compare}} \
+    expected-note {{explicitly cast the argument}}
+  if (memcpy(b1, b2, sizeof(b1)) < 0) {}
+
+  if (memcmp(b1, b2, sizeof(b1) <= 0)) {} // \
+    expected-warning{{size argument in 'memcmp' call is a comparison}} \
+    expected-note {{did you mean to compare}} \
+    expected-note {{explicitly cast the argument}}
+  if (memcmp(b1, b2, sizeof(b1)) <= 0) {}
+
+  if (strncmp(b1, b2, sizeof(b1) > 0)) {} // \
+    expected-warning{{size argument in 'strncmp' call is a comparison}} \
+    expected-note {{did you mean to compare}} \
+    expected-note {{explicitly cast the argument}}
+  if (strncmp(b1, b2, sizeof(b1)) > 0) {}
+
+  if (strncasecmp(b1, b2, sizeof(b1) >= 0)) {} // \
+    expected-warning{{size argument in 'strncasecmp' call is a comparison}} \
+    expected-note {{did you mean to compare}} \
+    expected-note {{explicitly cast the argument}}
+  if (strncasecmp(b1, b2, sizeof(b1)) >= 0) {}
+
+  if (strncpy(b1, b2, sizeof(b1) == 0 || true)) {} // \
+    expected-warning{{size argument in 'strncpy' call is a comparison}} \
+    expected-note {{did you mean to compare}} \
+    expected-note {{explicitly cast the argument}}
+  if (strncpy(b1, b2, sizeof(b1)) == 0 || true) {}
+
+  if (strncat(b1, b2, sizeof(b1) - 1 >= 0 && true)) {} // \
+    expected-warning{{size argument in 'strncat' call is a comparison}} \
+    expected-note {{did you mean to compare}} \
+    expected-note {{explicitly cast the argument}}
+  if (strncat(b1, b2, sizeof(b1) - 1) >= 0 && true) {}
+
+  if (strndup(b1, sizeof(b1) != 0)) {} // \
+    expected-warning{{size argument in 'strndup' call is a comparison}} \
+    expected-note {{did you mean to compare}} \
+    expected-note {{explicitly cast the argument}}
+  if (strndup(b1, sizeof(b1)) != 0) {}
+
+  if (strlcpy(b1, b2, sizeof(b1) != 0)) {} // \
+    expected-warning{{size argument in 'strlcpy' call is a comparison}} \
+    expected-note {{did you mean to compare}} \
+    expected-note {{explicitly cast the argument}}
+  if (strlcpy(b1, b2, sizeof(b1)) != 0) {}
+
+  if (strlcat(b1, b2, sizeof(b1) != 0)) {} // \
+    expected-warning{{size argument in 'strlcat' call is a comparison}} \
+    expected-note {{did you mean to compare}} \
+    expected-note {{explicitly cast the argument}}
+  if (strlcat(b1, b2, sizeof(b1)) != 0) {}
+
+  if (memset(b1, 0, sizeof(b1) / 2)) {}
+  if (memset(b1, 0, sizeof(b1) >> 2)) {}
+  if (memset(b1, 0, 4 << 2)) {}
+  if (memset(b1, 0, 4 + 2)) {}
+  if (memset(b1, 0, 4 - 2)) {}
+  if (memset(b1, 0, 4 * 2)) {}
+
+  if (memset(b1, 0, (size_t)(sizeof(b1) != 0))) {}
+}