From e1d0ea24edcde91d54d74767545b0feeef20b06c Mon Sep 17 00:00:00 2001 From: Jason Orendorff Date: Wed, 15 Jul 2020 18:07:13 +0000 Subject: [PATCH] Bug 1485588 - Part 1: Clang-plugin checker for locale-specific functions. r=hsivonen. They are warnings, not errors, because of the many existing uses of these functions throughout the codebase. Differential Revision: https://phabricator.services.mozilla.com/D81785 --- build/clang-plugin/Checks.inc | 1 + build/clang-plugin/ChecksIncludes.inc | 1 + .../clang-plugin/NoLocaleSpecificChecker.cpp | 107 ++++++++++++++++++ build/clang-plugin/NoLocaleSpecificChecker.h | 18 +++ build/clang-plugin/moz.build | 1 + .../tests/TestNoLocaleSpecificChecker.cpp | 29 +++++ .../tests/TestNoLocaleSpecificChecker2.cpp | 54 +++++++++ build/clang-plugin/tests/moz.build | 2 + 8 files changed, 213 insertions(+) create mode 100644 build/clang-plugin/NoLocaleSpecificChecker.cpp create mode 100644 build/clang-plugin/NoLocaleSpecificChecker.h create mode 100644 build/clang-plugin/tests/TestNoLocaleSpecificChecker.cpp create mode 100644 build/clang-plugin/tests/TestNoLocaleSpecificChecker2.cpp diff --git a/build/clang-plugin/Checks.inc b/build/clang-plugin/Checks.inc index 1630aa192c15..d8fe9eb45cae 100644 --- a/build/clang-plugin/Checks.inc +++ b/build/clang-plugin/Checks.inc @@ -25,6 +25,7 @@ CHECK(NoAddRefReleaseOnReturnChecker, "no-addref-release-on-return") CHECK(NoAutoTypeChecker, "no-auto-type") CHECK(NoDuplicateRefCntMemberChecker, "no-duplicate-refcnt-member") CHECK(NoExplicitMoveConstructorChecker, "no-explicit-move-constructor") +CHECK(NoLocaleSpecificChecker, "no-locale-specific-checker") CHECK(NoNewThreadsChecker, "no-new-threads") CHECK(NonMemMovableMemberChecker, "non-memmovable-member") CHECK(NonMemMovableTemplateArgChecker, "non-memmovable-template-arg") diff --git a/build/clang-plugin/ChecksIncludes.inc b/build/clang-plugin/ChecksIncludes.inc index 3f4ec4fe08e7..e49550098d18 100644 --- a/build/clang-plugin/ChecksIncludes.inc +++ b/build/clang-plugin/ChecksIncludes.inc @@ -26,6 +26,7 @@ #include "NoAutoTypeChecker.h" #include "NoDuplicateRefCntMemberChecker.h" #include "NoExplicitMoveConstructorChecker.h" +#include "NoLocaleSpecificChecker.h" #include "NoNewThreadsChecker.h" #include "NonMemMovableMemberChecker.h" #include "NonMemMovableTemplateArgChecker.h" diff --git a/build/clang-plugin/NoLocaleSpecificChecker.cpp b/build/clang-plugin/NoLocaleSpecificChecker.cpp new file mode 100644 index 000000000000..06d534f9ca55 --- /dev/null +++ b/build/clang-plugin/NoLocaleSpecificChecker.cpp @@ -0,0 +1,107 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "NoLocaleSpecificChecker.h" +#include +#include "CustomMatchers.h" + +void NoLocaleSpecificChecker::registerMatchers(MatchFinder *AstMatcher) { + auto hasCharPtrParam = [](const unsigned int Position) { + return hasParameter( + Position, hasType(hasCanonicalType(pointsTo(asString("char"))))); + }; + auto hasConstCharPtrParam = [](const unsigned int Position) { + return hasParameter( + Position, hasType(hasCanonicalType(pointsTo(asString("const char"))))); + }; + auto hasIntegerParam = [](const unsigned int Position) { + return hasParameter(Position, hasType(isInteger())); + }; + auto hasNameAndIntegerParam = [&](const char* name) { + return allOf(hasName(name), hasIntegerParam(0)); + }; + + AstMatcher->addMatcher( + declRefExpr( + allOf( + isFirstParty(), + to(functionDecl(allOf( + isInSystemHeader(), + anyOf( + hasNameAndIntegerParam("isalnum"), + hasNameAndIntegerParam("isalpha"), + hasNameAndIntegerParam("isblank"), + hasNameAndIntegerParam("iscntrl"), + hasNameAndIntegerParam("isdigit"), + hasNameAndIntegerParam("isgraph"), + hasNameAndIntegerParam("islower"), + hasNameAndIntegerParam("isprint"), + hasNameAndIntegerParam("ispunct"), + hasNameAndIntegerParam("isspace"), + hasNameAndIntegerParam("isupper"), + hasNameAndIntegerParam("isxdigit"), + allOf(hasName("strcasecmp"), + hasConstCharPtrParam(0), + hasConstCharPtrParam(1)), + allOf(hasName("strncasecmp"), + hasConstCharPtrParam(0), + hasConstCharPtrParam(1), + hasIntegerParam(2)), + allOf(hasName("strcoll"), + hasConstCharPtrParam(0), + hasConstCharPtrParam(1)), + hasNameAndIntegerParam("strerror"), + allOf(hasName("strxfrm"), + hasCharPtrParam(0), + hasConstCharPtrParam(1), + hasIntegerParam(2)), + hasNameAndIntegerParam("tolower"), + hasNameAndIntegerParam("toupper") + ) + ))) + ) + ).bind("id"), + this); +} + +struct LocaleSpecificCheckerHint { + const char* name; + const char* message; +}; + +static const LocaleSpecificCheckerHint Hints[] = { + { "isalnum", "Consider mozilla::IsAsciiAlphanumeric instead." }, + { "isalpha", "Consider mozilla::IsAsciiAlpha instead." }, + { "isdigit", "Consider mozilla::IsAsciiDigit instead." }, + { "islower", "Consider mozilla::IsAsciiLowercaseAlpha instead." }, + { "isspace", "Consider mozilla::IsAsciiWhitespace instead." }, + { "isupper", "Consider mozilla::IsAsciiUppercaseAlpha instead." }, + { "isxdigit", "Consider mozilla::IsAsciiHexDigit instead." }, + { "strcasecmp", + "Consider mozilla::CompareCaseInsensitiveASCII " + "or mozilla::EqualsCaseInsensitiveASCII instead." }, + { "strncasecmp", + "Consider mozilla::CompareCaseInsensitiveASCII " + "or mozilla::EqualsCaseInsensitiveASCII instead." }, +}; + +void NoLocaleSpecificChecker::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = Result.Nodes.getNodeAs("id"); + diag(MatchedDecl->getExprLoc(), + "Do not use locale-specific standard library functions.", + DiagnosticIDs::Warning); + + // Also emit a relevant hint, if we have one. + std::string Name = MatchedDecl->getNameInfo().getName().getAsString(); + const LocaleSpecificCheckerHint* HintsEnd = Hints + sizeof(Hints) / sizeof(Hints[0]); + auto Hit = std::find_if( + Hints, + HintsEnd, + [&](const LocaleSpecificCheckerHint& hint) { + return strcmp(hint.name, Name.c_str()) == 0; + }); + if (Hit != HintsEnd) { + diag(MatchedDecl->getExprLoc(), Hit->message, DiagnosticIDs::Note); + } +} diff --git a/build/clang-plugin/NoLocaleSpecificChecker.h b/build/clang-plugin/NoLocaleSpecificChecker.h new file mode 100644 index 000000000000..fe0473b27380 --- /dev/null +++ b/build/clang-plugin/NoLocaleSpecificChecker.h @@ -0,0 +1,18 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef NoLocaleSpecificChecker_h__ +#define NoLocaleSpecificChecker_h__ + +#include "plugin.h" + +class NoLocaleSpecificChecker : public BaseCheck { +public: + NoLocaleSpecificChecker(StringRef CheckName, ContextType *Context = nullptr) + : BaseCheck(CheckName, Context) {} + void registerMatchers(MatchFinder *AstMatcher) override; + void check(const MatchFinder::MatchResult &Result) override; +}; + +#endif diff --git a/build/clang-plugin/moz.build b/build/clang-plugin/moz.build index 00e20cc162bc..a86f1e4e8973 100644 --- a/build/clang-plugin/moz.build +++ b/build/clang-plugin/moz.build @@ -29,6 +29,7 @@ HOST_SOURCES += [ 'NoAutoTypeChecker.cpp', 'NoDuplicateRefCntMemberChecker.cpp', 'NoExplicitMoveConstructorChecker.cpp', + 'NoLocaleSpecificChecker.cpp', 'NoNewThreadsChecker.cpp', 'NonMemMovableMemberChecker.cpp', 'NonMemMovableTemplateArgChecker.cpp', diff --git a/build/clang-plugin/tests/TestNoLocaleSpecificChecker.cpp b/build/clang-plugin/tests/TestNoLocaleSpecificChecker.cpp new file mode 100644 index 000000000000..a1341900ab99 --- /dev/null +++ b/build/clang-plugin/tests/TestNoLocaleSpecificChecker.cpp @@ -0,0 +1,29 @@ +// Locale-specific standard library functions are banned. + +#include +#include +#include +#include +#include +#include + +int useLocaleSpecificFunctions() { + if (isalpha('r')) { // expected-warning{{locale-specific}} expected-note {{IsAsciiAlpha instead}} + return 1; + } + if (isalnum('8')) { // expected-warning{{locale-specific}} expected-note {{IsAsciiAlphanumeric instead}} + return 1; + } + std::string s("8167"); + if (std::find_if(s.begin(), s.end(), isdigit) != s.end()) { // expected-warning{{locale-specific}} expected-note {{IsAsciiDigit instead}} + return 1; + } + + if (strcasecmp(s.c_str(), "81d7") == 0) { // expected-warning{{locale-specific}} expected-note {{CompareCaseInsensitiveASCII}} + return 1; + } + + printf("problems: %s\n", strerror(errno)); // expected-warning{{locale-specific}} + + return 0; +} diff --git a/build/clang-plugin/tests/TestNoLocaleSpecificChecker2.cpp b/build/clang-plugin/tests/TestNoLocaleSpecificChecker2.cpp new file mode 100644 index 000000000000..f143f3cd66a3 --- /dev/null +++ b/build/clang-plugin/tests/TestNoLocaleSpecificChecker2.cpp @@ -0,0 +1,54 @@ +// Like TestNoLocaleSpecificChecker.cpp, but using the standard C++ and headers. + +#include +#include +#include +#include +#include +#include + +int useLocaleSpecificFunctions() { + if (isalpha('r')) { // expected-warning{{locale-specific}} expected-note {{IsAsciiAlpha instead}} + return 1; + } + using std::isalnum; + if (isalnum('8')) { // expected-warning{{locale-specific}} expected-note {{IsAsciiAlphanumeric instead}} + return 1; + } + std::string s("8167"); + if (std::find_if(s.begin(), s.end(), isdigit) != s.end()) { // expected-warning{{locale-specific}} expected-note {{IsAsciiDigit instead}} + return 1; + } + typedef int (*PredicateType)(int); + PredicateType predicate = std::isdigit; // expected-warning{{locale-specific}} expected-note {{IsAsciiDigit instead}} + + if (strcasecmp(s.c_str(), "81d7") == 0) { // expected-warning{{locale-specific}} expected-note {{CompareCaseInsensitiveASCII}} + return 1; + } + + printf("problems: %s\n", strerror(errno)); // expected-warning{{locale-specific}} + + return 0; +} + +using std::isalnum; + +int useStdLocaleFunctions() { + std::locale l; + + if (std::isalpha('r', l)) { // expected-warning{{locale-specific}} expected-note {{IsAsciiAlpha instead}} + return 1; + } + + if (isalnum('8', l)) { // expected-warning{{locale-specific}} expected-note {{IsAsciiAlphanumeric instead}} + return 1; + } + + using std::isdigit; + std::string s("8167"); + if (std::find_if(s.begin(), s.end(), [&](char c) { return isdigit(c, l); }) != s.end()) { // expected-warning{{locale-specific}} expected-note {{IsAsciiDigit instead}} + return 1; + } + + return 0; +} diff --git a/build/clang-plugin/tests/moz.build b/build/clang-plugin/tests/moz.build index bb0d533adfd2..a65bebf3a2d5 100644 --- a/build/clang-plugin/tests/moz.build +++ b/build/clang-plugin/tests/moz.build @@ -30,6 +30,8 @@ SOURCES += [ 'TestNoAutoType.cpp', 'TestNoDuplicateRefCntMember.cpp', 'TestNoExplicitMoveConstructor.cpp', + 'TestNoLocaleSpecificChecker.cpp', + 'TestNoLocaleSpecificChecker2.cpp', 'TestNoNewThreadsChecker.cpp', 'TestNonHeapClass.cpp', 'TestNonMemMovable.cpp',