Merge pull request #11712 from aschackmull/java/constant-guards

Java: Apply deadcode guard to data flow nodes.

java/ql/lib/change-notes/2023-02-06-dataflow-deadcode.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The data flow library now disregards flow through code that is dead based on some basic constant propagation, for example, guards like `if (1+1>3)`.

java/ql/lib/semmle/code/java/Constants.qll

@@ -0,0 +1,168 @@
+/**
+ * Provdides a module to calculate constant integer and boolean values.
+ */
+
+import java
+
+signature boolean getBoolValSig(Expr e);
+
+signature int getIntValSig(Expr e);
+
+/**
+ * Given predicates defining boolean and integer constants, this module
+ * calculates additional boolean and integer constants using only the rules that
+ * apply to compile-time constants.
+ *
+ * The input and output predicates are expected to be mutually recursive.
+ */
+module CalculateConstants<getBoolValSig/1 getBoolVal, getIntValSig/1 getIntVal> {
+  /** Gets the value of a constant boolean expression. */
+  pragma[assume_small_delta]
+  boolean calculateBooleanValue(Expr e) {
+    // No casts relevant to booleans.
+    // `!` is the only unary operator that evaluates to a boolean.
+    result = getBoolVal(e.(LogNotExpr).getExpr()).booleanNot()
+    or
+    // Handle binary expressions that have integer operands and a boolean result.
+    exists(BinaryExpr b, int left, int right |
+      b = e and
+      left = getIntVal(b.getLeftOperand()) and
+      right = getIntVal(b.getRightOperand())
+    |
+      (
+        b instanceof LTExpr and
+        if left < right then result = true else result = false
+      )
+      or
+      (
+        b instanceof LEExpr and
+        if left <= right then result = true else result = false
+      )
+      or
+      (
+        b instanceof GTExpr and
+        if left > right then result = true else result = false
+      )
+      or
+      (
+        b instanceof GEExpr and
+        if left >= right then result = true else result = false
+      )
+      or
+      (
+        b instanceof ValueOrReferenceEqualsExpr and
+        if left = right then result = true else result = false
+      )
+      or
+      (
+        b instanceof ValueOrReferenceNotEqualsExpr and
+        if left != right then result = true else result = false
+      )
+    )
+    or
+    // Handle binary expressions that have boolean operands and a boolean result.
+    exists(BinaryExpr b, boolean left, boolean right |
+      b = e and
+      left = getBoolVal(b.getLeftOperand()) and
+      right = getBoolVal(b.getRightOperand())
+    |
+      (
+        b instanceof ValueOrReferenceEqualsExpr and
+        if left = right then result = true else result = false
+      )
+      or
+      (
+        b instanceof ValueOrReferenceNotEqualsExpr and
+        if left != right then result = true else result = false
+      )
+      or
+      (b instanceof AndBitwiseExpr or b instanceof AndLogicalExpr) and
+      result = left.booleanAnd(right)
+      or
+      (b instanceof OrBitwiseExpr or b instanceof OrLogicalExpr) and
+      result = left.booleanOr(right)
+      or
+      b instanceof XorBitwiseExpr and result = left.booleanXor(right)
+    )
+    or
+    // Ternary expressions, where the `true` and `false` expressions are boolean constants.
+    exists(ConditionalExpr ce, boolean condition |
+      ce = e and
+      condition = getBoolVal(ce.getCondition()) and
+      result = getBoolVal(ce.getBranchExpr(condition))
+    )
+    or
+    // If a `Variable` is final, its value is its initializer, if it exists.
+    exists(Variable v | e = v.getAnAccess() and v.isFinal() |
+      result = getBoolVal(v.getInitializer())
+    )
+  }
+
+  /** Gets the value of a constant integer expression. */
+  pragma[assume_small_delta]
+  int calculateIntValue(Expr e) {
+    exists(IntegralType t | e.getType() = t | t.getName().toLowerCase() != "long") and
+    (
+      exists(CastingExpr cast, int val | cast = e and val = getIntVal(cast.getExpr()) |
+        if cast.getType().hasName("byte")
+        then result = (val + 128).bitAnd(255) - 128
+        else
+          if cast.getType().hasName("short")
+          then result = (val + 32768).bitAnd(65535) - 32768
+          else
+            if cast.getType().hasName("char")
+            then result = val.bitAnd(65535)
+            else result = val
+      )
+      or
+      result = getIntVal(e.(PlusExpr).getExpr())
+      or
+      result = -getIntVal(e.(MinusExpr).getExpr())
+      or
+      result = getIntVal(e.(BitNotExpr).getExpr()).bitNot()
+      or
+      // No `int` value for `LogNotExpr`.
+      exists(BinaryExpr b, int v1, int v2 |
+        b = e and
+        v1 = getIntVal(b.getLeftOperand()) and
+        v2 = getIntVal(b.getRightOperand())
+      |
+        b instanceof MulExpr and result = v1 * v2
+        or
+        b instanceof DivExpr and result = v1 / v2
+        or
+        b instanceof RemExpr and result = v1 % v2
+        or
+        b instanceof AddExpr and result = v1 + v2
+        or
+        b instanceof SubExpr and result = v1 - v2
+        or
+        b instanceof LeftShiftExpr and result = v1.bitShiftLeft(v2)
+        or
+        b instanceof RightShiftExpr and result = v1.bitShiftRightSigned(v2)
+        or
+        b instanceof UnsignedRightShiftExpr and result = v1.bitShiftRight(v2)
+        or
+        b instanceof AndBitwiseExpr and result = v1.bitAnd(v2)
+        or
+        b instanceof OrBitwiseExpr and result = v1.bitOr(v2)
+        or
+        b instanceof XorBitwiseExpr and result = v1.bitXor(v2)
+        // No `int` value for `AndLogicalExpr` or `OrLogicalExpr`.
+        // No `int` value for `LTExpr`, `GTExpr`, `LEExpr`, `GEExpr`, `ValueOrReferenceEqualsExpr` or `ValueOrReferenceNotEqualsExpr`.
+      )
+      or
+      // Ternary conditional, with constant condition.
+      exists(ConditionalExpr ce, boolean condition |
+        ce = e and
+        condition = getBoolVal(ce.getCondition()) and
+        result = getIntVal(ce.getBranchExpr(condition))
+      )
+      or
+      // If a `Variable` is final, its value is its initializer, if it exists.
+      exists(Variable v | e = v.getAnAccess() and v.isFinal() |
+        result = getIntVal(v.getInitializer())
+      )
+    )
+  }
+}

java/ql/lib/semmle/code/java/Expr.qll

@@ -4,6 +4,7 @@
 
 import java
 private import semmle.code.java.frameworks.android.Compose
+private import semmle.code.java.Constants
 
 /** A common super-class that represents all kinds of expressions. */
 class Expr extends ExprParent, @expr {
@@ -130,6 +131,7 @@ private predicate primitiveOrString(Type t) {
  * See JLS v8, section 15.28 (Constant Expressions).
  */
 class CompileTimeConstantExpr extends Expr {
+  pragma[assume_small_delta]
   CompileTimeConstantExpr() {
     primitiveOrString(this.getType()) and
     (
@@ -179,6 +181,7 @@ class CompileTimeConstantExpr extends Expr {
   /**
    * Gets the string value of this expression, where possible.
    */
+  pragma[assume_small_delta]
   pragma[nomagic]
   string getStringValue() {
     result = this.(StringLiteral).getValue()
@@ -204,75 +207,13 @@ class CompileTimeConstantExpr extends Expr {
   /**
    * Gets the boolean value of this expression, where possible.
    */
+  pragma[assume_small_delta]
+  pragma[nomagic]
   boolean getBooleanValue() {
     // Literal value.
     result = this.(BooleanLiteral).getBooleanValue()
     or
-    // No casts relevant to booleans.
-    // `!` is the only unary operator that evaluates to a boolean.
-    result = this.(LogNotExpr).getExpr().(CompileTimeConstantExpr).getBooleanValue().booleanNot()
-    or
-    // Handle binary expressions that have integer operands and a boolean result.
-    exists(BinaryExpr b, int left, int right |
-      b = this and
-      left = b.getLeftOperand().(CompileTimeConstantExpr).getIntValue() and
-      right = b.getRightOperand().(CompileTimeConstantExpr).getIntValue()
-    |
-      (
-        b instanceof LTExpr and
-        if left < right then result = true else result = false
-      )
-      or
-      (
-        b instanceof LEExpr and
-        if left <= right then result = true else result = false
-      )
-      or
-      (
-        b instanceof GTExpr and
-        if left > right then result = true else result = false
-      )
-      or
-      (
-        b instanceof GEExpr and
-        if left >= right then result = true else result = false
-      )
-      or
-      (
-        b instanceof ValueOrReferenceEqualsExpr and
-        if left = right then result = true else result = false
-      )
-      or
-      (
-        b instanceof ValueOrReferenceNotEqualsExpr and
-        if left != right then result = true else result = false
-      )
-    )
-    or
-    // Handle binary expressions that have boolean operands and a boolean result.
-    exists(BinaryExpr b, boolean left, boolean right |
-      b = this and
-      left = b.getLeftOperand().(CompileTimeConstantExpr).getBooleanValue() and
-      right = b.getRightOperand().(CompileTimeConstantExpr).getBooleanValue()
-    |
-      (
-        b instanceof ValueOrReferenceEqualsExpr and
-        if left = right then result = true else result = false
-      )
-      or
-      (
-        b instanceof ValueOrReferenceNotEqualsExpr and
-        if left != right then result = true else result = false
-      )
-      or
-      (b instanceof AndBitwiseExpr or b instanceof AndLogicalExpr) and
-      result = left.booleanAnd(right)
-      or
-      (b instanceof OrBitwiseExpr or b instanceof OrLogicalExpr) and
-      result = left.booleanOr(right)
-      or
-      b instanceof XorBitwiseExpr and result = left.booleanXor(right)
-    )
+    result = CalcCompileTimeConstants::calculateBooleanValue(this)
     or
     // Handle binary expressions that have `String` operands and a boolean result.
     exists(BinaryExpr b, string left, string right |
@@ -300,18 +241,6 @@ class CompileTimeConstantExpr extends Expr {
     )
     or
     // Note: no `getFloatValue()`, so we cannot support binary expressions with float or double operands.
-    // Ternary expressions, where the `true` and `false` expressions are boolean compile-time constants.
-    exists(ConditionalExpr ce, boolean condition |
-      ce = this and
-      condition = ce.getCondition().(CompileTimeConstantExpr).getBooleanValue() and
-      result = ce.getBranchExpr(condition).(CompileTimeConstantExpr).getBooleanValue()
-    )
-    or
-    // Simple or qualified names where the variable is final and the initializer is a constant.
-    exists(Variable v | this = v.getAnAccess() |
-      result = v.getInitializer().(CompileTimeConstantExpr).getBooleanValue()
-    )
-    or
     result = this.(LiveLiteral).getValue().getBooleanValue()
   }
 
@@ -329,75 +258,20 @@ class CompileTimeConstantExpr extends Expr {
       result = this.(IntegerLiteral).getIntValue()
       or
       result = this.(CharacterLiteral).getCodePointValue()
-      or
-      exists(CastingExpr cast, int val |
-        cast = this and val = cast.getExpr().(CompileTimeConstantExpr).getIntValue()
-      |
-        if cast.getType().hasName("byte")
-        then result = (val + 128).bitAnd(255) - 128
-        else
-          if cast.getType().hasName("short")
-          then result = (val + 32768).bitAnd(65535) - 32768
-          else
-            if cast.getType().hasName("char")
-            then result = val.bitAnd(65535)
-            else result = val
-      )
-      or
-      result = this.(PlusExpr).getExpr().(CompileTimeConstantExpr).getIntValue()
-      or
-      result = -this.(MinusExpr).getExpr().(CompileTimeConstantExpr).getIntValue()
-      or
-      result = this.(BitNotExpr).getExpr().(CompileTimeConstantExpr).getIntValue().bitNot()
-      or
-      // No `int` value for `LogNotExpr`.
-      exists(BinaryExpr b, int v1, int v2 |
-        b = this and
-        v1 = b.getLeftOperand().(CompileTimeConstantExpr).getIntValue() and
-        v2 = b.getRightOperand().(CompileTimeConstantExpr).getIntValue()
-      |
-        b instanceof MulExpr and result = v1 * v2
-        or
-        b instanceof DivExpr and result = v1 / v2
-        or
-        b instanceof RemExpr and result = v1 % v2
-        or
-        b instanceof AddExpr and result = v1 + v2
-        or
-        b instanceof SubExpr and result = v1 - v2
-        or
-        b instanceof LeftShiftExpr and result = v1.bitShiftLeft(v2)
-        or
-        b instanceof RightShiftExpr and result = v1.bitShiftRightSigned(v2)
-        or
-        b instanceof UnsignedRightShiftExpr and result = v1.bitShiftRight(v2)
-        or
-        b instanceof AndBitwiseExpr and result = v1.bitAnd(v2)
-        or
-        b instanceof OrBitwiseExpr and result = v1.bitOr(v2)
-        or
-        b instanceof XorBitwiseExpr and result = v1.bitXor(v2)
-        // No `int` value for `AndLogicalExpr` or `OrLogicalExpr`.
-        // No `int` value for `LTExpr`, `GTExpr`, `LEExpr`, `GEExpr`, `ValueOrReferenceEqualsExpr` or `ValueOrReferenceNotEqualsExpr`.
-      )
-      or
-      // Ternary conditional, with compile-time constant condition.
-      exists(ConditionalExpr ce, boolean condition |
-        ce = this and
-        condition = ce.getCondition().(CompileTimeConstantExpr).getBooleanValue() and
-        result = ce.getBranchExpr(condition).(CompileTimeConstantExpr).getIntValue()
-      )
-      or
-      // If a `Variable` is a `CompileTimeConstantExpr`, its value is its initializer.
-      exists(Variable v | this = v.getAnAccess() |
-        result = v.getInitializer().(CompileTimeConstantExpr).getIntValue()
-      )
     )
     or
+    result = CalcCompileTimeConstants::calculateIntValue(this)
+    or
     result = this.(LiveLiteral).getValue().getIntValue()
   }
 }
 
+private boolean getBoolValue(Expr e) { result = e.(CompileTimeConstantExpr).getBooleanValue() }
+
+private int getIntValue(Expr e) { result = e.(CompileTimeConstantExpr).getIntValue() }
+
+private module CalcCompileTimeConstants = CalculateConstants<getBoolValue/1, getIntValue/1>;
+
 /** An expression parent is an element that may have an expression as its child. */
 class ExprParent extends @exprparent, Top { }
 

java/ql/lib/semmle/code/java/dataflow/RangeUtils.qll

@@ -6,6 +6,7 @@ import java
 private import SSA
 private import semmle.code.java.controlflow.internal.GuardsLogic
 private import semmle.code.java.dataflow.internal.rangeanalysis.SsaReadPositionCommon
+private import semmle.code.java.Constants
 
 /**
  * Holds if `v` is an input to `phi` that is not along a back edge, and the
@@ -86,8 +87,29 @@ private predicate constantIntegerExpr(Expr e, int val) {
     arrlen.getQualifier() = a.getAnAccess() and
     e = arrlen
   )
+  or
+  CalcConstants::calculateIntValue(e) = val
 }
 
+pragma[nomagic]
+private predicate constantBooleanExpr(Expr e, boolean val) {
+  e.(CompileTimeConstantExpr).getBooleanValue() = val
+  or
+  exists(SsaExplicitUpdate v, Expr src |
+    e = v.getAUse() and
+    src = v.getDefiningExpr().(VariableAssign).getSource() and
+    constantBooleanExpr(src, val)
+  )
+  or
+  CalcConstants::calculateBooleanValue(e) = val
+}
+
+private boolean getBoolValue(Expr e) { constantBooleanExpr(e, result) }
+
+private int getIntValue(Expr e) { constantIntegerExpr(e, result) }
+
+private module CalcConstants = CalculateConstants<getBoolValue/1, getIntValue/1>;
+
 /** An expression that always has the same integer value. */
 class ConstantIntegerExpr extends Expr {
   ConstantIntegerExpr() { constantIntegerExpr(this, _) }
@@ -96,6 +118,14 @@ class ConstantIntegerExpr extends Expr {
   int getIntValue() { constantIntegerExpr(this, result) }
 }
 
+/** An expression that always has the same boolean value. */
+class ConstantBooleanExpr extends Expr {
+  ConstantBooleanExpr() { constantBooleanExpr(this, _) }
+
+  /** Gets the boolean value of this expression. */
+  boolean getBooleanValue() { constantBooleanExpr(this, result) }
+}
+
 /**
  * Gets an expression that equals `v - d`.
  */

java/ql/lib/semmle/code/java/dataflow/SSA.qll

@@ -384,7 +384,7 @@ private module SsaImpl {
   private predicate intraInstanceCallEdge(Callable c1, Method m2) {
     exists(MethodAccess ma, RefType t1 |
       ma.getCaller() = c1 and
-      m2 = viableImpl(ma) and
+      m2 = viableImpl_v2(ma) and
       not m2.isStatic() and
       (
         not exists(ma.getQualifier()) or
@@ -402,7 +402,7 @@ private module SsaImpl {
   }
 
   private Callable tgt(Call c) {
-    result = viableImpl(c)
+    result = viableImpl_v2(c)
     or
     result = getRunnerTarget(c)
     or

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowNodes.qll

@@ -7,16 +7,26 @@ private import DataFlowPrivate
 private import DataFlowUtil
 private import FlowSummaryImpl as FlowSummaryImpl
 private import DataFlowImplCommon as DataFlowImplCommon
+private import semmle.code.java.controlflow.Guards
+private import semmle.code.java.dataflow.RangeUtils
 
 /** Gets a string for approximating the name of a field. */
 string approximateFieldContent(FieldContent fc) { result = fc.getField().getName().prefix(1) }
 
+private predicate deadcode(Expr e) {
+  exists(Guard g, boolean b |
+    g.(ConstantBooleanExpr).getBooleanValue() = b and
+    g.controls(e.getBasicBlock(), b.booleanNot())
+  )
+}
+
 cached
 private module Cached {
   cached
   newtype TNode =
     TExprNode(Expr e) {
       DataFlowImplCommon::forceCachingInSameStage() and
+      not deadcode(e) and
       not e.getType() instanceof VoidType and
       not e.getParent*() instanceof Annotation
     } or

java/ql/lib/semmle/code/java/dispatch/WrappedInvocation.qll

@@ -34,7 +34,7 @@ private predicate runner(Method m, int n, Method runmethod) {
 private Expr getRunnerArgument(MethodAccess ma, Method runmethod) {
   exists(Method runner, int param |
     runner(runner, param, runmethod) and
-    viableImpl(ma) = runner and
+    viableImpl_v2(ma) = runner and
     result = ma.getArgument(param)
   )
   or

java/ql/src/Likely Bugs/Collections/ArrayIndexOutOfBounds.ql

@@ -44,6 +44,15 @@ predicate boundedArrayAccess(ArrayAccess aa, int k) {
       )
     )
   )
+  or
+  exists(Field arr, Expr index, int delta, int arrlen |
+    aa.getIndexExpr() = index and
+    aa.getArray() = arr.getAnAccess() and
+    bounded(index, any(ZeroBound z), delta, true, _) and
+    arr.isFinal() and
+    arr.getInitializer().(ArrayCreationExpr).getFirstDimensionSize() = arrlen and
+    k = delta - arrlen
+  )
 }
 
 /**

java/ql/src/change-notes/2023-02-06-index-out-of-bounds-constant.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `java/index-out-of-bounds` query has improved its handling of arrays of constant length, and may report additional results in those cases.

java/ql/test/query-tests/RangeAnalysis/A.java

@@ -204,4 +204,11 @@ public class A {
       A.arr1[RandomUtils.nextInt(0, arr1.length + 1)] + // BAD: random int may be out of range
       A.arr1[RandomUtils.nextInt(0, arr1.length)]; // GOOD: random int must be in range
   }
+
+  int m17() {
+    return this.arr2[(new Random()).nextInt(arr2.length + 1)] +  // BAD: random int may be out of range
+      this.arr2[(new Random()).nextInt(arr2.length)] + // GOOD: random int must be in range
+      this.arr2[RandomUtils.nextInt(0, arr2.length + 1)] + // BAD: random int may be out of range
+      this.arr2[RandomUtils.nextInt(0, arr2.length)]; // GOOD: random int must be in range
+  }
 }

java/ql/test/query-tests/RangeAnalysis/ArrayIndexOutOfBounds.expected

@@ -14,3 +14,5 @@
 | A.java:195:9:195:13 | ...[...] | This array access might be out of bounds, as the index might be equal to the array length. |
 | A.java:202:12:202:58 | ...[...] | This array access might be out of bounds, as the index might be equal to the array length. |
 | A.java:204:7:204:53 | ...[...] | This array access might be out of bounds, as the index might be equal to the array length. |
+| A.java:209:12:209:61 | ...[...] | This array access might be out of bounds, as the index might be equal to the array length. |
+| A.java:211:7:211:56 | ...[...] | This array access might be out of bounds, as the index might be equal to the array length. |