Index: clang-tidy/misc/SuspiciousStringCompareCheck.cpp =================================================================== --- clang-tidy/misc/SuspiciousStringCompareCheck.cpp +++ clang-tidy/misc/SuspiciousStringCompareCheck.cpp @@ -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 tidy { namespace misc { -AST_MATCHER(BinaryOperator, isComparisonOperator) { - return Node.isComparisonOp(); -} - static const char KnownStringCompareFunctions[] = "__builtin_memcmp;" "__builtin_strcasecmp;" "__builtin_strcmp;" @@ -84,7 +81,7 @@ 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::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 @@ std::vector FunctionNames; ParseFunctionNames(KnownStringCompareFunctions, &FunctionNames); ParseFunctionNames(StringCompareLikeFunctions, &FunctionNames); + + // Match a call to a string compare functions. const auto FunctionCompareDecl = functionDecl(hasAnyName(std::vector(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 @@ 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,10 @@ << Decl; } + if (Result.Nodes.getNodeAs("suspicious-operator")) { + diag(Call->getLocStart(), "suspicious usage of function %0") << Decl; + } + if (Result.Nodes.getNodeAs("invalid-conversion")) { diag(Call->getLocStart(), "function %0 has suspicious implicit cast") << Decl; Index: test/clang-tidy/misc-suspicious-string-compare.c =================================================================== --- test/clang-tidy/misc-suspicious-string-compare.c +++ test/clang-tidy/misc-suspicious-string-compare.c @@ -64,3 +64,16 @@ 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); +} Index: test/clang-tidy/misc-suspicious-string-compare.cpp =================================================================== --- test/clang-tidy/misc-suspicious-string-compare.cpp +++ test/clang-tidy/misc-suspicious-string-compare.cpp @@ -15,6 +15,8 @@ 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 @@ 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: suspicious usage of function 'strcmp' + + return 1; +}