Reapply "[analyzer] Accept C library functions from the std namespace" again (#85791)

This reapplies 80ab823 again, after
fixing a name collision warning in the unit tests (see the revert commit
13ccaf9 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 <[email protected]>

Author: NagyDonat

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

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();

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.

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

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
+}

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

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()));
+}

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",