llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Zeyi Xu (zeyi2) <details> <summary>Changes</summary> --- Full diff: https://github.com/llvm/llvm-project/pull/189312.diff 6 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp (+35-11) - (modified) clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h (+5-2) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+13-5) - (modified) clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst (+18-6) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c (+18-12) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.cpp (+32-2) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp index d87511b6bdd73..b1b5f72fbb126 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp @@ -24,12 +24,16 @@ static constexpr StringRef OptionNameReportDefaultFunctions = "ReportDefaultFunctions"; static constexpr StringRef OptionNameReportMoreUnsafeFunctions = "ReportMoreUnsafeFunctions"; +static constexpr StringRef OptionNameReportDeprecatedFunctions = + "ReportDeprecatedFunctions"; static constexpr StringRef FunctionNamesWithAnnexKReplacementId = "FunctionNamesWithAnnexKReplacement"; static constexpr StringRef FunctionNamesId = "FunctionsNames"; static constexpr StringRef AdditionalFunctionNamesId = "AdditionalFunctionsNames"; +static constexpr StringRef DeprecatedFunctionNamesId = + "DeprecatedFunctionsNames"; static constexpr StringRef CustomFunctionNamesId = "CustomFunctionNames"; static constexpr StringRef DeclRefId = "DRE"; @@ -64,12 +68,16 @@ static StringRef getReplacementFor(StringRef FunctionName, .Case("get_temporary_buffer", "operator new[]"); } -static StringRef getReplacementForAdditional(StringRef FunctionName, +static StringRef getReplacementForAdditional(StringRef FunctionName) { + return StringSwitch<StringRef>(FunctionName).Case("getpw", "getpwuid"); +} + +static StringRef getReplacementForDeprecated(StringRef FunctionName, bool IsAnnexKAvailable) { if (IsAnnexKAvailable) { // Try to find a better replacement from Annex K first. StringRef AnnexKReplacementFunction = StringSwitch<StringRef>(FunctionName) - .Case("bcopy", "memcpy_s") + .Case("bcopy", "memmove_s") .Case("bzero", "memset_s") .Default({}); @@ -79,9 +87,8 @@ static StringRef getReplacementForAdditional(StringRef FunctionName, return StringSwitch<StringRef>(FunctionName) .Case("bcmp", "memcmp") - .Case("bcopy", "memcpy") + .Case("bcopy", "memmove") .Case("bzero", "memset") - .Case("getpw", "getpwuid") .Case("vfork", "posix_spawn"); } @@ -96,8 +103,7 @@ static StringRef getRationaleFor(StringRef FunctionName) { .Case("gets", "is insecure, was deprecated and removed in C11 and C++14") .Case("getpw", "is dangerous as it may overflow the provided buffer") .Cases({"rewind", "setbuf"}, "has no error detection") - .Case("vfork", "is insecure as it can lead to denial of service " - "situations in the parent process") + .Case("vfork", "is deprecated") .Case("get_temporary_buffer", "returns uninitialized memory without " "performance advantages, was deprecated in " "C++17 and removed in C++20") @@ -170,7 +176,9 @@ UnsafeFunctionsCheck::UnsafeFunctionsCheck(StringRef Name, ReportDefaultFunctions( Options.get(OptionNameReportDefaultFunctions, true)), ReportMoreUnsafeFunctions( - Options.get(OptionNameReportMoreUnsafeFunctions, true)) {} + Options.get(OptionNameReportMoreUnsafeFunctions, true)), + DeprecatedFunctions( + Options.get(OptionNameReportDeprecatedFunctions, false)) {} void UnsafeFunctionsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, OptionNameCustomFunctions, @@ -178,6 +186,7 @@ void UnsafeFunctionsCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, OptionNameReportDefaultFunctions, ReportDefaultFunctions); Options.store(Opts, OptionNameReportMoreUnsafeFunctions, ReportMoreUnsafeFunctions); + Options.store(Opts, OptionNameReportDeprecatedFunctions, DeprecatedFunctions); } void UnsafeFunctionsCheck::registerMatchers(MatchFinder *Finder) { @@ -216,14 +225,24 @@ void UnsafeFunctionsCheck::registerMatchers(MatchFinder *Finder) { if (ReportMoreUnsafeFunctions) { // Matching functions with replacements without Annex K, at user request. - auto AdditionalFunctionNamesMatcher = - hasAnyName("::bcmp", "::bcopy", "::bzero", "::getpw", "::vfork"); + auto AdditionalFunctionNamesMatcher = hasAnyName("::getpw"); Finder->addMatcher( declRefExpr(to(functionDecl(AdditionalFunctionNamesMatcher) .bind(AdditionalFunctionNamesId))) .bind(DeclRefId), this); } + + if (DeprecatedFunctions) { + // Matching deprecated functions from widely used APIs, at user request. + auto DeprecatedFunctionNamesMatcher = + hasAnyName("::bcmp", "::bcopy", "::bzero", "::vfork"); + Finder->addMatcher( + declRefExpr(to(functionDecl(DeprecatedFunctionNamesMatcher) + .bind(DeprecatedFunctionNamesId))) + .bind(DeclRefId), + this); + } } if (!CustomFunctions.empty()) { @@ -273,9 +292,11 @@ void UnsafeFunctionsCheck::check(const MatchFinder::MatchResult &Result) { const auto *Normal = Result.Nodes.getNodeAs<FunctionDecl>(FunctionNamesId); const auto *Additional = Result.Nodes.getNodeAs<FunctionDecl>(AdditionalFunctionNamesId); + const auto *Deprecated = + Result.Nodes.getNodeAs<FunctionDecl>(DeprecatedFunctionNamesId); const auto *Custom = Result.Nodes.getNodeAs<FunctionDecl>(CustomFunctionNamesId); - assert((AnnexK || Normal || Additional || Custom) && + assert((AnnexK || Normal || Additional || Deprecated || Custom) && "No valid match category."); bool AnnexKIsAvailable = @@ -326,7 +347,10 @@ void UnsafeFunctionsCheck::check(const MatchFinder::MatchResult &Result) { return getReplacementFor(FunctionName, AnnexKIsAvailable).str(); if (Additional) - return getReplacementForAdditional(FunctionName, AnnexKIsAvailable).str(); + return getReplacementForAdditional(FunctionName).str(); + + if (Deprecated) + return getReplacementForDeprecated(FunctionName, AnnexKIsAvailable).str(); llvm_unreachable("Unhandled match category"); }(); diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h index 58d9b2830dafc..f0689c19e8870 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h @@ -45,9 +45,12 @@ class UnsafeFunctionsCheck : public ClangTidyCheck { /// 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. + /// If true, additional unsafe functions from widely used API-s (such as + /// POSIX) are added to the list of reported functions. const bool ReportMoreUnsafeFunctions; + /// If true, additional deprecated functions from widely used API-s (such as + /// POSIX) are added to the list of reported functions. + const bool DeprecatedFunctions; Preprocessor *PP = nullptr; /// Whether "Annex K" functions are available and should be diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index f8550e72dcc85..0cc889e89a77e 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -205,7 +205,7 @@ Changes in existing checks C++ files because suggested ``reinterpret_cast`` is not available in pure C. - Improved :doc:`bugprone-derived-method-shadowing-base-method - <clang-tidy/checks/bugprone/derived-method-shadowing-base-method>` check by + <clang-tidy/checks/bugprone/derived-method-shadowing-base-method>` check by correctly ignoring function templates. - Improved :doc:`bugprone-exception-escape @@ -231,7 +231,7 @@ Changes in existing checks <clang-tidy/checks/bugprone/std-namespace-modification>` check by fixing false positives when extending the standard library with a specialization of user-defined type and by removing detection of the compiler generated ``std`` - namespace extensions. + namespace extensions. - Improved :doc:`bugprone-string-constructor <clang-tidy/checks/bugprone/string-constructor>` check to detect suspicious @@ -244,9 +244,17 @@ Changes in existing checks number of false positives in test code. - Improved :doc:`bugprone-unsafe-functions - <clang-tidy/checks/bugprone/unsafe-functions>` check by adding the function - ``std::get_temporary_buffer`` to the default list of unsafe functions. (This - function is unsafe, useless, deprecated in C++17 and removed in C++20). + <clang-tidy/checks/bugprone/unsafe-functions>` check: + + - Added ``std::get_temporary_buffer`` to the default list of unsafe + functions. This function is unsafe, useless, deprecated in C++17, and + removed in C++20. + + - Added ``ReportDeprecatedFunctions`` to emit diagnostics for deprecated + functions from widely used APIs. + + - Updated the suggested replacement for ``bcopy`` from + ``memcpy``/``memcpy_s`` to ``memmove``/``memmove_s``. - Improved :doc:`bugprone-use-after-move <clang-tidy/checks/bugprone/use-after-move>` check: 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 4f5f8b39ed406..9bb37a1e47b3e 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 @@ -52,14 +52,18 @@ availability: with ``operator new[]`` If :option:`ReportMoreUnsafeFunctions` is enabled, -the following functions are also checked: +the following function is also checked: + + - ``getpw``, suggested replacement: ``getpwuid`` + +If :option:`ReportDeprecatedFunctions` is enabled, +the following deprecated functions are also checked: - ``bcmp``, suggested replacement: ``memcmp`` - - ``bcopy``, suggested replacement: ``memcpy_s`` if *Annex K* is available, - or ``memcpy`` + - ``bcopy``, suggested replacement: ``memmove_s`` if *Annex K* is available, + or ``memmove`` - ``bzero``, suggested replacement: ``memset_s`` if *Annex K* is available, or ``memset`` - - ``getpw``, suggested replacement: ``getpwuid`` - ``vfork``, suggested replacement: ``posix_spawn`` Although mentioned in the associated CERT rules, the following functions are @@ -161,12 +165,20 @@ Options .. option:: ReportMoreUnsafeFunctions - When `true`, additional functions from widely used APIs (such as POSIX) are - added to the list of reported functions. + When `true`, additional unsafe functions from widely used APIs (such as + POSIX) are added to the list of reported functions. See the main documentation of the check for the complete list as to what this option enables. Default is `true`. +.. option:: ReportDeprecatedFunctions + + When `true`, additional deprecated functions from widely used APIs (such as + POSIX) are added to the list of reported functions. + See the main documentation of the check for the complete list as to what + this option enables. + Default is `false`. + .. option:: ReportDefaultFunctions When `true`, the check reports the default set of functions. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c index 452e2295cb599..ffb829f94fd37 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c @@ -9,12 +9,18 @@ // RUN: %check_clang_tidy -check-suffix=WITHOUT-ANNEX-K %s bugprone-unsafe-functions %t -- -- -U__STDC_LIB_EXT1__ -U__STDC_WANT_LIB_EXT1__ // RUN: %check_clang_tidy -check-suffix=WITHOUT-ANNEX-K %s bugprone-unsafe-functions %t -- -- -D__STDC_LIB_EXT1__=1 -U__STDC_WANT_LIB_EXT1__ // RUN: %check_clang_tidy -check-suffix=WITHOUT-ANNEX-K %s bugprone-unsafe-functions %t -- -- -U__STDC_LIB_EXT1__ -D__STDC_WANT_LIB_EXT1__=1 +// RUN: %check_clang_tidy -std=c11-or-later -check-suffixes=WITH-ANNEX-K,WITH-ANNEX-K-DEPRECATED %s bugprone-unsafe-functions %t -- \ +// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.ReportDeprecatedFunctions: true}}" -- -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1 +// RUN: %check_clang_tidy -check-suffixes=WITHOUT-ANNEX-K,WITHOUT-ANNEX-K-DEPRECATED %s bugprone-unsafe-functions %t -- \ +// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.ReportDeprecatedFunctions: true}}" -- -U__STDC_LIB_EXT1__ -U__STDC_WANT_LIB_EXT1__ +// RUN: %check_clang_tidy -check-suffixes=WITHOUT-ANNEX-K,WITHOUT-ANNEX-K-DEPRECATED %s bugprone-unsafe-functions %t -- \ +// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.ReportDeprecatedFunctions: true}}" -- -D__STDC_LIB_EXT1__=1 -U__STDC_WANT_LIB_EXT1__ +// RUN: %check_clang_tidy -check-suffixes=WITHOUT-ANNEX-K,WITHOUT-ANNEX-K-DEPRECATED %s bugprone-unsafe-functions %t -- \ +// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.ReportDeprecatedFunctions: true}}" -- -U__STDC_LIB_EXT1__ -D__STDC_WANT_LIB_EXT1__=1 // RUN: %check_clang_tidy -std=c11-or-later -check-suffix=WITH-ANNEX-K-CERT-ONLY %s bugprone-unsafe-functions %t -- \ -// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.ReportMoreUnsafeFunctions: false}}" \ -// RUN: -- -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1 +// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.ReportMoreUnsafeFunctions: false}}" -- -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1 // RUN: %check_clang_tidy -check-suffix=WITH-NONE-ENABLED %s bugprone-unsafe-functions %t --\ -// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.ReportDefaultFunctions: false}}" \ -// RUN: -- -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1 +// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.ReportDefaultFunctions: false}}" -- -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1 // CHECK-MESSAGES-WITH-NONE-ENABLED: 1 warning generated // CHECK-MESSAGES-WITH-NONE-ENABLED: Suppressed 1 warnings @@ -127,18 +133,18 @@ void fOptional() { char Buf2[BUFSIZ] = {0}; bcmp(Buf1, Buf2, BUFSIZ); - // CHECK-MESSAGES-WITH-ANNEX-K: :[[@LINE-1]]:3: warning: function 'bcmp' is deprecated; 'memcmp' should be used instead - // CHECK-MESSAGES-WITHOUT-ANNEX-K: :[[@LINE-2]]:3: warning: function 'bcmp' is deprecated; 'memcmp' should be used instead + // CHECK-MESSAGES-WITH-ANNEX-K-DEPRECATED: :[[@LINE-1]]:3: warning: function 'bcmp' is deprecated; 'memcmp' should be used instead + // CHECK-MESSAGES-WITHOUT-ANNEX-K-DEPRECATED: :[[@LINE-2]]:3: warning: function 'bcmp' is deprecated; 'memcmp' should be used instead // no-warning CERT-ONLY bcopy(Buf1, Buf2, BUFSIZ); - // CHECK-MESSAGES-WITH-ANNEX-K: :[[@LINE-1]]:3: warning: function 'bcopy' is deprecated; 'memcpy_s' should be used instead - // CHECK-MESSAGES-WITHOUT-ANNEX-K: :[[@LINE-2]]:3: warning: function 'bcopy' is deprecated; 'memcpy' should be used instead + // CHECK-MESSAGES-WITH-ANNEX-K-DEPRECATED: :[[@LINE-1]]:3: warning: function 'bcopy' is deprecated; 'memmove_s' should be used instead + // CHECK-MESSAGES-WITHOUT-ANNEX-K-DEPRECATED: :[[@LINE-2]]:3: warning: function 'bcopy' is deprecated; 'memmove' should be used instead // no-warning CERT-ONLY bzero(Buf1, BUFSIZ); - // CHECK-MESSAGES-WITH-ANNEX-K: :[[@LINE-1]]:3: warning: function 'bzero' is deprecated; 'memset_s' should be used instead - // CHECK-MESSAGES-WITHOUT-ANNEX-K: :[[@LINE-2]]:3: warning: function 'bzero' is deprecated; 'memset' should be used instead + // CHECK-MESSAGES-WITH-ANNEX-K-DEPRECATED: :[[@LINE-1]]:3: warning: function 'bzero' is deprecated; 'memset_s' should be used instead + // CHECK-MESSAGES-WITHOUT-ANNEX-K-DEPRECATED: :[[@LINE-2]]:3: warning: function 'bzero' is deprecated; 'memset' should be used instead // no-warning CERT-ONLY getpw(0, Buf1); @@ -147,8 +153,8 @@ void fOptional() { // no-warning CERT-ONLY vfork(); - // CHECK-MESSAGES-WITH-ANNEX-K: :[[@LINE-1]]:3: warning: function 'vfork' is insecure as it can lead to denial of service situations in the parent process; 'posix_spawn' should be used instead - // CHECK-MESSAGES-WITHOUT-ANNEX-K: :[[@LINE-2]]:3: warning: function 'vfork' is insecure as it can lead to denial of service situations in the parent process; 'posix_spawn' should be used instead + // CHECK-MESSAGES-WITH-ANNEX-K-DEPRECATED: :[[@LINE-1]]:3: warning: function 'vfork' is deprecated; 'posix_spawn' should be used instead + // CHECK-MESSAGES-WITHOUT-ANNEX-K-DEPRECATED: :[[@LINE-2]]:3: warning: function 'vfork' is deprecated; 'posix_spawn' should be used instead // no-warning CERT-ONLY } diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.cpp index 59080155ad8c6..1dc1655b3a35b 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.cpp @@ -1,4 +1,6 @@ -// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-unsafe-functions %t +// RUN: %check_clang_tidy -std=c++11-or-later -check-suffixes=DEFAULT %s bugprone-unsafe-functions %t +// RUN: %check_clang_tidy -std=c++11-or-later -check-suffixes=DEPRECATED %s bugprone-unsafe-functions %t -- \ +// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.ReportDeprecatedFunctions: true}}" #include <utility> #include <cstddef> @@ -10,7 +12,35 @@ std::pair<T*, std::ptrdiff_t> get_temporary_buffer(std::ptrdiff_t count) noexcept; } +extern "C" { +int bcmp(const void *, const void *, std::size_t); +void bcopy(const void *, void *, std::size_t); +void bzero(void *, std::size_t); +int getpw(int, char *); +int vfork(); +} + void test() { (void)std::get_temporary_buffer<int>(64); - // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: function 'get_temporary_buffer<int>' returns uninitialized memory without performance advantages, was deprecated in C++17 and removed in C++20; 'operator new[]' should be used instead + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:9: warning: function 'get_temporary_buffer<int>' returns uninitialized memory without performance advantages, was deprecated in C++17 and removed in C++20; 'operator new[]' should be used instead + // CHECK-MESSAGES-DEPRECATED: :[[@LINE-2]]:9: warning: function 'get_temporary_buffer<int>' returns uninitialized memory without performance advantages, was deprecated in C++17 and removed in C++20; 'operator new[]' should be used instead + + char Buf[32] = {}; + char Other[32] = {}; + + getpw(0, Buf); + // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:3: warning: function 'getpw' is dangerous as it may overflow the provided buffer; 'getpwuid' should be used instead + // CHECK-MESSAGES-DEPRECATED: :[[@LINE-2]]:3: warning: function 'getpw' is dangerous as it may overflow the provided buffer; 'getpwuid' should be used instead + + bcmp(Buf, Other, sizeof(Buf)); + // CHECK-MESSAGES-DEPRECATED: :[[@LINE-1]]:3: warning: function 'bcmp' is deprecated; 'memcmp' should be used instead + + bcopy(Buf, Other, sizeof(Buf)); + // CHECK-MESSAGES-DEPRECATED: :[[@LINE-1]]:3: warning: function 'bcopy' is deprecated; 'memmove' should be used instead + + bzero(Buf, sizeof(Buf)); + // CHECK-MESSAGES-DEPRECATED: :[[@LINE-1]]:3: warning: function 'bzero' is deprecated; 'memset' should be used instead + + vfork(); + // CHECK-MESSAGES-DEPRECATED: :[[@LINE-1]]:3: warning: function 'vfork' is deprecated; 'posix_spawn' should be used instead } `````````` </details> https://github.com/llvm/llvm-project/pull/189312 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
