[clang] e1d4ddb - Reapply "[analyzer] Accept C library functions from the `std` namespace" again (#85791)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 25 04:43:56 PDT 2024
Author: NagyDonat
Date: 2024-03-25T12:43:51+01:00
New Revision: e1d4ddb0c68f69001d7de43d9c678f4d73c1ecba
URL: https://github.com/llvm/llvm-project/commit/e1d4ddb0c68f69001d7de43d9c678f4d73c1ecba
DIFF: https://github.com/llvm/llvm-project/commit/e1d4ddb0c68f69001d7de43d9c678f4d73c1ecba.diff
LOG: Reapply "[analyzer] Accept C library functions from the `std` namespace" again (#85791)
This reapplies 80ab8234ac309418637488b97e0a62d8377b2ecf again, after
fixing a name collision warning in the unit tests (see the revert commit
13ccaf9b9d4400bb128b35ff4ac733e4afc3ad1c for details).
In addition to the previously applied changes, this commit also clarifies the
code in MallocChecker that distinguishes POSIX "getline()" and C++ standard
library "std::getline()" (which are two completely different functions). Note
that "std::getline()" was (accidentally) handled correctly even without this
clarification; but it's better to explicitly handle and test this corner case.
---------
Co-authored-by: Balazs Benics <benicsbalazs at gmail.com>
Added:
clang/test/Analysis/getline-cpp.cpp
clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
Modified:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
clang/test/Analysis/Inputs/system-header-simulator-cxx.h
clang/unittests/StaticAnalyzer/CMakeLists.txt
llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn
Removed:
################################################################################
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
index 3432d2648633c2..b4e1636130ca7c 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h
@@ -41,12 +41,8 @@ class CallDescription {
/// - We also accept calls where the number of arguments or parameters is
/// greater than the specified value.
/// For the exact heuristics, see CheckerContext::isCLibraryFunction().
- /// Note that functions whose declaration context is not a TU (e.g.
- /// methods, functions in namespaces) are not accepted as C library
- /// functions.
- /// FIXME: If I understand it correctly, this discards calls where C++ code
- /// refers a C library function through the namespace `std::` via headers
- /// like <cstdlib>.
+ /// (This mode only matches functions that are declared either directly
+ /// within a TU or in the namespace `std`.)
CLibrary,
/// Matches "simple" functions that are not methods. (Static methods are
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index c2d96f59260906..88fb42b6625aa4 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -394,8 +394,10 @@ class MallocChecker
const CallEvent &Call, CheckerContext &C)>;
const CallDescriptionMap<CheckFn> PreFnMap{
- {{{"getline"}, 3}, &MallocChecker::preGetdelim},
- {{{"getdelim"}, 4}, &MallocChecker::preGetdelim},
+ // NOTE: the following CallDescription also matches the C++ standard
+ // library function std::getline(); the callback will filter it out.
+ {{CDM::CLibrary, {"getline"}, 3}, &MallocChecker::preGetdelim},
+ {{CDM::CLibrary, {"getdelim"}, 4}, &MallocChecker::preGetdelim},
};
const CallDescriptionMap<CheckFn> FreeingMemFnMap{
@@ -446,8 +448,11 @@ class MallocChecker
std::bind(&MallocChecker::checkRealloc, _1, _2, _3, false)},
{{{"g_realloc_n"}, 3}, &MallocChecker::checkReallocN},
{{{"g_try_realloc_n"}, 3}, &MallocChecker::checkReallocN},
- {{{"getline"}, 3}, &MallocChecker::checkGetdelim},
- {{{"getdelim"}, 4}, &MallocChecker::checkGetdelim},
+
+ // NOTE: the following CallDescription also matches the C++ standard
+ // library function std::getline(); the callback will filter it out.
+ {{CDM::CLibrary, {"getline"}, 3}, &MallocChecker::checkGetdelim},
+ {{CDM::CLibrary, {"getdelim"}, 4}, &MallocChecker::checkGetdelim},
};
bool isMemCall(const CallEvent &Call) const;
@@ -1435,9 +1440,17 @@ void MallocChecker::checkGMallocN0(const CallEvent &Call,
C.addTransition(State);
}
+static bool isFromStdNamespace(const CallEvent &Call) {
+ const Decl *FD = Call.getDecl();
+ assert(FD && "a CallDescription cannot match a call without a Decl");
+ return FD->isInStdNamespace();
+}
+
void MallocChecker::preGetdelim(const CallEvent &Call,
CheckerContext &C) const {
- if (!Call.isGlobalCFunction())
+ // Discard calls to the C++ standard library function std::getline(), which
+ // is completely unrelated to the POSIX getline() that we're checking.
+ if (isFromStdNamespace(Call))
return;
ProgramStateRef State = C.getState();
@@ -1458,7 +1471,9 @@ void MallocChecker::preGetdelim(const CallEvent &Call,
void MallocChecker::checkGetdelim(const CallEvent &Call,
CheckerContext &C) const {
- if (!Call.isGlobalCFunction())
+ // Discard calls to the C++ standard library function std::getline(), which
+ // is completely unrelated to the POSIX getline() that we're checking.
+ if (isFromStdNamespace(Call))
return;
ProgramStateRef State = C.getState();
diff --git a/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp b/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
index d6d4cec9dd3d4d..1a9bff529e9bb1 100644
--- a/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CheckerContext.cpp
@@ -87,9 +87,11 @@ bool CheckerContext::isCLibraryFunction(const FunctionDecl *FD,
if (!II)
return false;
- // Look through 'extern "C"' and anything similar invented in the future.
- // If this function is not in TU directly, it is not a C library function.
- if (!FD->getDeclContext()->getRedeclContext()->isTranslationUnit())
+ // C library functions are either declared directly within a TU (the common
+ // case) or they are accessed through the namespace `std` (when they are used
+ // in C++ via headers like <cstdlib>).
+ const DeclContext *DC = FD->getDeclContext()->getRedeclContext();
+ if (!(DC->isTranslationUnit() || DC->isStdNamespace()))
return false;
// If this function is not externally visible, it is not a C library function.
diff --git a/clang/test/Analysis/Inputs/system-header-simulator-cxx.h b/clang/test/Analysis/Inputs/system-header-simulator-cxx.h
index 3ef7af2ea6c6ab..85db68d41a6c80 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -1106,11 +1106,20 @@ using ostream = basic_ostream<char>;
extern std::ostream cout;
ostream &operator<<(ostream &, const string &);
-
#if __cplusplus >= 202002L
template <class T>
ostream &operator<<(ostream &, const std::unique_ptr<T> &);
#endif
+
+template <class CharT>
+class basic_istream;
+
+using istream = basic_istream<char>;
+
+extern std::istream cin;
+
+istream &getline(istream &, string &, char);
+istream &getline(istream &, string &);
} // namespace std
#ifdef TEST_INLINABLE_ALLOCATORS
diff --git a/clang/test/Analysis/getline-cpp.cpp b/clang/test/Analysis/getline-cpp.cpp
new file mode 100644
index 00000000000000..ef9d3186009c7f
--- /dev/null
+++ b/clang/test/Analysis/getline-cpp.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,debug.ExprInspection -verify %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix,alpha.unix,debug.ExprInspection -verify %s
+//
+// expected-no-diagnostics
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void test_std_getline() {
+ std::string userid, comment;
+ // MallocChecker should not confuse the POSIX function getline() and the
+ // unrelated C++ standard library function std::getline.
+ std::getline(std::cin, userid, ' '); // no-crash
+ std::getline(std::cin, comment); // no-crash
+}
diff --git a/clang/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt
index 519be36fe0fa38..ff34d5747cc81b 100644
--- a/clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ b/clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -11,6 +11,7 @@ add_clang_unittest(StaticAnalysisTests
CallEventTest.cpp
ConflictingEvalCallsTest.cpp
FalsePositiveRefutationBRVisitorTest.cpp
+ IsCLibraryFunctionTest.cpp
MemRegionDescriptiveNameTest.cpp
NoStateChangeFuncVisitorTest.cpp
ParamRegionTest.cpp
diff --git a/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp b/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
new file mode 100644
index 00000000000000..d6dfcaac6f3bdc
--- /dev/null
+++ b/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
@@ -0,0 +1,84 @@
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/AnalysisDeclContext.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+#include <memory>
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+class IsCLibraryFunctionTest : public testing::Test {
+ std::unique_ptr<ASTUnit> ASTUnitP;
+ const FunctionDecl *Result = nullptr;
+
+public:
+ const FunctionDecl *getFunctionDecl() const { return Result; }
+
+ testing::AssertionResult buildAST(StringRef Code) {
+ ASTUnitP = tooling::buildASTFromCode(Code);
+ if (!ASTUnitP)
+ return testing::AssertionFailure() << "AST construction failed";
+
+ ASTContext &Context = ASTUnitP->getASTContext();
+ if (Context.getDiagnostics().hasErrorOccurred())
+ return testing::AssertionFailure() << "Compilation error";
+
+ auto Matches = ast_matchers::match(functionDecl().bind("fn"), Context);
+ if (Matches.empty())
+ return testing::AssertionFailure() << "No function declaration found";
+
+ if (Matches.size() > 1)
+ return testing::AssertionFailure()
+ << "Multiple function declarations found";
+
+ Result = Matches[0].getNodeAs<FunctionDecl>("fn");
+ return testing::AssertionSuccess();
+ }
+};
+
+TEST_F(IsCLibraryFunctionTest, AcceptsGlobal) {
+ ASSERT_TRUE(buildAST(R"cpp(void fun();)cpp"));
+ EXPECT_TRUE(CheckerContext::isCLibraryFunction(getFunctionDecl()));
+}
+
+TEST_F(IsCLibraryFunctionTest, AcceptsExternCGlobal) {
+ ASSERT_TRUE(buildAST(R"cpp(extern "C" { void fun(); })cpp"));
+ EXPECT_TRUE(CheckerContext::isCLibraryFunction(getFunctionDecl()));
+}
+
+TEST_F(IsCLibraryFunctionTest, RejectsNoInlineNoExternalLinkage) {
+ // Functions that are neither inlined nor externally visible cannot be C
+ // library functions.
+ ASSERT_TRUE(buildAST(R"cpp(static void fun();)cpp"));
+ EXPECT_FALSE(CheckerContext::isCLibraryFunction(getFunctionDecl()));
+}
+
+TEST_F(IsCLibraryFunctionTest, RejectsAnonymousNamespace) {
+ ASSERT_TRUE(buildAST(R"cpp(namespace { void fun(); })cpp"));
+ EXPECT_FALSE(CheckerContext::isCLibraryFunction(getFunctionDecl()));
+}
+
+TEST_F(IsCLibraryFunctionTest, AcceptsStdNamespace) {
+ ASSERT_TRUE(buildAST(R"cpp(namespace std { void fun(); })cpp"));
+ EXPECT_TRUE(CheckerContext::isCLibraryFunction(getFunctionDecl()));
+}
+
+TEST_F(IsCLibraryFunctionTest, RejectsOtherNamespaces) {
+ ASSERT_TRUE(buildAST(R"cpp(namespace stdx { void fun(); })cpp"));
+ EXPECT_FALSE(CheckerContext::isCLibraryFunction(getFunctionDecl()));
+}
+
+TEST_F(IsCLibraryFunctionTest, RejectsClassStatic) {
+ ASSERT_TRUE(buildAST(R"cpp(class A { static void fun(); };)cpp"));
+ EXPECT_FALSE(CheckerContext::isCLibraryFunction(getFunctionDecl()));
+}
+
+TEST_F(IsCLibraryFunctionTest, RejectsClassMember) {
+ ASSERT_TRUE(buildAST(R"cpp(class A { void fun(); };)cpp"));
+ EXPECT_FALSE(CheckerContext::isCLibraryFunction(getFunctionDecl()));
+}
diff --git a/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn b/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn
index 9230ac79a48ae3..adaf6c476dfecc 100644
--- a/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn
+++ b/llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn
@@ -19,6 +19,7 @@ unittest("StaticAnalysisTests") {
"CallEventTest.cpp",
"ConflictingEvalCallsTest.cpp",
"FalsePositiveRefutationBRVisitorTest.cpp",
+ "IsCLibraryFunctionTest.cpp",
"MemRegionDescriptiveNameTest.cpp",
"NoStateChangeFuncVisitorTest.cpp",
"ParamRegionTest.cpp",
More information about the cfe-commits
mailing list