Merge pull request #5847 from MathiasVP/improve-wrong-in-detecting-and-handling-memory-allocation-errors

Improve wrong in detecting and handling memory allocation errors

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

@@ -17,7 +17,7 @@ make sure to handle the possibility of null pointers if <code>new(std::nothrow)
 
 </recommendation>
 <example>
-<sample src="WrongInDetectingAndHandlingMemoryAllocationErrors.cpp" />
+<sample src="IncorrectAllocationErrorHandling.cpp" />
 
 </example>
 <references>

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

@@ -0,0 +1,224 @@
+/**
+ * @name Incorrect allocation-error handling
+ * @description `operator new` throws an exception on allocation failures, while `operator new(std::nothrow)` returns a null pointer. Mixing up these two failure conditions can result in unexpected behavior.
+ * @kind problem
+ * @id cpp/incorrect-allocation-error-handling
+ * @problem.severity warning
+ * @precision medium
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-570
+ */
+
+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()
+}
+
+/** Gets the `Destructor` invoked when `deleteExpr` deallocates memory. */
+Destructor getDestructorForDeallocation(DeleteOrDeleteArrayExpr deleteExpr) {
+  result = deleteExpr.getDestructor()
+}
+
+/** Holds if the evaluation of `newExpr` may throw an exception. */
+predicate newMayThrow(NewOrNewArrayExpr newExpr) {
+  functionMayThrow(newExpr.getAllocator()) or
+  functionMayThrow(getConstructorForAllocation(newExpr))
+}
+
+/** Holds if the evaluation of `deleteExpr` may throw an exception. */
+predicate deleteMayThrow(DeleteOrDeleteArrayExpr deleteExpr) {
+  functionMayThrow(deleteExpr.getDeallocator()) or
+  functionMayThrow(getDestructorForDeallocation(deleteExpr))
+}
+
+/**
+ * Holds if the function may throw an exception when called. That is, if the body of the function looks
+ * like it might throw an exception, and the function does not have a `noexcept` or `throw()` specifier.
+ */
+predicate functionMayThrow(Function f) {
+  (not exists(f.getBlock()) or stmtMayThrow(f.getBlock())) and
+  not f.isNoExcept() and
+  not f.isNoThrow()
+}
+
+/** Holds if the evaluation of `stmt` may throw an exception. */
+predicate stmtMayThrow(Stmt stmt) {
+  stmtMayThrow(stmt.(BlockStmt).getAStmt())
+  or
+  convertedExprMayThrow(stmt.(ExprStmt).getExpr())
+  or
+  convertedExprMayThrow(stmt.(DeclStmt).getADeclaration().(Variable).getInitializer().getExpr())
+  or
+  exists(IfStmt ifStmt | ifStmt = stmt |
+    convertedExprMayThrow(ifStmt.getCondition()) or
+    stmtMayThrow([ifStmt.getThen(), ifStmt.getElse()])
+  )
+  or
+  exists(ConstexprIfStmt constIfStmt | constIfStmt = stmt |
+    stmtMayThrow([constIfStmt.getThen(), constIfStmt.getElse()])
+  )
+  or
+  exists(Loop loop | loop = stmt |
+    convertedExprMayThrow(loop.getCondition()) or
+    stmtMayThrow(loop.getStmt())
+  )
+  or
+  // The case for `Loop` already checked the condition and the statement.
+  convertedExprMayThrow(stmt.(RangeBasedForStmt).getUpdate())
+  or
+  // The case for `Loop` already checked the condition and the statement.
+  exists(ForStmt forStmt | forStmt = stmt |
+    stmtMayThrow(forStmt.getInitialization())
+    or
+    convertedExprMayThrow(forStmt.getUpdate())
+  )
+  or
+  exists(SwitchStmt switchStmt | switchStmt = stmt |
+    convertedExprMayThrow(switchStmt.getExpr()) or
+    stmtMayThrow(switchStmt.getStmt())
+  )
+  or
+  // NOTE: We don't include `TryStmt` as those exceptions are not "observable" outside the function.
+  stmtMayThrow(stmt.(Handler).getBlock())
+  or
+  convertedExprMayThrow(stmt.(CoReturnStmt).getExpr())
+  or
+  convertedExprMayThrow(stmt.(ReturnStmt).getExpr())
+}
+
+/** Holds if the evaluation of `e` (including conversions) may throw an exception. */
+predicate convertedExprMayThrow(Expr e) {
+  exprMayThrow(e)
+  or
+  convertedExprMayThrow(e.getConversion())
+}
+
+/** Holds if the evaluation of `e` may throw an exception. */
+predicate exprMayThrow(Expr e) {
+  e instanceof DynamicCast
+  or
+  e instanceof TypeidOperator
+  or
+  e instanceof ThrowExpr
+  or
+  newMayThrow(e)
+  or
+  deleteMayThrow(e)
+  or
+  convertedExprMayThrow(e.(UnaryOperation).getOperand())
+  or
+  exists(BinaryOperation binOp | binOp = e |
+    convertedExprMayThrow([binOp.getLeftOperand(), binOp.getRightOperand()])
+  )
+  or
+  exists(Assignment assign | assign = e |
+    convertedExprMayThrow([assign.getLValue(), assign.getRValue()])
+  )
+  or
+  exists(CommaExpr comma | comma = e |
+    convertedExprMayThrow([comma.getLeftOperand(), comma.getRightOperand()])
+  )
+  or
+  exists(StmtExpr stmtExpr | stmtExpr = e |
+    convertedExprMayThrow(stmtExpr.getResultExpr()) or
+    stmtMayThrow(stmtExpr.getStmt())
+  )
+  or
+  convertedExprMayThrow(e.(Conversion).getExpr())
+  or
+  exists(FunctionCall fc | fc = e |
+    not exists(fc.getTarget()) or
+    functionMayThrow(fc.getTarget()) or
+    convertedExprMayThrow(fc.getAnArgument())
+  )
+}
+
+/** An allocator that might throw an exception. */
+class ThrowingAllocator extends Function {
+  ThrowingAllocator() {
+    exists(NewOrNewArrayExpr newExpr |
+      newExpr.getAllocator() = this and
+      functionMayThrow(this)
+    )
+  }
+}
+
+/** The `std::bad_alloc` exception and its `bsl` variant. */
+class BadAllocType extends Class {
+  BadAllocType() { this.hasGlobalOrStdOrBslName("bad_alloc") }
+}
+
+/**
+ * A catch block that catches a `std::bad_alloc` (or any of its superclasses), or a catch
+ * block that catches every exception (i.e., `catch(...)`).
+ */
+class BadAllocCatchBlock extends CatchBlock {
+  BadAllocCatchBlock() {
+    this.getParameter().getUnspecifiedType().stripType() =
+      any(BadAllocType badAlloc).getABaseClass*()
+    or
+    not exists(this.getParameter())
+  }
+}
+
+/**
+ * Holds if `newExpr` is embedded in a `try` statement with a catch block `catchBlock` that
+ * catches a `std::bad_alloc` exception, but nothing in the `try` block (including the `newExpr`)
+ * will throw that exception.
+ */
+predicate noThrowInTryBlock(NewOrNewArrayExpr newExpr, BadAllocCatchBlock catchBlock) {
+  exists(TryStmt try |
+    not stmtMayThrow(try.getStmt()) and
+    try.getACatchClause() = catchBlock and
+    newExpr.getEnclosingBlock().getEnclosingBlock*() = try.getStmt()
+  )
+}
+
+/**
+ * Holds if `newExpr` is handles allocation failures by throwing an exception, yet
+ * the guard condition `guard` compares the result of `newExpr` to a null value.
+ */
+predicate nullCheckInThrowingNew(NewOrNewArrayExpr newExpr, GuardCondition guard) {
+  newExpr.getAllocator() instanceof ThrowingAllocator and
+  (
+    // Handles null comparisons.
+    guard.ensuresEq(globalValueNumber(newExpr).getAnExpr(), any(NullValue null), _, _, _)
+    or
+    // Handles `if(ptr)` and `if(!ptr)` cases.
+    guard = globalValueNumber(newExpr).getAnExpr()
+  )
+}
+
+from NewOrNewArrayExpr newExpr, Element element, string msg, string elementString
+where
+  not newExpr.isFromUninstantiatedTemplate(_) and
+  (
+    noThrowInTryBlock(newExpr, element) and
+    msg = "This allocation cannot throw. $@ is unnecessary." and
+    elementString = "This catch block"
+    or
+    nullCheckInThrowingNew(newExpr, element) and
+    msg = "This allocation cannot return null. $@ is unnecessary." and
+    elementString = "This check"
+  )
+select newExpr, msg, element, elementString

cpp/ql/src/experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.ql

@@ -1,93 +0,0 @@
-/**
- * @name Detect And Handle Memory Allocation Errors
- * @description `operator new` throws an exception on allocation failures, while `operator new(std::nothrow)` returns a null pointer. Mixing up these two failure conditions can result in unexpected behavior.
- * @kind problem
- * @id cpp/detect-and-handle-memory-allocation-errors
- * @problem.severity warning
- * @precision medium
- * @tags correctness
- *       security
- *       external/cwe/cwe-570
- */
-
-import cpp
-
-/**
- * Lookup if condition compare with 0
- */
-class IfCompareWithZero extends IfStmt {
-  IfCompareWithZero() {
-    this.getCondition().(EQExpr).getAChild().getValue() = "0"
-    or
-    this.getCondition().(NEExpr).getAChild().getValue() = "0" and
-    this.hasElse()
-    or
-    this.getCondition().(NEExpr).getAChild().getValue() = "0" and
-    this.getThen().getAChild*() instanceof ReturnStmt
-  }
-}
-
-/**
- * lookup for calls to `operator new`, with incorrect error handling.
- */
-class WrongCheckErrorOperatorNew extends FunctionCall {
-  Expr exp;
-
-  WrongCheckErrorOperatorNew() {
-    this = exp.(NewOrNewArrayExpr).getAChild().(FunctionCall) and
-    (
-      this.getTarget().hasGlobalOrStdName("operator new")
-      or
-      this.getTarget().hasGlobalOrStdName("operator new[]")
-    )
-  }
-
-  /**
-   * Holds if handler `try ... catch` exists.
-   */
-  predicate isExistsTryCatchBlock() {
-    exists(TryStmt ts | this.getEnclosingStmt() = ts.getStmt().getAChild*())
-  }
-
-  /**
-   * Holds if results call `operator new` check in `operator if`.
-   */
-  predicate isExistsIfCondition() {
-    exists(IfCompareWithZero ifc |
-      // call `operator new` directly from the condition of `operator if`.
-      this = ifc.getCondition().getAChild*()
-      or
-      postDominates(ifc, this) and
-      exists(Variable v |
-        v = ifc.getCondition().getAChild().(VariableAccess).getTarget() and
-        (
-          exists(AssignExpr aex |
-            // check results call `operator new` with variable appropriation
-            aex.getAChild() = exp and
-            v = aex.getLValue().(VariableAccess).getTarget()
-          )
-          or
-          exists(Initializer it |
-            // check results call `operator new` with declaration variable
-            exp = it.getExpr() and
-            it.getDeclaration() = v
-          )
-        )
-      )
-    )
-  }
-
-  /**
-   * Holds if `(std::nothrow)` or `(std::noexcept)` exists in call `operator new`.
-   */
-  predicate isExistsNothrow() { getTarget().isNoExcept() or getTarget().isNoThrow() }
-}
-
-from WrongCheckErrorOperatorNew op
-where
-  // use call `operator new` with `(std::nothrow)` and checking error using `try ... catch` block and not `operator if`
-  op.isExistsNothrow() and not op.isExistsIfCondition() and op.isExistsTryCatchBlock()
-  or
-  // use call `operator new` without `(std::nothrow)` and checking error using `operator if` and not  `try ... catch` block
-  not op.isExistsNothrow() and not op.isExistsTryCatchBlock() and op.isExistsIfCondition()
-select op, "memory allocation error check is incorrect or missing"

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

@@ -850,6 +850,24 @@ class NewOrNewArrayExpr extends Expr, @any_new_expr {
       this.getAllocatorCall()
           .getArgument(this.getAllocator().(OperatorNewAllocationFunction).getPlacementArgument())
   }
+
+  /**
+   * For `operator new`, this gets the call or expression that initializes the allocated object, if any.
+   *
+   * As examples, for `new int(4)`, this will be `4`, and for `new std::vector(4)`, this will
+   * be a call to the constructor `std::vector::vector(size_t)` with `4` as an argument.
+   *
+   * For `operator new[]`, this gets the call or expression that initializes the first element of the
+   * array, if any.
+   *
+   * This will either be a call to the default constructor for the array's element type (as
+   * in `new std::string[10]`), or a literal zero for arrays of scalars which are zero-initialized
+   * due to extra parentheses (as in `new int[10]()`).
+   *
+   * At runtime, the constructor will be called once for each element in the array, but the
+   * constructor call only exists once in the AST.
+   */
+  final Expr getInitializer() { result = this.getChild(1) }
 }
 
 /**
@@ -871,14 +889,6 @@ class NewExpr extends NewOrNewArrayExpr, @new_expr {
   override Type getAllocatedType() {
     new_allocated_type(underlyingElement(this), unresolveElement(result))
   }
-
-  /**
-   * Gets the call or expression that initializes the allocated object, if any.
-   *
-   * As examples, for `new int(4)`, this will be `4`, and for `new std::vector(4)`, this will
-   * be a call to the constructor `std::vector::vector(size_t)` with `4` as an argument.
-   */
-  Expr getInitializer() { result = this.getChild(1) }
 }
 
 /**
@@ -909,18 +919,6 @@ class NewArrayExpr extends NewOrNewArrayExpr, @new_array_expr {
     result = getType().getUnderlyingType().(PointerType).getBaseType()
   }
 
-  /**
-   * Gets the call or expression that initializes the first element of the array, if any.
-   *
-   * This will either be a call to the default constructor for the array's element type (as
-   * in `new std::string[10]`), or a literal zero for arrays of scalars which are zero-initialized
-   * due to extra parentheses (as in `new int[10]()`).
-   *
-   * At runtime, the constructor will be called once for each element in the array, but the
-   * constructor call only exists once in the AST.
-   */
-  Expr getInitializer() { result = this.getChild(1) }
-
   /**
    * Gets the extent of the non-constant array dimension, if any.
    *

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-570/semmle/tests/IncorrectAllocationErrorHandling.expected

@@ -0,0 +1,18 @@
+| test.cpp:21:9:21:15 | new | This allocation cannot return null. $@ is unnecessary. | test.cpp:21:9:21:15 | new | This check |
+| test.cpp:29:13:29:24 | new[] | This allocation cannot return null. $@ is unnecessary. | test.cpp:30:7:30:13 | ... == ... | This check |
+| test.cpp:33:13:33:24 | new[] | This allocation cannot return null. $@ is unnecessary. | test.cpp:34:8:34:9 | p2 | This check |
+| test.cpp:37:13:37:24 | new[] | This allocation cannot return null. $@ is unnecessary. | test.cpp:38:7:38:16 | ... == ... | This check |
+| test.cpp:41:13:41:24 | new[] | This allocation cannot return null. $@ is unnecessary. | test.cpp:42:7:42:19 | ... == ... | This check |
+| test.cpp:45:13:45:24 | new[] | This allocation cannot return null. $@ is unnecessary. | test.cpp:46:7:46:8 | p5 | This check |
+| test.cpp:49:8:49:19 | new[] | This allocation cannot return null. $@ is unnecessary. | test.cpp:50:7:50:13 | ... == ... | This check |
+| test.cpp:53:8:53:19 | new[] | This allocation cannot return null. $@ is unnecessary. | test.cpp:54:8:54:9 | p7 | This check |
+| test.cpp:58:8:58:19 | new[] | This allocation cannot return null. $@ is unnecessary. | test.cpp:59:7:59:16 | ... == ... | This check |
+| test.cpp:63:8:63:19 | new[] | This allocation cannot return null. $@ is unnecessary. | test.cpp:64:7:64:19 | ... != ... | This check |
+| test.cpp:69:9:69:20 | new[] | This allocation cannot return null. $@ is unnecessary. | test.cpp:70:7:70:14 | ... != ... | This check |
+| test.cpp:75:11:75:22 | new[] | This allocation cannot return null. $@ is unnecessary. | test.cpp:76:13:76:15 | p11 | This check |
+| test.cpp:92:5:92:31 | new[] | This allocation cannot throw. $@ is unnecessary. | test.cpp:97:36:98:3 | { ... } | This catch block |
+| test.cpp:93:15:93:41 | new[] | This allocation cannot throw. $@ is unnecessary. | test.cpp:97:36:98:3 | { ... } | This catch block |
+| test.cpp:96:10:96:36 | new[] | This allocation cannot throw. $@ is unnecessary. | test.cpp:97:36:98:3 | { ... } | This catch block |
+| test.cpp:151:9:151:24 | new | This allocation cannot throw. $@ is unnecessary. | test.cpp:152:15:152:18 | { ... } | This catch block |
+| test.cpp:199:15:199:35 | new | This allocation cannot throw. $@ is unnecessary. | test.cpp:201:16:201:19 | { ... } | This catch block |
+| test.cpp:212:14:212:34 | new | This allocation cannot throw. $@ is unnecessary. | test.cpp:213:34:213:36 | { ... } | This catch block |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-570/semmle/tests/IncorrectAllocationErrorHandling.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-570/IncorrectAllocationErrorHandling.ql

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-570/semmle/tests/WrongInDetectingAndHandlingMemoryAllocationErrors.expected

@@ -1,9 +0,0 @@
-| test.cpp:29:13:29:24 | call to operator new[] | memory allocation error check is incorrect or missing |
-| test.cpp:37:13:37:24 | call to operator new[] | memory allocation error check is incorrect or missing |
-| test.cpp:41:13:41:24 | call to operator new[] | memory allocation error check is incorrect or missing |
-| test.cpp:49:8:49:19 | call to operator new[] | memory allocation error check is incorrect or missing |
-| test.cpp:58:8:58:19 | call to operator new[] | memory allocation error check is incorrect or missing |
-| test.cpp:63:8:63:19 | call to operator new[] | memory allocation error check is incorrect or missing |
-| test.cpp:92:5:92:31 | call to operator new[] | memory allocation error check is incorrect or missing |
-| test.cpp:93:15:93:41 | call to operator new[] | memory allocation error check is incorrect or missing |
-| test.cpp:96:10:96:36 | call to operator new[] | memory allocation error check is incorrect or missing |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-570/semmle/tests/WrongInDetectingAndHandlingMemoryAllocationErrors.qlref

@@ -1 +0,0 @@
-experimental/Security/CWE/CWE-570/WrongInDetectingAndHandlingMemoryAllocationErrors.ql

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-570/semmle/tests/test.cpp

@@ -18,7 +18,7 @@ void *operator new(std::size_t, const std::nothrow_t &) noexcept;
 void *operator new[](std::size_t, const std::nothrow_t &) noexcept;
 
 void bad_new_in_condition() {
-  if (!(new int)) { // BAD [NOT DETECTED]
+  if (!(new int)) { // BAD
     return;
   }
 }
@@ -30,7 +30,7 @@ void bad_new_missing_exception_handling() {
   if (p1 == 0)
     return;
 
-  int *p2 = new int[100]; // BAD [NOT DETECTED]
+  int *p2 = new int[100]; // BAD
   if (!p2)
     return;
 
@@ -42,7 +42,7 @@ void bad_new_missing_exception_handling() {
   if (p4 == nullptr)
     return;
 
-  int *p5 = new int[100]; // BAD [NOT DETECTED]
+  int *p5 = new int[100]; // BAD
   if (p5) {} else return;
 
   int *p6;
@@ -50,7 +50,7 @@ void bad_new_missing_exception_handling() {
   if (p6 == 0) return;
 
   int *p7;
-  p7 = new int[100]; // BAD [NOT DETECTED]
+  p7 = new int[100]; // BAD
   if (!p7)
     return;
 
@@ -66,13 +66,13 @@ void bad_new_missing_exception_handling() {
     return;
 
   int *p10;
-  p10 = new int[100]; // BAD [NOT DETECTED]
+  p10 = new int[100]; // BAD
   if (p10 != 0) {
   }
 
   int *p11;
   do {
-    p11 = new int[100]; // BAD [NOT DETECTED]
+    p11 = new int[100]; // BAD
   } while (!p11);
 
   int* p12 = new int[100];
@@ -135,3 +135,93 @@ void good_new_handles_nullptr() {
   if (new (std::nothrow) int[100] == nullptr)
     return; // GOOD
 }
+
+void* operator new(std::size_t count, void*) noexcept;
+void* operator new[](std::size_t count, void*) noexcept;
+
+struct Foo {
+  Foo() noexcept;
+  Foo(int);
+
+  operator bool();
+};
+
+void bad_placement_new_with_exception_handling() {
+  char buffer[1024];
+  try { new (buffer) Foo; } // BAD
+  catch (...) {  }
+}
+
+void good_placement_new_with_exception_handling() {
+  char buffer[1024];
+  try { new (buffer) Foo(42); } // GOOD: Foo constructor might throw
+  catch (...) {  }
+}
+
+int unknown_value_without_exceptions() noexcept;
+
+void may_throw() {
+  if(unknown_value_without_exceptions()) {
+    throw "bad luck exception!";
+  }
+}
+
+void unknown_code_that_may_throw(int*);
+void unknown_code_that_will_not_throw(int*) noexcept;
+
+void calls_throwing_code(int* p) {
+  if(unknown_value_without_exceptions()) unknown_code_that_may_throw(p);
+}
+
+void calls_non_throwing(int* p) {
+  if (unknown_value_without_exceptions()) unknown_code_that_will_not_throw(p);
+}
+
+void good_new_with_throwing_call() {
+  try {
+    int* p1 = new(std::nothrow) int; // GOOD
+    may_throw();
+  } catch(...) {  }
+
+  try {
+    int* p2 = new(std::nothrow) int; // GOOD
+    Foo f(10);
+  } catch(...) {  }
+
+  try {
+    int* p3 = new(std::nothrow) int; // GOOD
+    calls_throwing_code(p3);
+  } catch(...) {  }
+}
+
+void bad_new_with_nonthrowing_call() {
+  try {
+    int* p1 = new(std::nothrow) int; // BAD
+    calls_non_throwing(p1);
+  } catch(...) {  }
+
+  try {
+    int* p2 = new(std::nothrow) int; // GOOD: boolean conversion constructor might throw
+    Foo f;
+    if(f) { }
+  } catch(...) {  }
+}
+
+void bad_new_catch_baseclass_of_bad_alloc() {
+  try {
+    int* p = new(std::nothrow) int; // BAD
+  } catch(const std::exception&) { }
+}
+
+void good_new_catch_exception_in_assignment() {
+  int* p;
+  try {
+    p = new int; // GOOD
+  } catch(const std::bad_alloc&) { }
+}
+
+void good_new_catch_exception_in_conversion() {
+  try {
+    long* p = (long*) new int; // GOOD
+  } catch(const std::bad_alloc&) { }
+}