Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[clang-tidy] Add new checker for comparison with runtime string funct…
…ions. Summary: This checker is validating suspicious usage of string compare functions. Example: ``` if (strcmp(...)) // Implicitly compare to zero if (!strcmp(...)) // Won't warn if (strcmp(...) != 0) // Won't warn ``` This patch was checked over large amount of code. There is three checks: [*] Implicit comparator to zero (coding-style, many warnings found), [*] Suspicious implicit cast to non-integral (bugs!?, almost none found), [*] Comparison to suspicious constant (bugs!?, found two cases), Example: [[https://github.com/kylepjohnson/sigma/blob/master/sigma/native-installers/debian/dependencies/files/opt/sigma/E/HEURISTICS/che_to_precgen.c | https://github.com/kylepjohnson/sigma/blob/master/sigma/native-installers/debian/dependencies/files/opt/sigma/E/HEURISTICS/che_to_precgen.c]] ``` else if(strcmp(id, "select") == 0) { array->array[i].key1 = 25; } else if(strcmp(id, "sk") == 28) // BUG!? { array->array[i].key1 = 20; } ``` Reviewers: alexfh Subscribers: Eugene.Zelenko, cfe-commits Differential Revision: http://reviews.llvm.org/D18703 llvm-svn: 267009
- Loading branch information
Showing
8 changed files
with
671 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
221 changes: 221 additions & 0 deletions
221
clang-tools-extra/clang-tidy/misc/SuspiciousStringCompareCheck.cpp
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,221 @@ | ||
//===--- 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" | ||
|
||
using namespace clang::ast_matchers; | ||
|
||
namespace clang { | ||
namespace tidy { | ||
namespace misc { | ||
|
||
AST_MATCHER(BinaryOperator, isComparisonOperator) { | ||
return Node.isComparisonOp(); | ||
} | ||
|
||
constexpr char KnownStringCompareFunctions[] = "__builtin_memcmp;" | ||
"__builtin_strcasecmp;" | ||
"__builtin_strcmp;" | ||
"__builtin_strncasecmp;" | ||
"__builtin_strncmp;" | ||
"_mbscmp;" | ||
"_mbscmp_l;" | ||
"_mbsicmp;" | ||
"_mbsicmp_l;" | ||
"_mbsnbcmp;" | ||
"_mbsnbcmp_l;" | ||
"_mbsnbicmp;" | ||
"_mbsnbicmp_l;" | ||
"_mbsncmp;" | ||
"_mbsncmp_l;" | ||
"_mbsnicmp;" | ||
"_mbsnicmp_l;" | ||
"_memicmp;" | ||
"_memicmp_l;" | ||
"_stricmp;" | ||
"_stricmp_l;" | ||
"_strnicmp;" | ||
"_strnicmp_l;" | ||
"_wcsicmp;" | ||
"_wcsicmp_l;" | ||
"_wcsnicmp;" | ||
"_wcsnicmp_l;" | ||
"lstrcmp;" | ||
"lstrcmpi;" | ||
"memcmp;" | ||
"memicmp;" | ||
"strcasecmp;" | ||
"strcmp;" | ||
"strcmpi;" | ||
"stricmp;" | ||
"strncasecmp;" | ||
"strncmp;" | ||
"strnicmp;" | ||
"wcscasecmp;" | ||
"wcscmp;" | ||
"wcsicmp;" | ||
"wcsncmp;" | ||
"wcsnicmp;" | ||
"wmemcmp;"; | ||
|
||
static const char StringCompareLikeFunctionsDelimiter[] = ";"; | ||
|
||
static void ParseFunctionNames(StringRef Option, | ||
std::vector<std::string> *Result) { | ||
SmallVector<StringRef, 4> Functions; | ||
Option.split(Functions, StringCompareLikeFunctionsDelimiter); | ||
for (StringRef &Function : Functions) { | ||
Function = Function.trim(); | ||
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)), | ||
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 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); | ||
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 = | ||
callExpr(hasDeclaration(FunctionCompareDecl)).bind("call"); | ||
|
||
if (WarnOnImplicitComparison) { | ||
// Detect suspicious calls to string compare (missing comparator) [only C]: | ||
// '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))) | ||
.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'. | ||
const auto InvalidLiteral = ignoringParenImpCasts( | ||
anyOf(integerLiteral(unless(equals(0))), | ||
unaryOperator(hasOperatorName("-"), | ||
has(integerLiteral(unless(equals(0))))), | ||
characterLiteral(), cxxBoolLiteral())); | ||
|
||
Finder->addMatcher(binaryOperator(isComparisonOperator(), | ||
hasEitherOperand(StringCompareCallExpr), | ||
hasEitherOperand(InvalidLiteral)) | ||
.bind("invalid-comparison"), | ||
this); | ||
} | ||
|
||
void SuspiciousStringCompareCheck::check( | ||
const MatchFinder::MatchResult &Result) { | ||
const auto *Decl = Result.Nodes.getNodeAs<FunctionDecl>("decl"); | ||
const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call"); | ||
assert(Decl != nullptr && Call != nullptr); | ||
|
||
if (Result.Nodes.getNodeAs<Stmt>("missing-comparison")) { | ||
SourceLocation EndLoc = Lexer::getLocForEndOfToken( | ||
Call->getRParenLoc(), 0, Result.Context->getSourceManager(), | ||
Result.Context->getLangOpts()); | ||
|
||
diag(Call->getLocStart(), | ||
"function %0 is called without explicitly comparing result") | ||
<< Decl << FixItHint::CreateInsertion(EndLoc, " != 0"); | ||
} | ||
|
||
if (const auto *E = Result.Nodes.getNodeAs<Expr>("logical-not-comparison")) { | ||
SourceLocation EndLoc = Lexer::getLocForEndOfToken( | ||
Call->getRParenLoc(), 0, Result.Context->getSourceManager(), | ||
Result.Context->getLangOpts()); | ||
SourceLocation NotLoc = E->getLocStart(); | ||
|
||
diag(Call->getLocStart(), | ||
"function %0 is compared using logical not operator") | ||
<< Decl << FixItHint::CreateRemoval( | ||
CharSourceRange::getTokenRange(NotLoc, NotLoc)) | ||
<< FixItHint::CreateInsertion(EndLoc, " == 0"); | ||
} | ||
|
||
if (Result.Nodes.getNodeAs<Stmt>("invalid-comparison")) { | ||
diag(Call->getLocStart(), | ||
"function %0 is compared to a suspicious constant") | ||
<< Decl; | ||
} | ||
|
||
if (Result.Nodes.getNodeAs<Stmt>("invalid-conversion")) { | ||
diag(Call->getLocStart(), "function %0 has suspicious implicit cast") | ||
<< Decl; | ||
} | ||
} | ||
|
||
} // namespace misc | ||
} // namespace tidy | ||
} // namespace clang |
40 changes: 40 additions & 0 deletions
40
clang-tools-extra/clang-tidy/misc/SuspiciousStringCompareCheck.h
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
//===--- SuspiciousStringCompareCheck.h - clang-tidy-------------*- C++ -*-===// | ||
// | ||
// The LLVM Compiler Infrastructure | ||
// | ||
// This file is distributed under the University of Illinois Open Source | ||
// License. See LICENSE.TXT for details. | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SUSPICIOUS_STRING_COMPARE_H | ||
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SUSPICIOUS_STRING_COMPARE_H | ||
|
||
#include "../ClangTidy.h" | ||
|
||
namespace clang { | ||
namespace tidy { | ||
namespace misc { | ||
|
||
/// Find suspicious calls to string compare functions. | ||
/// | ||
/// For the user-facing documentation see: | ||
/// http://clang.llvm.org/extra/clang-tidy/checks/misc-suspicious-string-compare.html | ||
class SuspiciousStringCompareCheck : public ClangTidyCheck { | ||
public: | ||
SuspiciousStringCompareCheck(StringRef Name, ClangTidyContext *Context); | ||
void storeOptions(ClangTidyOptions::OptionMap &Opts) override; | ||
void registerMatchers(ast_matchers::MatchFinder *Finder) override; | ||
void check(const ast_matchers::MatchFinder::MatchResult &Result) override; | ||
|
||
private: | ||
const bool WarnOnImplicitComparison; | ||
const bool WarnOnLogicalNotComparison; | ||
const std::string StringCompareLikeFunctions; | ||
}; | ||
|
||
} // namespace misc | ||
} // namespace tidy | ||
} // namespace clang | ||
|
||
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_SUSPICIOUS_STRING_COMPARE_H |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
40 changes: 40 additions & 0 deletions
40
clang-tools-extra/docs/clang-tidy/checks/misc-suspicious-string-compare.rst
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
.. title:: clang-tidy - misc-suspicious-string-compare | ||
|
||
misc-suspicious-string-compare | ||
============================== | ||
|
||
Find suspicious usage of runtime string comparison functions. | ||
This check is valid in C and C++. | ||
|
||
Checks for calls with implicit comparator and proposed to explicitly add it. | ||
|
||
.. code:: c++ | ||
|
||
if (strcmp(...)) // Implicitly compare to zero | ||
if (!strcmp(...)) // Won't warn | ||
if (strcmp(...) != 0) // Won't warn | ||
|
||
|
||
Checks that compare function results (i,e, ``strcmp``) are compared to valid | ||
constant. The resulting value is | ||
|
||
.. code:: | ||
< 0 when lower than, | ||
> 0 when greater than, | ||
== 0 when equals. | ||
A common mistake is to compare the result to '1' or '-1'. | ||
|
||
.. code:: c++ | ||
|
||
if (strcmp(...) == -1) // Incorrect usage of the returned value. | ||
|
||
|
||
Additionally, the check warns if the results value is implicitly cast to a | ||
*suspicious* non-integer type. It's happening when the returned value is used in | ||
a wrong context. | ||
|
||
.. code:: c++ | ||
|
||
if (strcmp(...) < 0.) // Incorrect usage of the returned value. |
Oops, something went wrong.