From 5f6d07dd57e965747722cf7c34e689aec74fc40b Mon Sep 17 00:00:00 2001
From: Jonas Jensen <jbj@github.com>
Date: Wed, 26 Feb 2020 11:32:41 +0100
Subject: [PATCH] C++: Fix performance of UnsignedGEZero.ql

This query used two fastTC operations that were already somewhat
inefficient on their own but could send the evaluator into an OOM loop
when run in parallel without enough RAM.

The fix is to recurse manually, starting just from the expressions that
are potential candidates for alerts.
---
 .../Likely Bugs/Arithmetic/UnsignedGEZero.qll | 39 ++++++++++++-------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/UnsignedGEZero.qll b/cpp/ql/src/Likely Bugs/Arithmetic/UnsignedGEZero.qll
index 4ca68fb27c4..e0cc92b6ce8 100644
--- a/cpp/ql/src/Likely Bugs/Arithmetic/UnsignedGEZero.qll	
+++ b/cpp/ql/src/Likely Bugs/Arithmetic/UnsignedGEZero.qll	
@@ -15,24 +15,33 @@ class ConstantZero extends Expr {
   }
 }
 
+/**
+ * Holds if `candidate` is an expression such that if it's unsigned then we
+ * want an alert at `ge`.
+ */
+private predicate lookForUnsignedAt(GEExpr ge, Expr candidate) {
+  // Base case: `candidate >= 0`
+  ge.getRightOperand() instanceof ConstantZero and
+  candidate = ge.getLeftOperand().getFullyConverted() and
+  // left operand was a signed or unsigned IntegralType before conversions
+  // (not a pointer, checking a pointer >= 0 is an entirely different mistake)
+  // (not an enum, as the fully converted type of an enum is compiler dependent
+  //  so checking an enum >= 0 is always reasonable)
+  ge.getLeftOperand().getUnderlyingType() instanceof IntegralType
+  or
+  // Recursive case: `...(largerType)candidate >= 0`
+  exists(Conversion conversion |
+    lookForUnsignedAt(ge, conversion) and
+    candidate = conversion.getExpr() and
+    conversion.getType().getSize() > candidate.getType().getSize()
+  )
+}
+
 class UnsignedGEZero extends GEExpr {
   UnsignedGEZero() {
-    this.getRightOperand() instanceof ConstantZero and
-    // left operand was a signed or unsigned IntegralType before conversions
-    // (not a pointer, checking a pointer >= 0 is an entirely different mistake)
-    // (not an enum, as the fully converted type of an enum is compiler dependent
-    //  so checking an enum >= 0 is always reasonable)
-    getLeftOperand().getUnderlyingType() instanceof IntegralType and
     exists(Expr ue |
-      // ue is some conversion of the left operand
-      ue = getLeftOperand().getConversion*() and
-      // ue is unsigned
-      ue.getUnderlyingType().(IntegralType).isUnsigned() and
-      // ue may be converted to zero or more strictly larger possibly signed types
-      // before it is fully converted
-      forall(Expr following | following = ue.getConversion+() |
-        following.getType().getSize() > ue.getType().getSize()
-      )
+      lookForUnsignedAt(this, ue) and
+      ue.getUnderlyingType().(IntegralType).isUnsigned()
     )
   }
 }