[clang-tidy] Add C++ member function support to custom bugprone-unsafe-functions matches #117165
Conversation
This file contains hidden or 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
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Discookie (Discookie) ChangesBefore, C++ member functions in the format of Full diff: https://github.com/llvm/llvm-project/pull/117165.diff 4 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp
index 604a7cac0e4903..a45949314a4ca8 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp
@@ -256,13 +256,32 @@ void UnsafeFunctionsCheck::registerMatchers(MatchFinder *Finder) {
.bind(CustomFunctionNamesId)))
.bind(DeclRefId),
this);
+ // C++ member calls do not contain a DeclRefExpr to the function decl.
+ // Instead, they contain a MemberExpr that refers to the decl.
+ Finder->addMatcher(memberExpr(member(functionDecl(CustomFunctionsMatcher)
+ .bind(CustomFunctionNamesId)))
+ .bind(DeclRefId),
+ this);
}
}
void UnsafeFunctionsCheck::check(const MatchFinder::MatchResult &Result) {
- const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>(DeclRefId);
- const auto *FuncDecl = cast<FunctionDecl>(DeclRef->getDecl());
- assert(DeclRef && FuncDecl && "No valid matched node in check()");
+ const Expr *SourceExpr;
+ const FunctionDecl *FuncDecl;
+
+ if (const auto *DeclRef = Result.Nodes.getNodeAs<DeclRefExpr>(DeclRefId)) {
+ SourceExpr = DeclRef;
+ FuncDecl = cast<FunctionDecl>(DeclRef->getDecl());
+ } else if (const auto *Member =
+ Result.Nodes.getNodeAs<MemberExpr>(DeclRefId)) {
+ SourceExpr = Member;
+ FuncDecl = cast<FunctionDecl>(Member->getMemberDecl());
+ } else {
+ llvm_unreachable("No valid matched node in check()");
+ return;
+ }
+
+ assert(SourceExpr && FuncDecl && "No valid matched node in check()");
// Only one of these are matched at a time.
const auto *AnnexK = Result.Nodes.getNodeAs<FunctionDecl>(
@@ -286,14 +305,15 @@ void UnsafeFunctionsCheck::check(const MatchFinder::MatchResult &Result) {
Entry.Reason.empty() ? "is marked as unsafe" : Entry.Reason.c_str();
if (Entry.Replacement.empty()) {
- diag(DeclRef->getExprLoc(), "function %0 %1; it should not be used")
+ diag(SourceExpr->getExprLoc(),
+ "function %0 %1; it should not be used")
<< FuncDecl << Reason << Entry.Replacement
- << DeclRef->getSourceRange();
+ << SourceExpr->getSourceRange();
} else {
- diag(DeclRef->getExprLoc(),
+ diag(SourceExpr->getExprLoc(),
"function %0 %1; '%2' should be used instead")
<< FuncDecl << Reason << Entry.Replacement
- << DeclRef->getSourceRange();
+ << SourceExpr->getSourceRange();
}
return;
@@ -323,9 +343,9 @@ void UnsafeFunctionsCheck::check(const MatchFinder::MatchResult &Result) {
if (!ReplacementFunctionName)
return;
- diag(DeclRef->getExprLoc(), "function %0 %1; '%2' should be used instead")
+ diag(SourceExpr->getExprLoc(), "function %0 %1; '%2' should be used instead")
<< FuncDecl << getRationaleFor(FunctionName)
- << ReplacementFunctionName.value() << DeclRef->getSourceRange();
+ << ReplacementFunctionName.value() << SourceExpr->getSourceRange();
}
void UnsafeFunctionsCheck::registerPPCallbacks(
diff --git a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
index 26befe0de59ae4..0bd9c109af97f5 100644
--- a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
@@ -331,6 +331,10 @@ class CERTModule : public ClangTidyModule {
// STR
CheckFactories.registerCheck<bugprone::SignedCharMisuseCheck>(
"cert-str34-c");
+ // temp
+
+ CheckFactories.registerCheck<bugprone::UnsafeFunctionsCheck>(
+ "ericsson-unsafe-functions");
}
ClangTidyOptions getModuleOptions() override {
@@ -347,6 +351,159 @@ class CERTModule : public ClangTidyModule {
Opts["cert-err33-c.AllowCastToVoid"] = "true";
Opts["cert-oop54-cpp.WarnOnlyIfThisHasSuspiciousField"] = "false";
Opts["cert-str34-c.DiagnoseSignedUnsignedCharComparisons"] = "false";
+ Opts["ericsson-unsafe-functions.ReportDefaultFunctions"] = "false";
+ Opts["ericsson-unsafe-functions.CustomFunctions"] =
+ // High priority
+ "^::gets$,fgets,is insecure and deprecated;"
+ "^::sprintf$,snprintf,is insecure and deprecated;"
+ "^::strcpy$,strlcpy,is insecure and deprecated;"
+ "^::strncpy$,strlcpy,is insecure and deprecated;"
+ "^::vsprintf,vsnprintf,is insecure and deprecated;"
+ "^::wcscat$,wcslcat,is insecure and deprecated;"
+ "^::wcscpy$,wcslcpy,is insecure and deprecated;"
+ "^::wcsncat$,wcslcat,is insecure and deprecated;"
+ "^::wcsncpy$,wcslcpy,is insecure and deprecated;"
+
+ "^::bcopy$,memcpy or memmove,is insecure and deprecated;"
+ "^::bzero$,memset,is insecure and deprecated;"
+ "^::index$,strchr,is insecure and deprecated;"
+ "^::rindex$,strrchr,is insecure and deprecated;"
+
+ "^::valloc$,aligned_alloc,is insecure and deprecated;"
+
+ "^::tmpnam$,mkstemp,is insecure and deprecated;"
+ "^::tmpnam_r$,mkstemp,is insecure and deprecated;"
+
+ "^::getwd$,getcwd,is insecure and deprecated;"
+ "^::crypt$,an Ericsson-recommended crypto algorithm,is insecure and "
+ "deprecated;"
+ "^::encrypt$,an Ericsson-recommended crypto algorithm,is insecure and "
+ "deprecated;"
+ "^::stpcpy$,strlcpy,is insecure and deprecated;"
+ "^::strcat$,strlcat,is insecure and deprecated;"
+ "^::strncat$,strlcat,is insecure and deprecated;"
+
+ // Medium priority
+ "^::scanf$,strto__,lacks error detection;"
+ "^::fscanf$,strto__,lacks error detection;"
+ "^::sscanf$,strto__,lacks error detection;"
+ "^::vscanf$,strto__,lacks error detection;"
+ "^::vsscanf$,strto__,lacks error detection;"
+
+ "^::atof$,strtod,lacks error detection;"
+ "^::atof_l$,strtod,lacks error detection;"
+ "^::atoi$,strtol,lacks error detection;"
+ "^::atoi_l$,strtol,lacks error detection;"
+ "^::atol$,strtol,lacks error detection;"
+ "^::atol_l$,strtol,lacks error detection;"
+ "^::atoll$,strtoll,lacks error detection;"
+ "^::atoll_l$,strtoll,lacks error detection;"
+ "^::setbuf$,setvbuf,lacks error detection;"
+ "^::setjmp$,,lacks error detection;"
+ "^::sigsetjmp$,,lacks error detection;"
+ "^::longjmp$,,lacks error detection;"
+ "^::siglongjmp$,,lacks error detection;"
+ "^::rewind$,fseek,lacks error detection;"
+
+ // Low priority
+ "^::strtok$,strtok_r,is not thread safe;"
+ "^::strerror$,strerror_r,is not thread safe;"
+ "^::wcstombs$,_FORTIFY_SOURCE,is recommended to have source hardening;"
+ "^::mbstowcs$,_FORTIFY_SOURCE,is recommended to have source hardening;"
+
+ "^::getenv$,,is not thread safe;"
+ "^::mktemp$,mkstemp,is not thread safe;"
+ "^::perror$,,is not thread safe;"
+
+ "^::access$,,is not thread safe;"
+ "^::asctime$,asctime_r,is not thread safe;"
+ "^::atomic_init$,,is not thread safe;"
+ "^::c16rtomb$,,is not thread safe;"
+ "^::c32rtomb$,,is not thread safe;"
+ "^::catgets$,,is not thread safe;"
+ "^::ctermid$,,is not thread safe;"
+ "^::ctime$,cmtime_r,is not thread safe;"
+ "^::dbm_clearerr$,a database client library,is not thread safe;"
+ "^::dbm_close$,a database client library,is not thread safe;"
+ "^::dbm_delete$,a database client library,is not thread safe;"
+ "^::dbm_error$,a database client library,is not thread safe;"
+ "^::dbm_fetch$,a database client library,is not thread safe;"
+ "^::dbm_firstkey$,a database client library,is not thread safe;"
+ "^::dbm_nextkey$,a database client library,is not thread safe;"
+ "^::dbm_open$,a database client library,is not thread safe;"
+ "^::dbm_store$,a database client library,is not thread safe;"
+ "^::dlerror$,,is not thread safe;"
+ "^::drand48$,drand48_r,is not thread safe;"
+ "^::endgrent$,,is not thread safe;"
+ "^::endpwent$,,is not thread safe;"
+ "^::endutxent$,,is not thread safe;"
+ "^::getc_unlocked$,,is not thread safe;"
+ "^::getchar_unlocked$,,is not thread safe;"
+ "^::getdate$,,is not thread safe;"
+ "^::getgrent$,,is not thread safe;"
+ "^::getgrgid$,,is not thread safe;"
+ "^::getgrnam$,,is not thread safe;"
+ "^::gethostent$,,is not thread safe;"
+ "^::getlogin$,,is not thread safe;"
+ "^::getnetbyaddr$,,is not thread safe;"
+ "^::getnetbyname$,,is not thread safe;"
+ "^::getnetent$,,is not thread safe;"
+ "^::getopt$,,is not thread safe;"
+ "^::getprotobyname$,,is not thread safe;"
+ "^::getprotobynumber$,,is not thread safe;"
+ "^::getprotoent$,,is not thread safe;"
+ "^::getpwent$,,is not thread safe;"
+ "^::getpwnam$,,is not thread safe;"
+ "^::getpwuid$,,is not thread safe;"
+ "^::getservbyname$,,is not thread safe;"
+ "^::getservbyport$,,is not thread safe;"
+ "^::getservent$,,is not thread safe;"
+ "^::getutxent$,,is not thread safe;"
+ "^::getutxid$,,is not thread safe;"
+ "^::getutxline$,,is not thread safe;"
+ "^::gmtime$,gmtime_r,is not thread safe;"
+ "^::hcreate$,,is not thread safe;"
+ "^::hdestroy$,,is not thread safe;"
+ "^::hsearch$,,is not thread safe;"
+ "^::inet_ntoa$,,is not thread safe;"
+ "^::l64a$,,is not thread safe;"
+ "^::lgamma$,,is not thread safe;"
+ "^::lgammaf$,,is not thread safe;"
+ "^::lgammal$,,is not thread safe;"
+ "^::localeconv$,,is not thread safe;"
+ "^::localtime$,localtime_r,is not thread safe;"
+ "^::lrand48$,lrand48_r,is not thread safe;"
+ "^::mblen$,,is not thread safe;"
+ "^::mbrto16$,,is not thread safe;"
+ "^::mbrto32$,,is not thread safe;"
+ "^::mbrtowc$,,is not thread safe;"
+ "^::mbsnrtowcs$,,is not thread safe;"
+ "^::mbsrtowcs$,,is not thread safe;"
+ "^::mrand48$,mrand48_r,is not thread safe;"
+ "^::nftw$,,is not thread safe;"
+ "^::ni_langinfo$,,is not thread safe;"
+ "^::ptsname$,,is not thread safe;"
+ "^::putc_unlocked$,,is not thread safe;"
+ "^::putchar_unlocked$,,is not thread safe;"
+ "^::putenv$,,is not thread safe;"
+ "^::pututxline$,,is not thread safe;"
+ "^::rand$,,is not thread safe;"
+ "^::readdir$,,is not thread safe;"
+ "^::setenv$,,is not thread safe;"
+ "^::setgrent$,,is not thread safe;"
+ "^::setkey$,an Ericsson-recommended crypto algorithm,is insecure, "
+ "deprecated, and not thread safe;"
+ "^::setlocale$,,is not thread safe;"
+ "^::setpwent$,,is not thread safe;"
+ "^::setutxent$,,is not thread safe;"
+ "^::srand$,,is not thread safe;"
+ "^::strsignal$,,is not thread safe;"
+ "^::ttyname$,,is not thread safe;"
+ "^::unsetenv$,,is not thread safe;"
+ "^::wctomb$,,is not thread safe;"
+ "^::wcrtomb$,,is not thread safe;"
+ "^::wcsnrtombs$,,is not thread safe;"
+ "^::wcsrtombs$,,is not thread safe;";
return Options;
}
};
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index f967dfabd1c940..94f70c51f00a81 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -192,7 +192,7 @@ Changes in existing checks
- Improved :doc:`bugprone-unsafe-functions
<clang-tidy/checks/bugprone/unsafe-functions>` check to allow specifying
- additional functions to match.
+ additional functions to match, including C++ member functions.
- Improved :doc:`bugprone-use-after-move
<clang-tidy/checks/bugprone/use-after-move>` to avoid triggering on
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
index fc97d1bc93bc54..b707e471ef7cc4 100644
--- 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
@@ -1,11 +1,16 @@
// RUN: %check_clang_tidy -check-suffix=NON-STRICT-REGEX %s bugprone-unsafe-functions %t --\
-// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.CustomFunctions: '::name_match,replacement,is a qualname match;^::prefix_match,,is matched on qualname prefix'}}"
+// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.CustomFunctions: '::name_match,replacement,is a qualname match;^::prefix_match,,is matched on qualname prefix;^::S::member_match_,,is matched on a C++ class member'}}"
// RUN: %check_clang_tidy -check-suffix=STRICT-REGEX %s bugprone-unsafe-functions %t --\
-// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.CustomFunctions: '^name_match$,replacement,is matched on function name only;^::prefix_match$,,is a full qualname match'}}"
+// RUN: -config="{CheckOptions: {bugprone-unsafe-functions.CustomFunctions: '^name_match$,replacement,is matched on function name only;^::prefix_match$,,is a full qualname match;^::S::member_match_1$,,is matched on a C++ class member'}}"
void name_match();
void prefix_match();
+struct S {
+ static void member_match_1() {}
+ void member_match_2() {}
+};
+
namespace regex_test {
void name_match();
void prefix_match();
@@ -42,3 +47,19 @@ void f1() {
// CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'prefix_match_regex' is matched on qualname prefix; it should not be used
// no-warning STRICT-REGEX
}
+
+void f2() {
+ S s;
+
+ S::member_match_1();
+ // CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:3: warning: function 'member_match_1' is matched on a C++ class member; it should not be used
+ // CHECK-MESSAGES-STRICT-REGEX: :[[@LINE-2]]:3: warning: function 'member_match_1' is matched on a C++ class member; it should not be used
+
+ s.member_match_1();
+ // CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:5: warning: function 'member_match_1' is matched on a C++ class member; it should not be used
+ // CHECK-MESSAGES-STRICT-REGEX: :[[@LINE-2]]:5: warning: function 'member_match_1' is matched on a C++ class member; it should not be used
+
+ s.member_match_2();
+ // CHECK-MESSAGES-NON-STRICT-REGEX: :[[@LINE-1]]:5: warning: function 'member_match_2' is matched on a C++ class member; it should not be used
+ // no-warning STRICT-REGEX
+}
|
…-unsafe-functions matches
Discookie
force-pushed
the
unsafe-member-functions
branch
from
8116524 to
e90ab99
Compare
PiotrZSL
requested changes
Nov 21, 2024
clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions-custom-regex.cpp
Show resolved
Hide resolved
|
#include <fcntl.h>
#include <fstream>
void f() {
int fd = open("file.txt", O_RDONLY, 0);
std::ifstream file;
file.open("file.txt");
}I do realize that the matching of templates and child classes in general need a bit of a cleanup. |
Discookie
force-pushed
the
unsafe-member-functions
branch
from
b54ee25 to
3bec1ac
Compare
5chmidti
reviewed
Dec 6, 2024
Comment on lines
46
to
48
| /// If true, the fully qualified name of custom functions will be shown in a | ||
| /// note tag. | ||
| const bool ShowFullyQualifiedNames; |
There was a problem hiding this comment.
I don't think this is something that should be added. The only reason this exists, is to provide users the capability to debug false-positive matches on their regex. Or is there an additional reason to add this option?
There was a problem hiding this comment.
Sometimes it is pretty difficult to discern the precise patteren in the first place. eg. above when matching .open(), aliases are not followed (ifstream =/= basic_ifstream<char>), and there's also a template parameter on the class name, but not on the function.
These are existing behaviors, and I haven't found a way to strip template data off of the classname yet.
I'll add some examples for template arguments to the docs.
HerrCai0907
reviewed
Dec 13, 2024
|
Removed |
5chmidti
approved these changes
Jan 9, 2025
clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before, C++ member functions in the format of
Class instance; instance.memberfn();were unable to be matched.This PR adds support for this type of call, and it is matched in exactly the same format as other functions (eg.
::Class::memberfnqualified name).