Merge pull request github#14058 from alexet/delete-or-delete-array

CPP: Add parent class for delete and delete[]

Author: MathiasVP

cpp/ql/lib/change-notes/2023-08-25-delete-or-delete-array.md

@@ -0,0 +1,4 @@
+---
+category: feature
+---
+* Added `DeleteOrDeleteArrayExpr` as a super type of `DeleteExpr` and `DeleteArrayExpr`

cpp/ql/lib/change-notes/2023-08-25-getAllocatorCall-deprecated.md

@@ -0,0 +1,4 @@
+---
+category: deprecated
+---
+* `getAllocatorCall` on `DeleteExpr` and `DeleteArrayExpr` has been deprecated. `getDeallocatorCall` should be used instead.

cpp/ql/lib/semmle/code/cpp/PrintAST.qll

@@ -826,17 +826,11 @@ private predicate namedExprChildPredicates(Expr expr, Element ele, string pred)
     or
     expr.(Conversion).getExpr() = ele and pred = "getExpr()"
     or
-    expr.(DeleteArrayExpr).getAllocatorCall() = ele and pred = "getAllocatorCall()"
+    expr.(DeleteOrDeleteArrayExpr).getDeallocatorCall() = ele and pred = "getDeallocatorCall()"
     or
-    expr.(DeleteArrayExpr).getDestructorCall() = ele and pred = "getDestructorCall()"
+    expr.(DeleteOrDeleteArrayExpr).getDestructorCall() = ele and pred = "getDestructorCall()"
     or
-    expr.(DeleteArrayExpr).getExpr() = ele and pred = "getExpr()"
-    or
-    expr.(DeleteExpr).getAllocatorCall() = ele and pred = "getAllocatorCall()"
-    or
-    expr.(DeleteExpr).getDestructorCall() = ele and pred = "getDestructorCall()"
-    or
-    expr.(DeleteExpr).getExpr() = ele and pred = "getExpr()"
+    expr.(DeleteOrDeleteArrayExpr).getExpr() = ele and pred = "getExpr()"
     or
     expr.(DestructorFieldDestruction).getExpr() = ele and pred = "getExpr()"
     or

cpp/ql/lib/semmle/code/cpp/controlflow/internal/CFG.qll

@@ -332,21 +332,12 @@ private Node getControlOrderChildSparse(Node n, int i) {
   n = any(ConditionDeclExpr cd | i = 0 and result = cd.getInitializingExpr())
   or
   n =
-    any(DeleteExpr del |
+    any(DeleteOrDeleteArrayExpr del |
       i = 0 and result = del.getExpr()
       or
       i = 1 and result = del.getDestructorCall()
       or
-      i = 2 and result = del.getAllocatorCall()
-    )
-  or
-  n =
-    any(DeleteArrayExpr del |
-      i = 0 and result = del.getExpr()
-      or
-      i = 1 and result = del.getDestructorCall()
-      or
-      i = 2 and result = del.getAllocatorCall()
+      i = 2 and result = del.getDeallocatorCall()
     )
   or
   n =

cpp/ql/lib/semmle/code/cpp/exprs/Expr.qll

@@ -932,52 +932,50 @@ class NewArrayExpr extends NewOrNewArrayExpr, @new_array_expr {
   Expr getExtent() { result = this.getChild(2) }
 }
 
+private class TDeleteOrDeleteArrayExpr = @delete_expr or @delete_array_expr;
+
 /**
- * A C++ `delete` (non-array) expression.
- * ```
- * delete ptr;
- * ```
+ * A C++ `delete` or `delete[]` expression.
  */
-class DeleteExpr extends Expr, @delete_expr {
-  override string toString() { result = "delete" }
-
-  override string getAPrimaryQlClass() { result = "DeleteExpr" }
-
+class DeleteOrDeleteArrayExpr extends Expr, TDeleteOrDeleteArrayExpr {
   override int getPrecedence() { result = 16 }
 
-  /**
-   * Gets the compile-time type of the object being deleted.
-   */
-  Type getDeletedObjectType() {
-    result =
-      this.getExpr()
-          .getFullyConverted()
-          .getType()
-          .stripTopLevelSpecifiers()
-          .(PointerType)
-          .getBaseType()
-  }
-
   /**
    * Gets the call to a destructor that occurs prior to the object's memory being deallocated, if any.
+   *
+   * In the case of `delete[]` at runtime, the destructor will be called once for each element in the array, but the
+   * destructor call only exists once in the AST.
    */
   DestructorCall getDestructorCall() { result = this.getChild(1) }
 
   /**
-   * Gets the destructor to be called to destroy the object, if any.
+   * Gets the destructor to be called to destroy the object or array, if any.
    */
   Destructor getDestructor() { result = this.getDestructorCall().getTarget() }
 
   /**
-   * Gets the `operator delete` that deallocates storage. Does not hold
-   * if the type being destroyed has a virtual destructor. In that case, the
+   * Gets the `operator delete` or `operator delete[]` that deallocates storage.
+   * Does not hold if the type being destroyed has a virtual destructor. In that case, the
    * `operator delete` that will be called is determined at runtime based on the
    * dynamic type of the object.
    */
   Function getDeallocator() {
     expr_deallocator(underlyingElement(this), unresolveElement(result), _)
   }
 
+  /**
+   * DEPRECATED: use `getDeallocatorCall` instead.
+   */
+  deprecated FunctionCall getAllocatorCall() { result = this.getChild(0) }
+
+  /**
+   * Gets the call to a non-default `operator delete`/`delete[]` that deallocates storage, if any.
+   *
+   * This will only be present when the type being deleted has a custom `operator delete` and
+   * does not have a virtual destructor.
+   */
+  FunctionCall getDeallocatorCall() { result = this.getChild(0) }
+
   /**
    * Holds if the deallocation function expects a size argument.
    */
@@ -999,16 +997,38 @@ class DeleteExpr extends Expr, @delete_expr {
   }
 
   /**
-   * Gets the call to a non-default `operator delete` that deallocates storage, if any.
-   *
-   * This will only be present when the type being deleted has a custom `operator delete`.
+   * Gets the object or array being deleted.
    */
-  FunctionCall getAllocatorCall() { result = this.getChild(0) }
+  Expr getExpr() {
+    // If there is a destructor call, the object being deleted is the qualifier
+    // otherwise it is the third child.
+    result = this.getChild(3) or result = this.getDestructorCall().getQualifier()
+  }
+}
+
+/**
+ * A C++ `delete` (non-array) expression.
+ * ```
+ * delete ptr;
+ * ```
+ */
+class DeleteExpr extends DeleteOrDeleteArrayExpr, @delete_expr {
+  override string toString() { result = "delete" }
+
+  override string getAPrimaryQlClass() { result = "DeleteExpr" }
 
   /**
-   * Gets the object being deleted.
+   * Gets the compile-time type of the object being deleted.
    */
-  Expr getExpr() { result = this.getChild(3) or result = this.getChild(1).getChild(-1) }
+  Type getDeletedObjectType() {
+    result =
+      this.getExpr()
+          .getFullyConverted()
+          .getType()
+          .stripTopLevelSpecifiers()
+          .(PointerType)
+          .getBaseType()
+  }
 }
 
 /**
@@ -1017,13 +1037,11 @@ class DeleteExpr extends Expr, @delete_expr {
  * delete[] arr;
  * ```
  */
-class DeleteArrayExpr extends Expr, @delete_array_expr {
+class DeleteArrayExpr extends DeleteOrDeleteArrayExpr, @delete_array_expr {
   override string toString() { result = "delete[]" }
 
   override string getAPrimaryQlClass() { result = "DeleteArrayExpr" }
 
-  override int getPrecedence() { result = 16 }
-
   /**
    * Gets the element type of the array being deleted.
    */
@@ -1036,58 +1054,6 @@ class DeleteArrayExpr extends Expr, @delete_array_expr {
           .(PointerType)
           .getBaseType()
   }
-
-  /**
-   * Gets the call to a destructor that occurs prior to the array's memory being deallocated, if any.
-   *
-   * At runtime, the destructor will be called once for each element in the array, but the
-   * destructor call only exists once in the AST.
-   */
-  DestructorCall getDestructorCall() { result = this.getChild(1) }
-
-  /**
-   * Gets the destructor to be called to destroy each element in the array, if any.
-   */
-  Destructor getDestructor() { result = this.getDestructorCall().getTarget() }
-
-  /**
-   * Gets the `operator delete[]` that deallocates storage.
-   */
-  Function getDeallocator() {
-    expr_deallocator(underlyingElement(this), unresolveElement(result), _)
-  }
-
-  /**
-   * Holds if the deallocation function expects a size argument.
-   */
-  predicate hasSizedDeallocation() {
-    exists(int form |
-      expr_deallocator(underlyingElement(this), _, form) and
-      form.bitAnd(1) != 0 // Bit zero is the "size" bit
-    )
-  }
-
-  /**
-   * Holds if the deallocation function expects an alignment argument.
-   */
-  predicate hasAlignedDeallocation() {
-    exists(int form |
-      expr_deallocator(underlyingElement(this), _, form) and
-      form.bitAnd(2) != 0 // Bit one is the "alignment" bit
-    )
-  }
-
-  /**
-   * Gets the call to a non-default `operator delete` that deallocates storage, if any.
-   *
-   * This will only be present when the type being deleted has a custom `operator delete`.
-   */
-  FunctionCall getAllocatorCall() { result = this.getChild(0) }
-
-  /**
-   * Gets the array being deleted.
-   */
-  Expr getExpr() { result = this.getChild(3) or result = this.getChild(1).getChild(-1) }
 }
 
 /**

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll

@@ -84,9 +84,9 @@ private predicate ignoreExprAndDescendants(Expr expr) {
   or
   // We do not yet translate destructors properly, so for now we ignore any
   // custom deallocator call, if present.
-  exists(DeleteExpr deleteExpr | deleteExpr.getAllocatorCall() = expr)
+  exists(DeleteExpr deleteExpr | deleteExpr.getDeallocatorCall() = expr)
   or
-  exists(DeleteArrayExpr deleteArrayExpr | deleteArrayExpr.getAllocatorCall() = expr)
+  exists(DeleteArrayExpr deleteArrayExpr | deleteArrayExpr.getDeallocatorCall() = expr)
   or
   exists(BuiltInVarArgsStart vaStartExpr |
     vaStartExpr.getLastNamedParameter().getFullyConverted() = expr

cpp/ql/src/Security/CWE/CWE-570/IncorrectAllocationErrorHandling.ql

@@ -17,21 +17,6 @@ import cpp
 import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import semmle.code.cpp.controlflow.Guards
 
-/**
- * A C++ `delete` or `delete[]` expression.
- */
-class DeleteOrDeleteArrayExpr extends Expr {
-  DeleteOrDeleteArrayExpr() { this instanceof DeleteExpr or this instanceof DeleteArrayExpr }
-
-  DeallocationFunction getDeallocator() {
-    result = [this.(DeleteExpr).getDeallocator(), this.(DeleteArrayExpr).getDeallocator()]
-  }
-
-  Destructor getDestructor() {
-    result = [this.(DeleteExpr).getDestructor(), this.(DeleteArrayExpr).getDestructor()]
-  }
-}
-
 /** Gets the `Constructor` invoked when `newExpr` allocates memory. */
 Constructor getConstructorForAllocation(NewOrNewArrayExpr newExpr) {
   result.getACallToThisFunction() = newExpr.getInitializer()

cpp/ql/test/library-tests/allocators/allocators.expected

@@ -34,11 +34,11 @@ newArrayExprDeallocators
 | allocators.cpp:108:3:108:19 | new[] | FailedInit | void FailedInit::operator delete[](void*, size_t) | 1 | 1 | sized  |
 | allocators.cpp:110:3:110:37 | new[] | FailedInitOveraligned | void FailedInitOveraligned::operator delete[](void*, std::align_val_t, float) | 128 | 128 |  aligned |
 deleteExprs
-| allocators.cpp:59:3:59:35 | delete | int | void operator delete(void*, unsigned long) | 4 | 4 | sized  |
-| allocators.cpp:60:3:60:38 | delete | String | void operator delete(void*, unsigned long) | 8 | 8 | sized  |
-| allocators.cpp:61:3:61:44 | delete | SizedDealloc | void SizedDealloc::operator delete(void*, size_t) | 32 | 1 | sized  |
-| allocators.cpp:62:3:62:43 | delete | Overaligned | void operator delete(void*, unsigned long, std::align_val_t) | 256 | 128 | sized aligned |
-| allocators.cpp:64:3:64:44 | delete | const String | void operator delete(void*, unsigned long) | 8 | 8 | sized  |
+| allocators.cpp:59:3:59:35 | delete | int | void operator delete(void*, unsigned long) | 4 | 4 | sized  | false |
+| allocators.cpp:60:3:60:38 | delete | String | void operator delete(void*, unsigned long) | 8 | 8 | sized  | false |
+| allocators.cpp:61:3:61:44 | delete | SizedDealloc | void SizedDealloc::operator delete(void*, size_t) | 32 | 1 | sized  | true |
+| allocators.cpp:62:3:62:43 | delete | Overaligned | void operator delete(void*, unsigned long, std::align_val_t) | 256 | 128 | sized aligned | false |
+| allocators.cpp:64:3:64:44 | delete | const String | void operator delete(void*, unsigned long) | 8 | 8 | sized  | false |
 deleteArrayExprs
 | allocators.cpp:78:3:78:37 | delete[] | int | void operator delete[](void*, unsigned long) | 4 | 4 | sized  |
 | allocators.cpp:79:3:79:40 | delete[] | String | void operator delete[](void*, unsigned long) | 8 | 8 | sized  |

cpp/ql/test/library-tests/allocators/allocators.ql

@@ -77,7 +77,8 @@ query predicate newArrayExprDeallocators(
 }
 
 query predicate deleteExprs(
-  DeleteExpr expr, string type, string sig, int size, int alignment, string form
+  DeleteExpr expr, string type, string sig, int size, int alignment, string form,
+  boolean hasDeallocatorCall
 ) {
   exists(Function deallocator, Type deletedType |
     expr.getDeallocator() = deallocator and
@@ -90,7 +91,10 @@ query predicate deleteExprs(
       (if expr.hasAlignedDeallocation() then aligned = "aligned" else aligned = "") and
       (if expr.hasSizedDeallocation() then sized = "sized" else sized = "") and
       form = sized + " " + aligned
-    )
+    ) and
+    if exists(expr.getDeallocatorCall())
+    then hasDeallocatorCall = true
+    else hasDeallocatorCall = false
   )
 }
 

cpp/ql/test/library-tests/ir/ir/PrintAST.expected

@@ -8477,7 +8477,7 @@ ir.cpp:
 # 1018|       getExpr(): [DeleteExpr] delete
 # 1018|           Type = [VoidType] void
 # 1018|           ValueCategory = prvalue
-# 1018|         getAllocatorCall(): [FunctionCall] call to operator delete
+# 1018|         getDeallocatorCall(): [FunctionCall] call to operator delete
 # 1018|             Type = [VoidType] void
 # 1018|             ValueCategory = prvalue
 # 1018|         getExpr(): [Literal] 0
@@ -8555,7 +8555,7 @@ ir.cpp:
 # 1027|       getExpr(): [DeleteArrayExpr] delete[]
 # 1027|           Type = [VoidType] void
 # 1027|           ValueCategory = prvalue
-# 1027|         getAllocatorCall(): [FunctionCall] call to operator delete[]
+# 1027|         getDeallocatorCall(): [FunctionCall] call to operator delete[]
 # 1027|             Type = [VoidType] void
 # 1027|             ValueCategory = prvalue
 # 1027|         getExpr(): [Literal] 0