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

NagyDonat · 2024-03-19T14:06:35Z

This reapplies 80ab823 again, after fixing a name collision warning in the unit tests (see the revert commit 13ccaf9 for details).

…ce" again This reapplies 80ab823 again, after fixing a name collision warning in the unit tests (see the revert commit 13ccaf9 for details). Co-authored-by: Balazs Benics <[email protected]>

llvmbot · 2024-03-19T14:07:07Z

@llvm/pr-subscribers-clang-static-analyzer-1

Author: None (NagyDonat)

Changes

This reapplies 80ab823 again, after fixing a name collision warning in the unit tests (see the revert commit 13ccaf9 for details).

Full diff: https://github.com/llvm/llvm-project/pull/85791.diff

5 Files Affected:

(modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h (+2-6)
(modified) clang/lib/StaticAnalyzer/Core/CheckerContext.cpp (+5-3)
(modified) clang/unittests/StaticAnalyzer/CMakeLists.txt (+1)
(added) clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp (+82)
(modified) llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn (+1)

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/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/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt
index 775f0f8486b8f9..db56e77331b821 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
   NoStateChangeFuncVisitorTest.cpp
   ParamRegionTest.cpp
   RangeSetTest.cpp
diff --git a/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp b/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
new file mode 100644
index 00000000000000..f26acf07e23ac0
--- /dev/null
+++ b/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
@@ -0,0 +1,82 @@
+#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 01c2b6ced3366f..9c240cff181634 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",
     "NoStateChangeFuncVisitorTest.cpp",
     "ParamRegionTest.cpp",
     "RangeSetTest.cpp",

llvmbot · 2024-03-19T14:07:07Z

@llvm/pr-subscribers-clang

Author: None (NagyDonat)

Changes

This reapplies 80ab823 again, after fixing a name collision warning in the unit tests (see the revert commit 13ccaf9 for details).

Full diff: https://github.com/llvm/llvm-project/pull/85791.diff

5 Files Affected:

(modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h (+2-6)
(modified) clang/lib/StaticAnalyzer/Core/CheckerContext.cpp (+5-3)
(modified) clang/unittests/StaticAnalyzer/CMakeLists.txt (+1)
(added) clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp (+82)
(modified) llvm/utils/gn/secondary/clang/unittests/StaticAnalyzer/BUILD.gn (+1)

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/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/unittests/StaticAnalyzer/CMakeLists.txt b/clang/unittests/StaticAnalyzer/CMakeLists.txt
index 775f0f8486b8f9..db56e77331b821 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
   NoStateChangeFuncVisitorTest.cpp
   ParamRegionTest.cpp
   RangeSetTest.cpp
diff --git a/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp b/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
new file mode 100644
index 00000000000000..f26acf07e23ac0
--- /dev/null
+++ b/clang/unittests/StaticAnalyzer/IsCLibraryFunctionTest.cpp
@@ -0,0 +1,82 @@
+#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 01c2b6ced3366f..9c240cff181634 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",
     "NoStateChangeFuncVisitorTest.cpp",
     "ParamRegionTest.cpp",
     "RangeSetTest.cpp",

github-actions · 2024-03-19T14:09:16Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Szelethus

Lets hope it works fine this time around :)

NagyDonat · 2024-03-21T15:58:50Z

I'll delay reapplying this commit because I realized that it has minor unintended side effects in MallocChecker.

The name "getline" is used for two completely different functions: `std::getline` from the C++ standard library works with streams and strings, while the C function `getline` defined by POSIX and the dynamic memory TS works with C data structures and `malloc` style allocations. This commit adds a manual `isInStdNamespace()` check to discard calls to `std::getline` in MallocChecker. In theory we could keep a separate matching mode for "only a C function, not from the std namespace" but I think that situations like this `getline` are so rare that it's better to handle them individually. Note that the analyzer was apparently producing correct behavior even without this fix, because the unexpected `std::getline` calls were silently discarded (as their arguments had unexpected types and the callback just bailed out).

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

github-actions · 2024-03-25T09:41:53Z

✅ With the latest revision this PR passed the Python code formatter.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

steakhal · 2024-07-01T08:35:38Z

This fixed tickets in our backlog. Thanks for pushing for this.

Reapply "[analyzer] Accept C library functions from the std namespa…

3fbc8da

…ce" again This reapplies 80ab823 again, after fixing a name collision warning in the unit tests (see the revert commit 13ccaf9 for details). Co-authored-by: Balazs Benics <[email protected]>

llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Mar 19, 2024

NagyDonat requested review from gamesh411 and steakhal March 19, 2024 14:07

Satisfy git-clang-format

f06f093

NagyDonat requested a review from Szelethus March 20, 2024 17:29

Szelethus approved these changes Mar 21, 2024

View reviewed changes

NagyDonat requested a review from Szelethus March 21, 2024 16:30

NagyDonat commented Mar 25, 2024

View reviewed changes

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Outdated Show resolved Hide resolved

NagyDonat commented Mar 25, 2024

View reviewed changes

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Outdated Show resolved Hide resolved

NagyDonat added 2 commits March 25, 2024 10:37

Fix a mistake in comments

61bdd67

Merge branch 'main' into CallDescription_fix_unittest_again

e689c55

NagyDonat commented Mar 25, 2024

View reviewed changes

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Outdated Show resolved Hide resolved

Remove superfluous parentheses

f51641b

steakhal approved these changes Mar 25, 2024

View reviewed changes

NagyDonat merged commit e1d4ddb into llvm:main Mar 25, 2024

NagyDonat mentioned this pull request Apr 15, 2024

[analyzer] Harden security.cert.env.InvalidPtr checker fn matching #88536

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

NagyDonat commented Mar 19, 2024

Uh oh!

llvmbot commented Mar 19, 2024

Uh oh!

llvmbot commented Mar 19, 2024

Uh oh!

github-actions bot commented Mar 19, 2024 •

edited

Loading

Uh oh!

Szelethus left a comment

Uh oh!

NagyDonat commented Mar 21, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 25, 2024

Uh oh!

Uh oh!

steakhal commented Jul 1, 2024

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

NagyDonat commented Mar 19, 2024

Uh oh!

llvmbot commented Mar 19, 2024

Uh oh!

llvmbot commented Mar 19, 2024

Uh oh!

github-actions bot commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Szelethus left a comment

Choose a reason for hiding this comment

Uh oh!

NagyDonat commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 25, 2024

Uh oh!

Uh oh!

steakhal commented Jul 1, 2024

Uh oh!

Uh oh!

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

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

github-actions bot commented Mar 19, 2024 •

edited

Loading

NagyDonat commented Mar 21, 2024 •

edited

Loading