llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Discookie (Discookie) <details> <summary>Changes</summary> Adds the check options `bugprone-unsafe-functions.CustomNormalFunctions` and `CustomAnnexKFunctions` to be able to match user-defined functions as part of the checker. Adds the option `bugprone-unsafe-functions.ReportDefaultFunctions` to disable reporting the default set of functions as well. The functions names are matched using the same mechanism as the `matchesAnyListedName` tidy matcher, documented in the [check docs](https://github.com/Discookie/llvm-project/blob/unsafe-expand/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst). --- Patch is 26.16 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/106350.diff 7 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp (+184-32) - (modified) clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h (+13) - (modified) clang-tools-extra/clang-tidy/utils/Matchers.h (+10-8) - (modified) clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst (+53) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions-custom-regex.cpp (+63) - (added) clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions-custom.c (+35) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c (+6) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp index ea7eaa0b0ff811..8d9ffda04a847a 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp @@ -7,6 +7,8 @@ //===----------------------------------------------------------------------===// #include "UnsafeFunctionsCheck.h" +#include "../utils/Matchers.h" +#include "../utils/OptionsUtils.h" #include "clang/AST/ASTContext.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Lex/PPCallbacks.h" @@ -18,6 +20,12 @@ using namespace llvm; namespace clang::tidy::bugprone { +static constexpr llvm::StringLiteral OptionNameCustomNormalFunctions = + "CustomNormalFunctions"; +static constexpr llvm::StringLiteral OptionNameCustomAnnexKFunctions = + "CustomAnnexKFunctions"; +static constexpr llvm::StringLiteral OptionNameReportDefaultFunctions = + "ReportDefaultFunctions"; static constexpr llvm::StringLiteral OptionNameReportMoreUnsafeFunctions = "ReportMoreUnsafeFunctions"; @@ -26,6 +34,8 @@ static constexpr llvm::StringLiteral FunctionNamesWithAnnexKReplacementId = static constexpr llvm::StringLiteral FunctionNamesId = "FunctionsNames"; static constexpr llvm::StringLiteral AdditionalFunctionNamesId = "AdditionalFunctionsNames"; +static constexpr llvm::StringLiteral CustomFunctionNamesId = "CustomFunctionNames"; +static constexpr llvm::StringLiteral CustomAnnexKFunctionNamesId = "CustomAnnexKFunctionNames"; static constexpr llvm::StringLiteral DeclRefId = "DRE"; static std::optional<std::string> @@ -127,55 +137,151 @@ static bool isAnnexKAvailable(std::optional<bool> &CacheVar, Preprocessor *PP, return CacheVar.value(); } +static std::vector<UnsafeFunctionsCheck::CheckedFunction> +ParseCheckedFunctions(StringRef Option, StringRef OptionName, + ClangTidyContext *Context) { + std::vector<StringRef> Functions = utils::options::parseStringList(Option); + std::vector<UnsafeFunctionsCheck::CheckedFunction> Result; + Result.reserve(Functions.size()); + + for (StringRef Function : Functions) { + if (Function.empty()) { + continue; + } + + auto [Name, Rest] = Function.split(','); + auto [Replacement, Reason] = Rest.split(','); + + if (Name.trim().empty()) { + Context->configurationDiag( + "invalid configuration value for option '%0'; " + "expected the name of an unsafe function") << OptionName; + } + + if (Replacement.trim().empty()) { + Context->configurationDiag( + "invalid configuration value '%0' for option '%1'; " + "expected a replacement function name") << Name.trim() << OptionName; + } + + Result.push_back({ Name.trim().str(), matchers::MatchesAnyListedNameMatcher::NameMatcher(Name.trim()), + Replacement.trim().str(), Reason.trim().str() }); + } + + return Result; +} + +static std::string SerializeCheckedFunctions( + const std::vector<UnsafeFunctionsCheck::CheckedFunction> &Functions) { + std::vector<std::string> Result; + Result.reserve(Functions.size()); + + for (const auto &Entry : Functions) { + if (Entry.Reason.empty()) + Result.push_back(Entry.Name + "," + Entry.Replacement); + else + Result.push_back(Entry.Name + "," + Entry.Replacement + "," + + Entry.Reason); + } + + return llvm::join(Result, ";"); +} + UnsafeFunctionsCheck::UnsafeFunctionsCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), + CustomNormalFunctions(ParseCheckedFunctions( + Options.get(OptionNameCustomNormalFunctions, ""), + OptionNameCustomNormalFunctions, Context)), + CustomAnnexKFunctions(ParseCheckedFunctions( + Options.get(OptionNameCustomAnnexKFunctions, ""), + OptionNameCustomAnnexKFunctions, Context)), + ReportDefaultFunctions(Options.get(OptionNameReportDefaultFunctions, true)), ReportMoreUnsafeFunctions( Options.get(OptionNameReportMoreUnsafeFunctions, true)) {} void UnsafeFunctionsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, OptionNameCustomNormalFunctions, + SerializeCheckedFunctions(CustomNormalFunctions)); + Options.store(Opts, OptionNameCustomAnnexKFunctions, + SerializeCheckedFunctions(CustomAnnexKFunctions)); + Options.store(Opts, OptionNameReportDefaultFunctions, ReportDefaultFunctions); Options.store(Opts, OptionNameReportMoreUnsafeFunctions, ReportMoreUnsafeFunctions); } void UnsafeFunctionsCheck::registerMatchers(MatchFinder *Finder) { - if (getLangOpts().C11) { - // Matching functions with safe replacements only in Annex K. - auto FunctionNamesWithAnnexKReplacementMatcher = hasAnyName( - "::bsearch", "::ctime", "::fopen", "::fprintf", "::freopen", "::fscanf", - "::fwprintf", "::fwscanf", "::getenv", "::gmtime", "::localtime", - "::mbsrtowcs", "::mbstowcs", "::memcpy", "::memmove", "::memset", - "::printf", "::qsort", "::scanf", "::snprintf", "::sprintf", "::sscanf", - "::strcat", "::strcpy", "::strerror", "::strlen", "::strncat", - "::strncpy", "::strtok", "::swprintf", "::swscanf", "::vfprintf", - "::vfscanf", "::vfwprintf", "::vfwscanf", "::vprintf", "::vscanf", - "::vsnprintf", "::vsprintf", "::vsscanf", "::vswprintf", "::vswscanf", - "::vwprintf", "::vwscanf", "::wcrtomb", "::wcscat", "::wcscpy", - "::wcslen", "::wcsncat", "::wcsncpy", "::wcsrtombs", "::wcstok", - "::wcstombs", "::wctomb", "::wmemcpy", "::wmemmove", "::wprintf", - "::wscanf"); + if (ReportDefaultFunctions) { + if (getLangOpts().C11) { + // Matching functions with safe replacements only in Annex K. + auto FunctionNamesWithAnnexKReplacementMatcher = hasAnyName( + "::bsearch", "::ctime", "::fopen", "::fprintf", "::freopen", "::fscanf", + "::fwprintf", "::fwscanf", "::getenv", "::gmtime", "::localtime", + "::mbsrtowcs", "::mbstowcs", "::memcpy", "::memmove", "::memset", + "::printf", "::qsort", "::scanf", "::snprintf", "::sprintf", "::sscanf", + "::strcat", "::strcpy", "::strerror", "::strlen", "::strncat", + "::strncpy", "::strtok", "::swprintf", "::swscanf", "::vfprintf", + "::vfscanf", "::vfwprintf", "::vfwscanf", "::vprintf", "::vscanf", + "::vsnprintf", "::vsprintf", "::vsscanf", "::vswprintf", "::vswscanf", + "::vwprintf", "::vwscanf", "::wcrtomb", "::wcscat", "::wcscpy", + "::wcslen", "::wcsncat", "::wcsncpy", "::wcsrtombs", "::wcstok", + "::wcstombs", "::wctomb", "::wmemcpy", "::wmemmove", "::wprintf", + "::wscanf"); + Finder->addMatcher( + declRefExpr(to(functionDecl(FunctionNamesWithAnnexKReplacementMatcher) + .bind(FunctionNamesWithAnnexKReplacementId))) + .bind(DeclRefId), + this); + } + + // Matching functions with replacements without Annex K. + auto FunctionNamesMatcher = + hasAnyName("::asctime", "asctime_r", "::gets", "::rewind", "::setbuf"); Finder->addMatcher( - declRefExpr(to(functionDecl(FunctionNamesWithAnnexKReplacementMatcher) - .bind(FunctionNamesWithAnnexKReplacementId))) + declRefExpr(to(functionDecl(FunctionNamesMatcher).bind(FunctionNamesId))) .bind(DeclRefId), this); + + if (ReportMoreUnsafeFunctions) { + // Matching functions with replacements without Annex K, at user request. + auto AdditionalFunctionNamesMatcher = + hasAnyName("::bcmp", "::bcopy", "::bzero", "::getpw", "::vfork"); + Finder->addMatcher( + declRefExpr(to(functionDecl(AdditionalFunctionNamesMatcher) + .bind(AdditionalFunctionNamesId))) + .bind(DeclRefId), + this); + } } - // Matching functions with replacements without Annex K. - auto FunctionNamesMatcher = - hasAnyName("::asctime", "asctime_r", "::gets", "::rewind", "::setbuf"); - Finder->addMatcher( - declRefExpr(to(functionDecl(FunctionNamesMatcher).bind(FunctionNamesId))) - .bind(DeclRefId), - this); - - if (ReportMoreUnsafeFunctions) { - // Matching functions with replacements without Annex K, at user request. - auto AdditionalFunctionNamesMatcher = - hasAnyName("::bcmp", "::bcopy", "::bzero", "::getpw", "::vfork"); + if (!CustomAnnexKFunctions.empty()) { + std::vector<llvm::StringRef> FunctionNames; + FunctionNames.reserve(CustomAnnexKFunctions.size()); + + for (const auto &Entry : CustomAnnexKFunctions) + FunctionNames.push_back(Entry.Name); + + auto CustomAnnexKFunctionsMatcher = + matchers::matchesAnyListedName(FunctionNames); + Finder->addMatcher( - declRefExpr(to(functionDecl(AdditionalFunctionNamesMatcher) - .bind(AdditionalFunctionNamesId))) + declRefExpr(to(functionDecl(CustomAnnexKFunctionsMatcher).bind(CustomAnnexKFunctionNamesId))) + .bind(DeclRefId), + this); + } + + if (!CustomNormalFunctions.empty()) { + std::vector<llvm::StringRef> FunctionNames; + FunctionNames.reserve(CustomNormalFunctions.size()); + + for (const auto &Entry : CustomNormalFunctions) + FunctionNames.push_back(Entry.Name); + + auto CustomFunctionsMatcher = + matchers::matchesAnyListedName(FunctionNames); + + Finder->addMatcher( + declRefExpr(to(functionDecl(CustomFunctionsMatcher).bind(CustomFunctionNamesId))) .bind(DeclRefId), this); } @@ -186,16 +292,62 @@ void UnsafeFunctionsCheck::check(const MatchFinder::MatchResult &Result) { const auto *FuncDecl = cast<FunctionDecl>(DeclRef->getDecl()); assert(DeclRef && FuncDecl && "No valid matched node in check()"); + // Only one of these are matched at a time. const auto *AnnexK = Result.Nodes.getNodeAs<FunctionDecl>( FunctionNamesWithAnnexKReplacementId); const auto *Normal = Result.Nodes.getNodeAs<FunctionDecl>(FunctionNamesId); const auto *Additional = Result.Nodes.getNodeAs<FunctionDecl>(AdditionalFunctionNamesId); - assert((AnnexK || Normal || Additional) && "No valid match category."); + const auto *CustomAnnexK = Result.Nodes.getNodeAs<FunctionDecl>( + CustomAnnexKFunctionNamesId); + const auto *CustomNormal = Result.Nodes.getNodeAs<FunctionDecl>( + CustomFunctionNamesId); + assert((AnnexK || Normal || Additional || CustomAnnexK || CustomNormal) && + "No valid match category."); bool AnnexKIsAvailable = isAnnexKAvailable(IsAnnexKAvailable, PP, getLangOpts()); StringRef FunctionName = FuncDecl->getName(); + + if (CustomAnnexK || CustomNormal) { + const auto ShowCheckedFunctionWarning = [&](const CheckedFunction &Entry) { + StringRef Reason = Entry.Reason.empty() ? "is marked as unsafe" + : Entry.Reason.c_str(); + diag(DeclRef->getExprLoc(), "function %0 %1; '%2' should be used instead") + << FuncDecl << Reason + << Entry.Replacement << DeclRef->getSourceRange(); + }; + + if (AnnexKIsAvailable) { + for (const auto &Entry : CustomAnnexKFunctions) { + if (Entry.Pattern.match(*FuncDecl)) { + // If both Annex K and Normal are matched, show Annex K warning only. + if (CustomAnnexK) + ShowCheckedFunctionWarning(Entry); + + return; + } + } + + assert(!CustomAnnexK && "No custom Annex K function was matched."); + } + + // Annex K was not available, or the assertion failed. + if (CustomAnnexK) + return; + + for (const auto &Entry : CustomNormalFunctions) { + + if (Entry.Pattern.match(*FuncDecl)) { + ShowCheckedFunctionWarning(Entry); + return; + } + } + + assert(false && "No custom function was matched."); + return; + } + const std::optional<std::string> ReplacementFunctionName = [&]() -> std::optional<std::string> { if (AnnexK) { diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h index 5adfee60d1a7de..ad23a807666e79 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h @@ -10,6 +10,7 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSAFEFUNCTIONSCHECK_H #include "../ClangTidyCheck.h" +#include "../utils/Matchers.h" #include <optional> namespace clang::tidy::bugprone { @@ -32,7 +33,19 @@ class UnsafeFunctionsCheck : public ClangTidyCheck { Preprocessor *ModuleExpanderPP) override; void onEndOfTranslationUnit() override; + struct CheckedFunction { + std::string Name; + matchers::MatchesAnyListedNameMatcher::NameMatcher Pattern; + std::string Replacement; + std::string Reason; + }; + private: + const std::vector<CheckedFunction> CustomNormalFunctions; + const std::vector<CheckedFunction> CustomAnnexKFunctions; + + // If true, the default set of functions are reported. + const bool ReportDefaultFunctions; /// If true, additional functions from widely used API-s (such as POSIX) are /// added to the list of reported functions. const bool ReportMoreUnsafeFunctions; diff --git a/clang-tools-extra/clang-tidy/utils/Matchers.h b/clang-tools-extra/clang-tidy/utils/Matchers.h index 5fd98db9678708..f79d872300320b 100644 --- a/clang-tools-extra/clang-tidy/utils/Matchers.h +++ b/clang-tools-extra/clang-tidy/utils/Matchers.h @@ -85,15 +85,7 @@ class MatchesAnyListedNameMatcher NameList.begin(), NameList.end(), std::back_inserter(NameMatchers), [](const llvm::StringRef Name) { return NameMatcher(Name); }); } - bool matches( - const NamedDecl &Node, ast_matchers::internal::ASTMatchFinder *Finder, - ast_matchers::internal::BoundNodesTreeBuilder *Builder) const override { - return llvm::any_of(NameMatchers, [&Node](const NameMatcher &NM) { - return NM.match(Node); - }); - } -private: class NameMatcher { llvm::Regex Regex; enum class MatchMode { @@ -136,6 +128,16 @@ class MatchesAnyListedNameMatcher } }; + bool matches( + const NamedDecl &Node, ast_matchers::internal::ASTMatchFinder *Finder, + ast_matchers::internal::BoundNodesTreeBuilder *Builder) const override { + return llvm::any_of(NameMatchers, [&Node](const NameMatcher &NM) { + return NM.match(Node); + }); + } + +private: + std::vector<NameMatcher> NameMatchers; }; diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst index a0a267883b6fe9..e97794cd8f30b0 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst @@ -19,6 +19,9 @@ The check implements the following rules from the CERT C Coding Standard: Unsafe functions ---------------- +The following functions are reported if `ReportDefaultFunctions +<unsafe-functions.html#cmdoption-arg-ReportDefaultFunctions>`_ is enabled. + If *Annex K.* is available, a replacement from *Annex K.* is suggested for the following functions: @@ -74,6 +77,40 @@ Both macros have to be defined to suggest replacement functions from *Annex K.* ``__STDC_WANT_LIB_EXT1__`` must be defined to ``1`` by the user **before** including any system headers. +Custom functions +---------------- + +The options `CustomNormalFunctions` and `CustomAnnexKFunctions` allow the user +to define custom functions to be checked. The format is the following, without +newlines: + +.. code:: + + bugprone-unsafe-functions.CustomNormalFunctions=" + functionRegex1, replacement1[, reason1]; + functionRegex2, replacement2[, reason2]; + ... + " + +The functions are matched using POSIX extended regular expressions. +*(Note: The regular expressions do not support negative* ``(?!)`` *matches)* + +The `reason` is optional and is used to provide additional information about the +reasoning behind the replacement. The default reason is ``is marked as unsafe``. + +As an example, the configuration ``^original$, replacement, is deprecated;`` +will produce the following diagnostic message. + +.. code:: c + + original(); // warning: function 'original' is deprecated; 'replacement' should be used instead. + ::std::original(); // no-warning + original_function(); // no-warning + +If the regular expression contains the character ``:``, it is matched against the +qualified name (i.e. ``std::original``), otherwise the regex is matched against the unqualified name (``original``). +If the regular expression starts with ``::`` (or ``^::``), it is matched against the +fully qualified name (``::std::original``). Options ------- @@ -86,6 +123,22 @@ Options this option enables. Default is `true`. +.. option:: ReportDefaultFunctions + + When `true`, the check reports the default set of functions. + Default is `true`. + +.. option:: CustomNormalFunctions + + A comma-separated list of regular expressions, their replacements, and an + optional reason. For more information, see `Custom functions + <unsafe-functions.html#custom-functions>`_. + +.. option:: CustomAnnexKFunctions + + A comma-separated list of regular expressions, their replacements, and an + optional reason. For more information, see `Custom functions + <unsafe-functions.html#custom-functions>`_. Examples -------- diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions-custom-regex.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions-custom-regex.cpp new file mode 100644 index 00000000000000..4554c43fb700d5 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions-custom-regex.cpp @@ -0,0 +1,63 @@ +// This test fails on "x86_64-sie" buildbot and "x86_64-scei-ps4" target. +// According to @dyung, something related to the kind of standard library +// availability is causing the failure. Even though we explicitly define +// the relevant macros the check is hunting for in the invocation, the real +// parsing and preprocessor state will not have that case. +// UNSUPPORTED: target={{.*-(ps4|ps5)}} +// +// RUN: %check_clang_tidy -check-suffix=NON-STRICT-REGEX %s bugprone-unsafe-functions %t --\ +// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.CustomNormalFunctions: '::non_annex_k_only,replacement;::both,replacement,is a normal function'}}"\ +// RUN: -- -U__STDC_LIB_EXT1__ -U__STDC_WANT_LIB_EXT1__ +// RUN: %check_clang_tidy -check-suffix=STRICT-REGEX %s bugprone-unsafe-functions %t --\ +// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.CustomNormalFunctions: '^non_annex_k_only$,replacement,is matched on function name only;^::both,replacement,is matched on qualname prefix'}}"\ +// RUN: -- -U__STDC_LIB_EXT1__ -U__STDC_WANT_LIB_EXT1__ +// RUN: %check_clang_tidy -check-suffix=WITH-ANNEX-K %s bugprone-unsafe-functions %t --\ +// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.CustomAnnexKFunctions: '::annex_k_only,replacement;::both,replacement,is an Annex K function'}}"\ +// RUN: -- -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1 + +// TODO: How is "Annex K" available in C++ mode? +// no-warning WITH-ANNEX-K + +void non_annex_k_only(); +void annex_k_only(); +void both(); + +namespace regex_test { +void non_annex_k_only(); +void both(); +} + +void non_annex_k_only_regex(); +void both_regex(); + +void f1() { + non_annex_k_only(); + // CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'non_annex_k_only' is marked as unsafe; 'replacement' should be used instead + // CHECK-MESSAGES-STRICT-REGEX: :[[@LINE-2]]:3: warning: function 'non_annex_k_only' is matched on function name only; 'replacement' should be used instead + annex_k_only(); + // no-warning NON-STRICT-REGEX + // no-warning STRICT-REGEX + both(); + // CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'both' is a normal function; 'replacement' should be... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/106350 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
