[clang-tidy] Enhance misc-suspicious-string-compare to move down false-positives.

[etienneb]

Summary

The checker was noisy when running over llvm code base.
This patch is impriving the way string-compare functions are matched.

1. By default, do not report !strcmp(...) unless it's activate by the user,
2. Only match suspicious expression over a subset of expression (binary operator),
3. Added matching of macro wrapper used with clang on linux.

See bug: 27465.

[alexfh]

LG with one nit.

clang-tidy/misc/SuspiciousStringCompareCheck.cpp

215

Can you make the message more specific? It should make it clear, what exactly is "suspicious" in this usage.

[etienneb]

address alexfh comments.

clang-tidy/misc/SuspiciousStringCompareCheck.cpp

 //===--- SuspiciousStringCompareCheck.cpp - clang-tidy---------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "SuspiciousStringCompareCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
+#include "../utils/Matchers.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
-AST_MATCHER(BinaryOperator, isComparisonOperator) {
-  return Node.isComparisonOp();
-}
-
 static const char KnownStringCompareFunctions[] = "__builtin_memcmp;"
                                                   "__builtin_strcasecmp;"
                                                   "__builtin_strcmp;"
                                                   "__builtin_strncasecmp;"
                                                   "__builtin_strncmp;"
                                                   "_mbscmp;"
                                                   "_mbscmp_l;"
                                                   "_mbsicmp;"
if (!Function.empty())
       Result->push_back(Function);
   }
 }
 
 SuspiciousStringCompareCheck::SuspiciousStringCompareCheck(
     StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       WarnOnImplicitComparison(Options.get("WarnOnImplicitComparison", 1)),
-      WarnOnLogicalNotComparison(Options.get("WarnOnLogicalNotComparison", 1)),
+      WarnOnLogicalNotComparison(Options.get("WarnOnLogicalNotComparison", 0)),
       StringCompareLikeFunctions(
           Options.get("StringCompareLikeFunctions", "")) {}
 
 void SuspiciousStringCompareCheck::storeOptions(
     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnImplicitComparison", WarnOnImplicitComparison);
   Options.store(Opts, "WarnOnLogicalNotComparison", WarnOnLogicalNotComparison);
   Options.store(Opts, "StringCompareLikeFunctions", StringCompareLikeFunctions);
 }
 
 void SuspiciousStringCompareCheck::registerMatchers(MatchFinder *Finder) {
   // Match relational operators.
   const auto ComparisonUnaryOperator = unaryOperator(hasOperatorName("!"));
-  const auto ComparisonBinaryOperator = binaryOperator(isComparisonOperator());
+  const auto ComparisonBinaryOperator =
+      binaryOperator(matchers::isComparisonOperator());
   const auto ComparisonOperator =
       expr(anyOf(ComparisonUnaryOperator, ComparisonBinaryOperator));
 
   // Add the list of known string compare-like functions and add user-defined
   // functions.
   std::vector<std::string> FunctionNames;
   ParseFunctionNames(KnownStringCompareFunctions, &FunctionNames);
   ParseFunctionNames(StringCompareLikeFunctions, &FunctionNames);
+
+  // Match a call to a string compare functions.
   const auto FunctionCompareDecl =
       functionDecl(hasAnyName(std::vector<StringRef>(FunctionNames.begin(),
                                                      FunctionNames.end())))
           .bind("decl");
-
-  // Match a call to a string compare functions.
-  const auto StringCompareCallExpr =
+  const auto DirectStringCompareCallExpr =
       callExpr(hasDeclaration(FunctionCompareDecl)).bind("call");
+  const auto MacroStringCompareCallExpr =
+      conditionalOperator(
+        anyOf(hasTrueExpression(ignoringParenImpCasts(DirectStringCompareCallExpr)),
+              hasFalseExpression(ignoringParenImpCasts(DirectStringCompareCallExpr))));
+  // The implicit cast is not present in C.
+  const auto StringCompareCallExpr = ignoringParenImpCasts(
+        anyOf(DirectStringCompareCallExpr, MacroStringCompareCallExpr));
 
   if (WarnOnImplicitComparison) {
-    // Detect suspicious calls to string compare (missing comparator) [only C]:
+    // Detect suspicious calls to string compare:
     //     'if (strcmp())'  ->  'if (strcmp() != 0)'
     Finder->addMatcher(
         stmt(anyOf(ifStmt(hasCondition(StringCompareCallExpr)),
                    whileStmt(hasCondition(StringCompareCallExpr)),
                    doStmt(hasCondition(StringCompareCallExpr)),
-                   forStmt(hasCondition(StringCompareCallExpr))))
-            .bind("missing-comparison"),
-        this);
-
-    Finder->addMatcher(expr(StringCompareCallExpr,
-                            unless(hasParent(ComparisonOperator)),
-                            unless(hasParent(implicitCastExpr())))
-                           .bind("missing-comparison"),
-                       this);
-
-    // Detect suspicious calls to string compare with implicit comparison:
-    //     'if (strcmp())'  ->  'if (strcmp() != 0)'
-    //     'if (!strcmp())' is considered valid (see WarnOnLogicalNotComparison)
-    Finder->addMatcher(
-        implicitCastExpr(hasType(isInteger()),
-                         hasSourceExpression(StringCompareCallExpr),
-                         unless(hasParent(ComparisonOperator)))
+                   forStmt(hasCondition(StringCompareCallExpr)),
+                   binaryOperator(
+                       anyOf(hasOperatorName("&&"), hasOperatorName("||")),
+                       hasEitherOperand(StringCompareCallExpr))))
             .bind("missing-comparison"),
         this);
-
-    // Detect suspicious cast to an inconsistant type.
-    Finder->addMatcher(
-        implicitCastExpr(unless(hasType(isInteger())),
-                         hasSourceExpression(StringCompareCallExpr))
-            .bind("invalid-conversion"),
-        this);
   }
 
   if (WarnOnLogicalNotComparison) {
     // Detect suspicious calls to string compared with '!' operator:
     //     'if (!strcmp())'  ->  'if (strcmp() == 0)'
     Finder->addMatcher(unaryOperator(hasOperatorName("!"),
                                      hasUnaryOperand(ignoringParenImpCasts(
                                          StringCompareCallExpr)))
                            .bind("logical-not-comparison"),
                        this);
   }
 
-  // Detect suspicious calls to string compare functions: 'strcmp() == -1'.
+  // Detect suspicious cast to an inconsistant type (i.e. not integer type).
+  Finder->addMatcher(
+      implicitCastExpr(unless(hasType(isInteger())),
+                       hasSourceExpression(StringCompareCallExpr))
+          .bind("invalid-conversion"),
+      this);
+
+  // Detect suspicious operator with string compare function as operand.
+  Finder->addMatcher(
+      binaryOperator(
+          unless(anyOf(matchers::isComparisonOperator(), hasOperatorName("&&"),
+                       hasOperatorName("||"), hasOperatorName("="))),
+          hasEitherOperand(StringCompareCallExpr))
+          .bind("suspicious-operator"),
+      this);
+
+  // Detect comparison to invalid constant: 'strcmp() == -1'.
   const auto InvalidLiteral = ignoringParenImpCasts(
       anyOf(integerLiteral(unless(equals(0))),
             unaryOperator(hasOperatorName("-"),
                           has(integerLiteral(unless(equals(0))))),
             characterLiteral(), cxxBoolLiteral()));
 
-  Finder->addMatcher(binaryOperator(isComparisonOperator(),
+  Finder->addMatcher(binaryOperator(matchers::isComparisonOperator(),
                                     hasEitherOperand(StringCompareCallExpr),
                                     hasEitherOperand(InvalidLiteral))
                          .bind("invalid-comparison"),
                      this);
 }
 
 void SuspiciousStringCompareCheck::check(
     const MatchFinder::MatchResult &Result) {
void SuspiciousStringCompareCheck::check(
   }
 
   if (Result.Nodes.getNodeAs<Stmt>("invalid-comparison")) {
     diag(Call->getLocStart(),
          "function %0 is compared to a suspicious constant")
         << Decl;
   }
 
+  if (const auto* BinOp = Result.Nodes.getNodeAs<BinaryOperator>("suspicious-operator")) {
+    diag(Call->getLocStart(), "results of function %0 used by operator '%1'")
+        << Decl << BinOp->getOpcodeStr();
+  }
+
   if (Result.Nodes.getNodeAs<Stmt>("invalid-conversion")) {
     diag(Call->getLocStart(), "function %0 has suspicious implicit cast")
         << Decl;
   }
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

test/clang-tidy/misc-suspicious-string-compare.c

 int test_valid_patterns() {
   // The following cases are valid.
   if (strcmp(A, "a") < 0) return 0;
   if (strcmp(A, "a") == 0) return 0;
   if (strcmp(A, "a") <= 0) return 0;
   if (strcmp(A, "a") == strcmp(A, "b")) return 0;
   return 1;
 }
+
+int wrapper(const char* a, const char* b) {
+  return strcmp(a, b);
+}
+
+int assignment_wrapper(const char* a, const char* b) {
+  int cmp = strcmp(a, b);
+  return cmp;
+}
+
+int condexpr_wrapper(const char* a, const char* b) {
+  return (a < b) ? strcmp(a, b) : strcmp(b, a);
+}

test/clang-tidy/misc-suspicious-string-compare.cpp

struct locale_t {
   void* dummy;
 } locale;
 
 static const char A[] = "abc";
 static const unsigned char U[] = "abc";
 static const unsigned char V[] = "xyz";
 static const wchar_t W[] = L"abc";
 
+int strlen(const char *);
+
 int memcmp(const void *, const void *, size);
 int wmemcmp(const wchar_t *, const wchar_t *, size);
 int memicmp(const void *, const void *, size);
 int _memicmp(const void *, const void *, size);
 int _memicmp_l(const void *, const void *, size, locale_t);
 
 int strcmp(const char *, const char *);
 int strncmp(const char *, const char *, size);
int test_implicit_compare_with_functions() {
 
   if (_mbsnbicmp_l(U, V, 1, locale))
     return 0;
   // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function '_mbsnbicmp_l' is called without explicitly comparing result
   // CHECK-FIXES: _mbsnbicmp_l(U, V, 1, locale) != 0)
 
   return 1;
 }
+
+int strcmp_wrapper1(const char* a, const char* b) {
+  return strcmp(a, b);
+}
+
+int strcmp_wrapper2(const char* a, const char* b) {
+  return (a && b) ? strcmp(a, b) : 0;
+}
+
+#define macro_strncmp(s1, s2, n)                                              \
+  (__extension__ (__builtin_constant_p (n)                                    \
+                  && ((__builtin_constant_p (s1)                              \
+                       && strlen (s1) < ((size) (n)))                         \
+                      || (__builtin_constant_p (s2)                           \
+                          && strlen (s2) < ((size) (n))))                     \
+                  ? strcmp (s1, s2) : strncmp (s1, s2, n)))
+
+int strncmp_macro(const char* a, const char* b) {
+  if (macro_strncmp(a, b, 4))
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is called without explicitly comparing result
+
+  if (macro_strncmp(a, b, 4) == 2)
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' is compared to a suspicious constant
+
+  if (macro_strncmp(a, b, 4) <= .0)
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strcmp' has suspicious implicit cast
+
+  if (macro_strncmp(a, b, 4) + 0)
+    return 0;
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: results of function 'strcmp' used by operator '+'
+
+  return 1;
+}