[clang][analyzer] Bring cplusplus.ArrayDelete out of alpha #83985

Discookie · 2024-03-05T09:47:20Z

The checker finds a type of undefined behavior, where if the type of a pointer to an object-array is different from the objects' underlying type, calling delete[] is undefined, as the size of the two objects might be different.

The checker has been in alpha for a while now, it is a simple checker that causes no crashes, and considering the severity of the issue, it has a low result-count on open-source projects (in my last test-run on my usual projects, it had 0 results).

This commit cleans up the documentation and adds docs for the limitation related to tracking through references, in addition to moving it to cplusplus.

llvmbot · 2024-03-05T09:47:49Z

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

@llvm/pr-subscribers-clang

Author: Discookie (Discookie)

Changes

The checker finds a type of undefined behavior, where if the type of a pointer to an object-array is different from the objects' underlying type, calling delete[] is undefined, as the size of the two objects might be different.

The checker has been in alpha for a while now, it is a simple checker that causes no crashes, and considering the severity of the issue, it has a low result-count on open-source projects.

This commit cleans up the documentation and adds docs for the limitation related to tracking through references, in addition to moving it to cplusplus.

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

6 Files Affected:

(modified) clang/docs/analyzer/checkers.rst (+44-24)
(modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+5-5)
(modified) clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp (+2-2)
(modified) clang/test/Analysis/ArrayDelete.cpp (+1-1)
(modified) clang/www/analyzer/alpha_checks.html (-20)
(modified) clang/www/analyzer/available_checks.html (+27)

diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index fe211514914272..fc0c90f3f5d4e5 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -340,6 +340,50 @@ cplusplus
 
 C++ Checkers.
 
+.. _cplusplus-ArrayDelete:
+
+cplusplus.ArrayDelete (C++)
+"""""""""""""""""""""""""""
+Reports destructions of arrays of polymorphic objects that are destructed as
+their base class. If the dynamic type of the array's object is different from
+its static type, calling `delete[]` is undefined.
+
+This checker corresponds to the CERT rule `EXP51-CPP: Do not delete an array through a pointer of the incorrect type <https://wiki.sei.cmu.edu/confluence/display/cplusplus/EXP51-CPP.+Do+not+delete+an+array+through+a+pointer+of+the+incorrect+type>`_.
+
+.. code-block:: cpp
+
+ class Base {
+ public:
+   virtual ~Base() {}
+ };
+ class Derived : public Base {};
+
+ Base *create() {
+   Base *x = new Derived[10]; // note: Casting from 'Derived' to 'Base' here
+   return x;
+ }
+
+ void foo() {
+   Base *x = create();
+   delete[] x; // warn: Deleting an array of 'Derived' objects as their base class 'Base' is undefined
+ }
+
+**Limitations**
+
+The checker does not emit note tags when casting to and from reference types,
+even though the pointer values are tracked across references.
+
+.. code-block:: cpp
+
+ void foo() {
+   Derived *d = new Derived[10];
+   Derived &dref = *d;
+
+   Base &bref = static_cast<Base&>(dref); // no note
+   Base *b = &bref;
+   delete[] b; // warn: Deleting an array of 'Derived' objects as their base class 'Base' is undefined
+ }
+
 .. _cplusplus-InnerPointer:
 
 cplusplus.InnerPointer (C++)
@@ -2139,30 +2183,6 @@ Either the comparison is useless or there is division by zero.
 alpha.cplusplus
 ^^^^^^^^^^^^^^^
 
-.. _alpha-cplusplus-ArrayDelete:
-
-alpha.cplusplus.ArrayDelete (C++)
-"""""""""""""""""""""""""""""""""
-Reports destructions of arrays of polymorphic objects that are destructed as their base class.
-This checker corresponds to the CERT rule `EXP51-CPP: Do not delete an array through a pointer of the incorrect type <https://wiki.sei.cmu.edu/confluence/display/cplusplus/EXP51-CPP.+Do+not+delete+an+array+through+a+pointer+of+the+incorrect+type>`_.
-
-.. code-block:: cpp
-
- class Base {
-   virtual ~Base() {}
- };
- class Derived : public Base {}
-
- Base *create() {
-   Base *x = new Derived[10]; // note: Casting from 'Derived' to 'Base' here
-   return x;
- }
-
- void foo() {
-   Base *x = create();
-   delete[] x; // warn: Deleting an array of 'Derived' objects as their base class 'Base' is undefined
- }
-
 .. _alpha-cplusplus-DeleteWithNonVirtualDtor:
 
 alpha.cplusplus.DeleteWithNonVirtualDtor (C++)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index a224b81c33a624..97fa8ac060eafa 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -622,6 +622,11 @@ def BlockInCriticalSectionChecker : Checker<"BlockInCriticalSection">,
 
 let ParentPackage = Cplusplus in {
 
+def ArrayDeleteChecker : Checker<"ArrayDelete">,
+  HelpText<"Reports destructions of arrays of polymorphic objects that are "
+           "destructed as their base class.">,
+  Documentation<HasDocumentation>;
+
 def InnerPointerChecker : Checker<"InnerPointer">,
   HelpText<"Check for inner pointers of C++ containers used after "
            "re/deallocation">,
@@ -777,11 +782,6 @@ def ContainerModeling : Checker<"ContainerModeling">,
   Documentation<NotDocumented>,
   Hidden;
 
-def CXXArrayDeleteChecker : Checker<"ArrayDelete">,
-  HelpText<"Reports destructions of arrays of polymorphic objects that are "
-           "destructed as their base class.">,
-  Documentation<HasDocumentation>;
-
 def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">,
   HelpText<"Reports destructions of polymorphic objects with a non-virtual "
            "destructor in their base class">,
diff --git a/clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp
index b4dee1e300e886..1b1226a7f1a71d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp
@@ -220,11 +220,11 @@ CXXDeleteChecker::PtrCastVisitor::VisitNode(const ExplodedNode *N,
                                                     /*addPosRange=*/true);
 }
 
-void ento::registerCXXArrayDeleteChecker(CheckerManager &mgr) {
+void ento::registerArrayDeleteChecker(CheckerManager &mgr) {
   mgr.registerChecker<CXXArrayDeleteChecker>();
 }
 
-bool ento::shouldRegisterCXXArrayDeleteChecker(const CheckerManager &mgr) {
+bool ento::shouldRegisterArrayDeleteChecker(const CheckerManager &mgr) {
   return true;
 }
 
diff --git a/clang/test/Analysis/ArrayDelete.cpp b/clang/test/Analysis/ArrayDelete.cpp
index 3b8d49552376ed..6887e0a35fb8bd 100644
--- a/clang/test/Analysis/ArrayDelete.cpp
+++ b/clang/test/Analysis/ArrayDelete.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.ArrayDelete -std=c++11 -verify -analyzer-output=text %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=cplusplus.ArrayDelete -std=c++11 -verify -analyzer-output=text %s
 
 struct Base {
     virtual ~Base() = default;
diff --git a/clang/www/analyzer/alpha_checks.html b/clang/www/analyzer/alpha_checks.html
index 7bbe4a20288f23..f040d1957b0f98 100644
--- a/clang/www/analyzer/alpha_checks.html
+++ b/clang/www/analyzer/alpha_checks.html
@@ -307,26 +307,6 @@ <h3 id="cplusplus_alpha_checkers">C++ Alpha Checkers</h3>
 <tbody>
 
 
-<tr><td><a id="alpha.cplusplus.ArrayDelete"><div class="namedescr expandable"><span class="name">
-alpha.cplusplus.ArrayDelete</span><span class="lang">
-(C++)</span><div class="descr">
-Reports destructions of arrays of polymorphic objects that are destructed as
-their base class
-</div></div></a></td>
-<td><div class="exampleContainer expandable">
-<div class="example"><pre>
-Base *create() {
-  Base *x = new Derived[10]; // note: Casting from 'Derived' to 'Base' here
-  return x;
-}
-
-void sink(Base *x) {
-  delete[] x; // warn: Deleting an array of 'Derived' objects as their base class 'Base' undefined
-}
-
-</pre></div></div></td></tr>
-
-
 <tr><td><a id="alpha.cplusplus.DeleteWithNonVirtualDtor"><div class="namedescr expandable"><span class="name">
 alpha.cplusplus.DeleteWithNonVirtualDtor</span><span class="lang">
 (C++)</span><div class="descr">
diff --git a/clang/www/analyzer/available_checks.html b/clang/www/analyzer/available_checks.html
index 501a2b306dfadd..c23865e57e87d3 100644
--- a/clang/www/analyzer/available_checks.html
+++ b/clang/www/analyzer/available_checks.html
@@ -369,6 +369,33 @@ <h3 id="cplusplus_checkers">C++ Checkers</h3>
 <thead><tr><td>Name, Description</td><td>Example</td></tr></thead>
 
 <tbody>
+
+<tr><td><a id="cplusplus.ArrayDelete"><div class="namedescr expandable"><span class="name">
+cplusplus.ArrayDelete</span><span class="lang">
+(C++)</span><div class="descr">
+Reports destructions of arrays of polymorphic objects that are destructed as
+their base class. If the type of an array-pointer is different from the type of
+its underlying objects, calling <code>delete[]</code> is undefined.
+</div></div></a></td>
+<td><div class="exampleContainer expandable">
+<div class="example"><pre>
+class Base {
+public:
+  virtual ~Base() {}
+};
+class Derived : public Base {};
+
+Base *create() {
+  Base *x = new Derived[10]; // note: Casting from 'Derived' to 'Base' here
+  return x;
+}
+
+void sink(Base *x) {
+  delete[] x; // warn: Deleting an array of 'Derived' objects as their base class 'Base' undefined
+}
+</pre></div></div></td></tr>
+
+
 <tr><td><a id="cplusplus.NewDelete"><div class="namedescr expandable"><span class="name">
 cplusplus.NewDelete</span><span class="lang">
 (C++)</span><div class="descr">

NagyDonat

LGTM. I think you can merge this in a few days (if nobody requests changes).

steakhal · 2024-03-05T15:09:19Z

I'm fine with the change.

whisperity

Style nits only.

whisperity · 2024-03-14T14:47:00Z

clang/docs/analyzer/checkers.rst

+"""""""""""""""""""""""""""
+Reports destructions of arrays of polymorphic objects that are destructed as


Suggested change

"""""""""""""""""""""""""""

Reports destructions of arrays of polymorphic objects that are destructed as

"""""""""""""""""""""""""""

Reports destructions of arrays of polymorphic objects that are destructed as

Style nit.

whisperity · 2024-03-14T14:47:40Z

clang/docs/analyzer/checkers.rst

+their base class. If the dynamic type of the array's object is different from
+its static type, calling `delete[]` is undefined.


Suggested change

their base class. If the dynamic type of the array's object is different from

its static type, calling `delete[]` is undefined.

their base class. If the dynamic type of the array is different from

its static type, calling `delete[]` is undefined.

clang/docs/analyzer/checkers.rst

whisperity · 2024-03-14T14:50:13Z

clang/docs/analyzer/checkers.rst

+   delete[] x; // warn: Deleting an array of 'Derived' objects as their base class 'Base' is undefined
+ }
+
+**Limitations**


(Shouldn't this be a sub-heading?)

No, this is the formatting that's consistently used in this document.

whisperity · 2024-03-14T14:50:55Z

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

@@ -220,11 +220,11 @@ CXXDeleteChecker::PtrCastVisitor::VisitNode(const ExplodedNode *N,
                                                    /*addPosRange=*/true);
 }

-void ento::registerCXXArrayDeleteChecker(CheckerManager &mgr) {
+void ento::registerArrayDeleteChecker(CheckerManager &mgr) {


Why was the CXX removed here if the checker is still named cplusplus.ArrayDelete.

Brings it more in line with similar C++ checkers, see eg. DeleteWithNonVirtualDtorChecker in the same file.

The only place where this name matters is the Checkers.td where it is under the cplusplus group anyways.

NagyDonat

Still LGTM, apply the few suggested changes and then feel free to merge the commit.

steakhal · 2024-03-21T17:29:03Z

Retriggered the premerge checks.

Co-authored-by: whisperity <[email protected]>

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

Discookie requested a review from NagyDonat March 5, 2024 09:48

NagyDonat reviewed Mar 5, 2024

View reviewed changes

whisperity reviewed Mar 14, 2024

View reviewed changes

NagyDonat approved these changes Mar 14, 2024

View reviewed changes

[clang][analyzer] Bring cplusplus.ArrayDelete out of alpha

881701d

Discookie force-pushed the ericsson/array-delete-dealpha branch from 28ed5ca to 881701d Compare March 20, 2024 10:13

Merge branch 'main' into ericsson/array-delete-dealpha

5df8974

Style nit fix

aa345ea

Co-authored-by: whisperity <[email protected]>

Discookie merged commit 37785fe into llvm:main Mar 25, 2024

whisperity mentioned this pull request Mar 27, 2024

[config] Label Clang 18.0-19.trunk SA & Tidy checkers Ericsson/codechecker#4193

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][analyzer] Bring cplusplus.ArrayDelete out of alpha #83985

[clang][analyzer] Bring cplusplus.ArrayDelete out of alpha #83985

Uh oh!

Discookie commented Mar 5, 2024 •

edited by whisperity

Loading

Uh oh!

llvmbot commented Mar 5, 2024 •

edited

Loading

Uh oh!

NagyDonat left a comment

Uh oh!

steakhal commented Mar 5, 2024

Uh oh!

whisperity left a comment

Uh oh!

whisperity Mar 14, 2024

Uh oh!

whisperity Mar 14, 2024

Uh oh!

Uh oh!

whisperity Mar 14, 2024

Uh oh!

NagyDonat Mar 14, 2024

Uh oh!

whisperity Mar 14, 2024

Uh oh!

Discookie Mar 14, 2024

Uh oh!

NagyDonat left a comment

Uh oh!

steakhal commented Mar 21, 2024

Uh oh!

Uh oh!

		"""""""""""""""""""""""""""
		Reports destructions of arrays of polymorphic objects that are destructed as

		their base class. If the dynamic type of the array's object is different from
		its static type, calling `delete[]` is undefined.

[clang][analyzer] Bring cplusplus.ArrayDelete out of alpha #83985

[clang][analyzer] Bring cplusplus.ArrayDelete out of alpha #83985

Uh oh!

Conversation

Discookie commented Mar 5, 2024 • edited by whisperity Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

NagyDonat left a comment

Choose a reason for hiding this comment

Uh oh!

steakhal commented Mar 5, 2024

Uh oh!

whisperity left a comment

Choose a reason for hiding this comment

Uh oh!

whisperity Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

whisperity Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

whisperity Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

NagyDonat Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

whisperity Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

Discookie Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

NagyDonat left a comment

Choose a reason for hiding this comment

Uh oh!

steakhal commented Mar 21, 2024

Uh oh!

Uh oh!

Discookie commented Mar 5, 2024 •

edited by whisperity

Loading

llvmbot commented Mar 5, 2024 •

edited

Loading