[clang-tools-extra] r267570 - [clang-tidy] Enhance misc-suspicious-string-compare to move down false-positives.
Etienne Bergeron via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 26 09:53:21 PDT 2016
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;
+}
More information about the cfe-commits
mailing list