Author: etienneb Date: Tue Apr 26 11:53:21 2016 New Revision: 267570 URL: http://llvm.org/viewvc/llvm-project?rev=267570&view=rev Log: [clang-tidy] Enhance misc-suspicious-string-compare to move down false-positives.
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. Reviewers: alexfh Subscribers: cfe-commits Differential Revision: http://reviews.llvm.org/D19497 Modified: clang-tools-extra/trunk/clang-tidy/misc/SuspiciousStringCompareCheck.cpp clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-string-compare.c clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-string-compare.cpp Modified: clang-tools-extra/trunk/clang-tidy/misc/SuspiciousStringCompareCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/misc/SuspiciousStringCompareCheck.cpp?rev=267570&r1=267569&r2=267570&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/misc/SuspiciousStringCompareCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/misc/SuspiciousStringCompareCheck.cpp Tue Apr 26 11:53:21 2016 @@ -11,6 +11,7 @@ #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Lex/Lexer.h" +#include "../utils/Matchers.h" using namespace clang::ast_matchers; @@ -18,10 +19,6 @@ namespace clang { namespace tidy { namespace misc { -AST_MATCHER(BinaryOperator, isComparisonOperator) { - return Node.isComparisonOp(); -} - static const char KnownStringCompareFunctions[] = "__builtin_memcmp;" "__builtin_strcasecmp;" "__builtin_strcmp;" @@ -84,7 +81,7 @@ SuspiciousStringCompareCheck::Suspicious 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", "")) {} @@ -98,7 +95,8 @@ void SuspiciousStringCompareCheck::store 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)); @@ -107,48 +105,35 @@ void SuspiciousStringCompareCheck::regis 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)))) + forStmt(hasCondition(StringCompareCallExpr)), + binaryOperator( + anyOf(hasOperatorName("&&"), hasOperatorName("||")), + hasEitherOperand(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))) - .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) { @@ -161,14 +146,30 @@ void SuspiciousStringCompareCheck::regis 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"), @@ -210,6 +211,11 @@ void SuspiciousStringCompareCheck::check << 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; Modified: clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-string-compare.c URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-string-compare.c?rev=267570&r1=267569&r2=267570&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-string-compare.c (original) +++ clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-string-compare.c Tue Apr 26 11:53:21 2016 @@ -64,3 +64,16 @@ int test_valid_patterns() { 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); +} Modified: clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-string-compare.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-string-compare.cpp?rev=267570&r1=267569&r2=267570&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-string-compare.cpp (original) +++ clang-tools-extra/trunk/test/clang-tidy/misc-suspicious-string-compare.cpp Tue Apr 26 11:53:21 2016 @@ -15,6 +15,8 @@ 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); @@ -297,3 +299,39 @@ int test_implicit_compare_with_functions 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; +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
