Index: clang-tidy/misc/SuspiciousStringCompareCheck.cpp =================================================================== --- clang-tidy/misc/SuspiciousStringCompareCheck.cpp +++ clang-tidy/misc/SuspiciousStringCompareCheck.cpp @@ -8,10 +8,10 @@ //===----------------------------------------------------------------------===// #include "SuspiciousStringCompareCheck.h" +#include "../utils/Matchers.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; @@ -106,20 +106,43 @@ ParseFunctionNames(KnownStringCompareFunctions, &FunctionNames); ParseFunctionNames(StringCompareLikeFunctions, &FunctionNames); - // Match a call to a string compare functions. + // Match a std::string::compare call. + const auto StdStringCompareCallExpr = + cxxMemberCallExpr(callee(cxxMethodDecl(ofClass(hasName("basic_string")), + hasName("compare")) + .bind("decl"))) + .bind("call"); + + // Match llvm strings variants. + const auto LLVMStringCompareCallExpr = + cxxMemberCallExpr( + callee(cxxMethodDecl( + ofClass(hasName("::llvm::StringRef")), + hasAnyName("compare", "compare_lower", "compare_numeric")) + .bind("decl"))) + .bind("call"); + + // Match a call to a string compare functions (i.e. strcmp). const auto FunctionCompareDecl = functionDecl(hasAnyName(std::vector(FunctionNames.begin(), FunctionNames.end()))) .bind("decl"); const auto DirectStringCompareCallExpr = callExpr(hasDeclaration(FunctionCompareDecl)).bind("call"); - const auto MacroStringCompareCallExpr = - conditionalOperator( - anyOf(hasTrueExpression(ignoringParenImpCasts(DirectStringCompareCallExpr)), - hasFalseExpression(ignoringParenImpCasts(DirectStringCompareCallExpr)))); + + // Match common macros used to wrapped runtime functions. + const auto CondMacroStringCompareCallExpr = conditionalOperator(anyOf( + hasTrueExpression(ignoringParenImpCasts(DirectStringCompareCallExpr)), + hasFalseExpression(ignoringParenImpCasts(DirectStringCompareCallExpr)))); + const auto CommaMacroStringCompareCallExpr = binaryOperator( + hasOperatorName(","), + hasRHS(ignoringParenImpCasts(DirectStringCompareCallExpr))); + // The implicit cast is not present in C. const auto StringCompareCallExpr = ignoringParenImpCasts( - anyOf(DirectStringCompareCallExpr, MacroStringCompareCallExpr)); + anyOf(DirectStringCompareCallExpr, CondMacroStringCompareCallExpr, + CommaMacroStringCompareCallExpr, StdStringCompareCallExpr, + LLVMStringCompareCallExpr)); if (WarnOnImplicitComparison) { // Detect suspicious calls to string compare: @@ -155,23 +178,34 @@ // Detect suspicious operator with string compare function as operand. Finder->addMatcher( - binaryOperator( - unless(anyOf(matchers::isComparisonOperator(), hasOperatorName("&&"), - hasOperatorName("||"), hasOperatorName("="))), - hasEitherOperand(StringCompareCallExpr)) + binaryOperator(unless(anyOf(matchers::isComparisonOperator(), + hasOperatorName("&&"), hasOperatorName("||"), + hasOperatorName("="), hasOperatorName(","))), + hasEitherOperand(StringCompareCallExpr)) .bind("suspicious-operator"), this); - // Detect comparison to invalid constant: 'strcmp() == -1'. + // Detect comparison to invalid constant: 'strcmp() == -2'. const auto InvalidLiteral = ignoringParenImpCasts( anyOf(integerLiteral(unless(equals(0))), unaryOperator(hasOperatorName("-"), has(integerLiteral(unless(equals(0))))), characterLiteral(), cxxBoolLiteral())); + // A subset of functions allow comparison to 1 and -1. + const auto ValidLiteral = ignoringParenImpCasts(anyOf( + integerLiteral(equals(1)), + unaryOperator(hasOperatorName("-"), has(integerLiteral(equals(1)))))); + + const auto ValidComparator = + binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")), + hasEitherOperand(LLVMStringCompareCallExpr), + hasEitherOperand(ValidLiteral)); + Finder->addMatcher(binaryOperator(matchers::isComparisonOperator(), hasEitherOperand(StringCompareCallExpr), - hasEitherOperand(InvalidLiteral)) + hasEitherOperand(InvalidLiteral), + unless(ValidComparator)) .bind("invalid-comparison"), this); } @@ -211,7 +245,8 @@ << Decl; } - if (const auto* BinOp = Result.Nodes.getNodeAs("suspicious-operator")) { + if (const auto *BinOp = + Result.Nodes.getNodeAs("suspicious-operator")) { diag(Call->getLocStart(), "results of function %0 used by operator '%1'") << Decl << BinOp->getOpcodeStr(); } 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 @@ -6,6 +6,32 @@ typedef __SIZE_TYPE__ size; +namespace std { +template +class allocator {}; +template +class char_traits {}; +template +struct basic_string { + typedef basic_string _Type; + basic_string(); + basic_string(const C *p, const A &a = A()); + int compare(const C* s) const; +}; + +typedef basic_string, std::allocator > string; +} + +namespace llvm { +struct StringRef { + StringRef(); + StringRef(const char*); + int compare(StringRef RHS); + int compare_lower(StringRef RHS); + int compare_numeric(StringRef RHS); +}; +} + struct locale_t { void* dummy; } locale; @@ -335,3 +361,82 @@ return 1; } + +#define uprv_checkValidMemory(a, b) ((void)(a)) +#define uprv_strncmp(s1, s2, n) ( \ + uprv_checkValidMemory(s1, 1), \ + uprv_checkValidMemory(s2, 1), \ + strncmp(s1, s2, n)) + +int strncmp_macro_uprv(const char* a, const char* b) { + if (uprv_strncmp(a, b, 4)) + return 0; + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strncmp' is called without explicitly comparing result + + if (uprv_strncmp(a, b, 4) == 2) + return 0; + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strncmp' is compared to a suspicious constant + + if (uprv_strncmp(a, b, 4) <= .0) + return 0; + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'strncmp' has suspicious implicit cast + + if (uprv_strncmp(a, b, 4) + 0) + return 0; + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: results of function 'strncmp' used by operator '+' + + return 1; +} + +int test_string_patterns() { + std::string str; + if (str.compare("a")) + return 0; + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is called without explicitly comparing result + // CHECK-FIXES: if (str.compare("a") != 0) + + if (str.compare("a") == 0 || + str.compare("b")) + return 0; + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is called without explicitly comparing result + // CHECK-FIXES: str.compare("b") != 0) + + if (str.compare("a") == 1) + return 0; + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is compared to a suspicious constant + + if (str.compare("a") == 2) + return 0; + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is compared to a suspicious constant +} + +int test_llvm_patterns() { + llvm::StringRef str; + if (str.compare("a")) + return 0; + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is called without explicitly comparing result + // CHECK-FIXES: if (str.compare("a") != 0) + + if (str.compare_lower("a")) + return 0; + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare_lower' is called without explicitly comparing result + // CHECK-FIXES: if (str.compare_lower("a") != 0) + + if (str.compare("a") == 0 || + str.compare("b")) + return 0; + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is called without explicitly comparing result + // CHECK-FIXES: str.compare("b") != 0) + + if (str.compare("a") == 2) + return 0; + // CHECK-MESSAGES: [[@LINE-2]]:7: warning: function 'compare' is compared to a suspicious constant + + // The following calls are valid. + if (str.compare("a") == 1) return 0; + if (str.compare("a") != 1) return 0; + if (str.compare("a") == 0) return 0; + if (str.compare("a") != 0) return 0; + if (str.compare("a") == -1) return 0; + if (str.compare("a") != -1) return 0; +}