Java: Add negative type check for uncaught exceptions.

Author: aschackmull

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

@@ -536,6 +536,24 @@ module Private {
 
     /** Gets the catch clause associated with this node. */
     CatchClause getCatch() { this = TCatchTypeTestNode(result) }
+
+    Node getSuccessor(boolean match) {
+      match = true and
+      this.getCatch() = result.(CatchParameterNode).getCatch()
+      or
+      match = false and
+      exists(TryStmt try, int i, CatchClause cc |
+        cc = this.getCatch() and
+        cc = try.getCatchClause(i) and
+        // A catch-all does not allow for uncaught exceptions.
+        not cc.getACaughtType() instanceof TypeThrowable
+      |
+        result.(CatchTypeTestNode).getCatch() = try.getCatchClause(i + 1)
+        or
+        not exists(try.getCatchClause(i + 1)) and
+        result.(UncaughtNode).getTry() = try
+      )
+    }
   }
 
   /**

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

@@ -144,14 +144,7 @@ module ExceptionFlow {
       )
     )
     or
-    node1.(CatchTypeTestNode).getCatch() = node2.(CatchParameterNode).getCatch()
-    or
-    exists(TryStmt try, int i | node1.(CatchTypeTestNode).getCatch() = try.getCatchClause(i) |
-      node2.(CatchTypeTestNode).getCatch() = try.getCatchClause(i + 1)
-      or
-      not exists(try.getCatchClause(i + 1)) and
-      node2.(UncaughtNode).getTry() = try
-    )
+    node1.(CatchTypeTestNode).getSuccessor(_) = node2
   }
 }
 
@@ -243,7 +236,7 @@ predicate expectsContent(Node n, ContentSet c) {
  * possible flow. A single type is used for all numeric types to account for
  * numeric conversions, and otherwise the erasure is used.
  */
-DataFlowType getErasedRepr(Type t) {
+RefType getErasedRepr0(Type t) {
   exists(Type e | e = t.getErasure() |
     if e instanceof NumericOrCharType
     then result.(BoxedType).getPrimitiveType().getName() = "double"
@@ -256,47 +249,59 @@ DataFlowType getErasedRepr(Type t) {
   t instanceof NullType and result instanceof TypeObject
 }
 
+DataFlowType getErasedRepr(Type t) { result = TType(getErasedRepr0(t)) }
+
+CatchClause getNegativeType(Node n) {
+  exists(CatchTypeTestNode ct |
+    n = ct.getSuccessor(false) and
+    result = ct.getCatch()
+  )
+}
+
 pragma[noinline]
 DataFlowType getNodeType(Node n) {
+  result.asNegType() = getNegativeType(n)
+  or
+  not exists(getNegativeType(n)) and
   result = getErasedRepr(n.getTypeBound())
   or
   result = FlowSummaryImpl::Private::summaryNodeType(n)
 }
 
 /** Gets a string representation of a type returned by `getErasedRepr`. */
-string ppReprType(Type t) {
+private string ppErasedReprType(Type t) {
   if t.(BoxedType).getPrimitiveType().getName() = "double"
   then result = "Number"
   else result = t.toString()
 }
 
-private predicate canContainBool(Type t) {
-  t instanceof BooleanType or
-  any(BooleanType b).(RefType).getASourceSupertype+() = t
-}
+/** Gets a string representation of a type returned by `getErasedRepr`. */
+string ppReprType(DataFlowType t) { result = t.toString() }
 
 /**
  * Holds if `t1` and `t2` are compatible, that is, whether data can flow from
  * a node of type `t1` to a node of type `t2`.
  */
 pragma[inline]
-predicate compatibleTypes(Type t1, Type t2) {
-  exists(Type e1, Type e2 |
-    e1 = getErasedRepr(t1) and
-    e2 = getErasedRepr(t2)
-  |
-    // Because of `getErasedRepr`, `erasedHaveIntersection` is a sufficient
-    // compatibility check, but `conContainBool` is kept as a dummy disjunct
-    // to get the proper join-order.
-    erasedHaveIntersection(e1, e2)
+predicate compatibleTypes(DataFlowType t1, DataFlowType t2) {
+  erasedHaveIntersection(t1.asType(), t2.asType())
+  or
+  exists(RefType pos, CatchClause neg |
+    pos = t1.asType() and neg = t2.asNegType()
     or
-    canContainBool(e1) and canContainBool(e2)
+    pos = t2.asType() and neg = t1.asNegType()
+  |
+    not pos.getASourceSupertype*() = neg.getACaughtType()
   )
 }
 
 /** A node that performs a type cast. */
 class CastNode extends Node {
-  CastNode() { this.asExpr() instanceof CastingExpr or this instanceof CatchParameterNode }
+  CastNode() {
+    this.asExpr() instanceof CastingExpr or
+    this instanceof CatchParameterNode or
+    exists(getNegativeType(this))
+  }
 }
 
 private newtype TDataFlowCallable =
@@ -326,7 +331,20 @@ class DataFlowCallable extends TDataFlowCallable {
 
 class DataFlowExpr = Expr;
 
-class DataFlowType = RefType;
+private newtype TDataFlowType =
+  TType(RefType t) { t = getErasedRepr0(_) } or
+  TNegType(CatchClause cc)
+
+class DataFlowType extends TDataFlowType {
+  RefType asType() { this = TType(result) }
+
+  CatchClause asNegType() { this = TNegType(result) }
+
+  string toString() {
+    result = ppErasedReprType(this.asType()) or
+    result = "Not type: " + this.asNegType().getVariable().getTypeAccess()
+  }
+}
 
 private newtype TDataFlowCall =
   TCall(Call c) or
@@ -468,6 +486,7 @@ predicate lambdaCreation(Node creation, LambdaCallKind kind, DataFlowCallable c)
 predicate lambdaCall(DataFlowCall call, LambdaCallKind kind, Node receiver) {
   receiver = call.(SummaryCall).getReceiver() and
   getNodeDataFlowType(receiver)
+      .asType()
       .getSourceDeclaration()
       .(FunctionalInterface)
       .getRunMethod()

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

@@ -204,7 +204,7 @@ private newtype TContent =
  */
 class Content extends TContent {
   /** Gets the type of the contained data for the purpose of type pruning. */
-  abstract DataFlowType getType();
+  abstract Type getType();
 
   /** Gets a textual representation of this element. */
   abstract string toString();
@@ -229,7 +229,7 @@ class FieldContent extends Content, TFieldContent {
 
   InstanceField getField() { result = f }
 
-  override DataFlowType getType() { result = getErasedRepr(f.getType()) }
+  override Type getType() { result = getErasedRepr0(f.getType()) }
 
   override string toString() { result = f.toString() }
 
@@ -240,28 +240,28 @@ class FieldContent extends Content, TFieldContent {
 
 /** A reference through an array. */
 class ArrayContent extends Content, TArrayContent {
-  override DataFlowType getType() { result instanceof TypeObject }
+  override Type getType() { result instanceof TypeObject }
 
   override string toString() { result = "[]" }
 }
 
 /** A reference through the contents of some collection-like container. */
 class CollectionContent extends Content, TCollectionContent {
-  override DataFlowType getType() { result instanceof TypeObject }
+  override Type getType() { result instanceof TypeObject }
 
   override string toString() { result = "<element>" }
 }
 
 /** A reference through a map key. */
 class MapKeyContent extends Content, TMapKeyContent {
-  override DataFlowType getType() { result instanceof TypeObject }
+  override Type getType() { result instanceof TypeObject }
 
   override string toString() { result = "<map.key>" }
 }
 
 /** A reference through a map value. */
 class MapValueContent extends Content, TMapValueContent {
-  override DataFlowType getType() { result instanceof TypeObject }
+  override Type getType() { result instanceof TypeObject }
 
   override string toString() { result = "<map.value>" }
 }
@@ -274,7 +274,7 @@ class SyntheticFieldContent extends Content, TSyntheticFieldContent {
 
   SyntheticField getField() { result = s }
 
-  override DataFlowType getType() { result = getErasedRepr(s.getType()) }
+  override Type getType() { result = getErasedRepr0(s.getType()) }
 
   override string toString() { result = s.toString() }
 }

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

@@ -26,7 +26,7 @@ Node summaryNode(SummarizedCallable c, SummaryNodeState state) { result = getSum
 SummaryCall summaryDataFlowCall(Node receiver) { result.getReceiver() = receiver }
 
 /** Gets the type of content `c`. */
-DataFlowType getContentType(Content c) { result = c.getType() }
+DataFlowType getContentType(Content c) { result.asType() = c.getType() }
 
 /** Gets the return type of kind `rk` for callable `c`. */
 DataFlowType getReturnType(SummarizedCallable c, ReturnKind rk) {
@@ -35,17 +35,17 @@ DataFlowType getReturnType(SummarizedCallable c, ReturnKind rk) {
   or
   rk instanceof ExceptionReturnKind and
   exists(c) and
-  result instanceof TypeThrowable
+  result.asType() instanceof TypeThrowable
 }
 
 /**
  * Gets the type of the `i`th parameter in a synthesized call that targets a
  * callback of type `t`.
  */
 DataFlowType getCallbackParameterType(DataFlowType t, int i) {
-  result = getErasedRepr(t.(FunctionalInterface).getRunMethod().getParameterType(i))
+  result = getErasedRepr(t.asType().(FunctionalInterface).getRunMethod().getParameterType(i))
   or
-  result = getErasedRepr(t.(FunctionalInterface)) and i = -1
+  result = getErasedRepr(t.asType().(FunctionalInterface)) and i = -1
 }
 
 /**
@@ -54,11 +54,11 @@ DataFlowType getCallbackParameterType(DataFlowType t, int i) {
  */
 DataFlowType getCallbackReturnType(DataFlowType t, ReturnKind rk) {
   rk instanceof NormalReturnKind and
-  result = getErasedRepr(t.(FunctionalInterface).getRunMethod().getReturnType())
+  result = getErasedRepr(t.asType().(FunctionalInterface).getRunMethod().getReturnType())
   or
   rk instanceof ExceptionReturnKind and
   exists(t) and
-  result instanceof TypeThrowable
+  result.asType() instanceof TypeThrowable
 }
 
 /** Gets the type of synthetic global `sg`. */

java/ql/test/library-tests/dataflow/exceptions/A.java

@@ -47,7 +47,7 @@ void foo(int i) {
     } catch (MyOtherException e) {
       sink(e.msg); // $ hasValueFlow=other
     } catch (Exception e) {
-      sink(((MyOtherException)e).msg); // $ SPURIOUS: hasValueFlow=other
+      sink(((MyOtherException)e).msg); // no flow
     }
   }