Merge branch 'main' into cleartextstorage

Author: geoffw0

java/ql/lib/semmle/code/java/controlflow/BasicBlocks.qll

@@ -57,7 +57,7 @@ private module Input implements BB::InputSig<Location> {
    * Holds if `node` represents an exit node to be used when calculating
    * post dominance.
    */
-  predicate nodeIsPostDominanceExit(Node node) { node instanceof ControlFlow::ExitNode }
+  predicate nodeIsPostDominanceExit(Node node) { node instanceof ControlFlow::NormalExitNode }
 }
 
 private module BbImpl = BB::Make<Location, Input>;

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

@@ -348,6 +348,16 @@ predicate expectsContent(Node n, ContentSet c) {
   FlowSummaryImpl::Private::Steps::summaryExpectsContent(n.(FlowSummaryNode).getSummaryNode(), c)
 }
 
+pragma[nomagic]
+private predicate numericRepresentative(RefType t) {
+  t.(BoxedType).getPrimitiveType().getName() = "double"
+}
+
+pragma[nomagic]
+private predicate booleanRepresentative(RefType t) {
+  t.(BoxedType).getPrimitiveType().getName() = "boolean"
+}
+
 /**
  * Gets a representative (boxed) type for `t` for the purpose of pruning
  * possible flow. A single type is used for all numeric types to account for
@@ -356,10 +366,10 @@ predicate expectsContent(Node n, ContentSet c) {
 RefType getErasedRepr(Type t) {
   exists(Type e | e = t.getErasure() |
     if e instanceof NumericOrCharType
-    then result.(BoxedType).getPrimitiveType().getName() = "double"
+    then numericRepresentative(result)
     else
       if e instanceof BooleanType
-      then result.(BoxedType).getPrimitiveType().getName() = "boolean"
+      then booleanRepresentative(result)
       else result = e
   )
   or

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

@@ -214,24 +214,35 @@ private predicate relevantNode(ObjNode n) {
   exists(ObjNode mid | relevantNode(mid) and objStep(mid, n) and relevantNodeBack(n))
 }
 
-pragma[noinline]
-private predicate objStepPruned(ObjNode n1, ObjNode n2) {
-  objStep(n1, n2) and relevantNode(n1) and relevantNode(n2)
+private newtype TObjFlowNode =
+  TObjNode(ObjNode n) { relevantNode(n) } or
+  TObjType(RefType t) { source(t, _) }
+
+private predicate objStepPruned(TObjFlowNode node1, TObjFlowNode node2) {
+  exists(ObjNode n1, ObjNode n2 |
+    node1 = TObjNode(n1) and
+    node2 = TObjNode(n2) and
+    objStep(n1, n2)
+  )
+  or
+  exists(RefType t, ObjNode n |
+    node1 = TObjType(t) and
+    node2 = TObjNode(n) and
+    source(t, n)
+  )
 }
 
-private predicate stepPlus(Node n1, Node n2) = fastTC(objStepPruned/2)(n1, n2)
+private predicate flowSrc(TObjFlowNode src) { src instanceof TObjType }
+
+private predicate flowSink(TObjFlowNode sink) { exists(ObjNode n | sink = TObjNode(n) and sink(n)) }
+
+private predicate stepPlus(TObjFlowNode n1, TObjFlowNode n2) =
+  doublyBoundedFastTC(objStepPruned/2, flowSrc/1, flowSink/1)(n1, n2)
 
 /**
  * Holds if the qualifier `n` of an `Object.toString()` call might have type `t`.
  */
-pragma[noopt]
-private predicate objType(ObjNode n, RefType t) {
-  exists(ObjNode n2 |
-    sink(n) and
-    (stepPlus(n2, n) or n2 = n) and
-    source(t, n2)
-  )
-}
+private predicate objType(ObjNode n, RefType t) { stepPlus(TObjType(t), TObjNode(n)) }
 
 private VirtualMethodCall objectToString(ObjNode n) {
   result.getQualifier() = n.asExpr() and sink(n)

java/ql/src/Likely Bugs/Resource Leaks/CloseType.qll

@@ -212,33 +212,35 @@ private LocalVariableDecl getCloseableVariable(CloseableInitExpr cie) {
 /**
  * A variable on which a "close" method is called, implicitly or explicitly, directly or indirectly.
  */
-private predicate closeCalled(Variable v) {
+private predicate closeCalled(LocalScopeVariable v) {
   // `close()` is implicitly called on variables declared or referenced
   // in the resources clause of try-with-resource statements.
   exists(TryStmt try | try.getAResourceVariable() = v)
   or
   // Otherwise, there should be an explicit call to a method whose name contains "close".
   exists(MethodCall e |
-    v = getCloseableVariable(_) or v instanceof Parameter or v instanceof LocalVariableDecl
-  |
     e.getMethod().getName().toLowerCase().matches("%close%") and
     exists(VarAccess va | va = v.getAnAccess() |
       e.getQualifier() = va or
       e.getAnArgument() = va
     )
-    or
-    // The "close" call could happen indirectly inside a helper method of unknown name.
-    exists(int i | e.getArgument(i) = v.getAnAccess() |
-      exists(Parameter p, int j | p.getPosition() = j and p.getCallable() = e.getMethod() |
-        closeCalled(p) and i = j
-        or
-        // The helper method could be iterating over a varargs parameter.
-        exists(EnhancedForStmt for | for.getExpr() = p.getAnAccess() |
-          closeCalled(for.getVariable().getVariable())
-        ) and
-        p.isVarargs() and
-        j <= i
-      )
+  )
+  or
+  // The "close" call could happen indirectly inside a helper method of unknown name.
+  exists(Parameter p |
+    closeCalled(p) and p.getAnArgument() = v.getAnAccess() and p.getCallable() instanceof Method
+  )
+  or
+  exists(MethodCall e, int i | e.getArgument(i) = v.getAnAccess() |
+    exists(Parameter p, int j |
+      p.getPosition() = j and p.getCallable() = e.getMethod().getSourceDeclaration()
+    |
+      // The helper method could be iterating over a varargs parameter.
+      exists(EnhancedForStmt for | for.getExpr() = p.getAnAccess() |
+        closeCalled(for.getVariable().getVariable())
+      ) and
+      p.isVarargs() and
+      j <= i
     )
   )
 }

java/ql/test-kotlin1/library-tests/controlflow/basic/strictPostDominance.expected

@@ -208,6 +208,12 @@
 | Test.kt:101:5:103:5 | ... -> ... | Test.kt:101:5:103:5 | <Expr>; |
 | Test.kt:101:5:103:5 | <Expr>; | Test.kt:100:25:110:1 | { ... } |
 | Test.kt:102:9:102:25 | throw ... | Test.kt:101:33:103:5 | { ... } |
+| Test.kt:105:5:109:5 | <Expr>; | Test.kt:100:25:110:1 | { ... } |
+| Test.kt:105:5:109:5 | <Expr>; | Test.kt:101:5:103:5 | ... -> ... |
+| Test.kt:105:5:109:5 | <Expr>; | Test.kt:101:5:103:5 | <Expr>; |
+| Test.kt:105:9:107:5 | ... -> ... | Test.kt:100:25:110:1 | { ... } |
+| Test.kt:105:9:107:5 | ... -> ... | Test.kt:101:5:103:5 | ... -> ... |
+| Test.kt:105:9:107:5 | ... -> ... | Test.kt:101:5:103:5 | <Expr>; |
 | Test.kt:105:9:107:5 | ... -> ... | Test.kt:105:5:109:5 | <Expr>; |
 | Test.kt:106:9:106:29 | <Expr>; | Test.kt:105:20:107:5 | { ... } |
 | Test.kt:108:9:108:29 | <Expr>; | Test.kt:107:27:109:5 | { ... } |

java/ql/test-kotlin2/library-tests/controlflow/basic/strictPostDominance.expected

@@ -208,6 +208,12 @@
 | Test.kt:101:9:103:5 | ... -> ... | Test.kt:100:25:110:1 | { ... } |
 | Test.kt:101:9:103:5 | ... -> ... | Test.kt:101:5:103:5 | <Expr>; |
 | Test.kt:102:9:102:25 | throw ... | Test.kt:101:33:103:5 | { ... } |
+| Test.kt:105:5:109:5 | <Expr>; | Test.kt:100:25:110:1 | { ... } |
+| Test.kt:105:5:109:5 | <Expr>; | Test.kt:101:5:103:5 | <Expr>; |
+| Test.kt:105:5:109:5 | <Expr>; | Test.kt:101:9:103:5 | ... -> ... |
+| Test.kt:105:9:107:5 | ... -> ... | Test.kt:100:25:110:1 | { ... } |
+| Test.kt:105:9:107:5 | ... -> ... | Test.kt:101:5:103:5 | <Expr>; |
+| Test.kt:105:9:107:5 | ... -> ... | Test.kt:101:9:103:5 | ... -> ... |
 | Test.kt:105:9:107:5 | ... -> ... | Test.kt:105:5:109:5 | <Expr>; |
 | Test.kt:106:9:106:29 | <Expr>; | Test.kt:105:20:107:5 | { ... } |
 | Test.kt:108:9:108:29 | <Expr>; | Test.kt:107:27:109:5 | { ... } |

java/ql/test/query-tests/Nullness/B.java

@@ -408,4 +408,32 @@ public void bitwise(Object x, boolean b) {
       x.hashCode(); // NPE
     }
   }
+
+  public void corrCondLoop1(boolean a[]) {
+    Object x = new Object();
+    for (int i = 0; i < a.length; i++) {
+      boolean b = a[i];
+      if (b) {
+        x = null;
+      }
+      if (!b) {
+        x.hashCode(); // NPE - false negative
+      }
+      // flow can loop around from one iteration to the next
+    }
+  }
+
+  public void corrCondLoop2(boolean a[]) {
+    for (int i = 0; i < a.length; i++) {
+      // x is local to the loop iteration and thus cannot loop around and reach the sink
+      Object x = new Object();
+      boolean b = a[i];
+      if (b) {
+        x = null;
+      }
+      if (!b) {
+        x.hashCode(); // OK
+      }
+    }
+  }
 }

rust/ql/lib/codeql/rust/Frameworks.qll

@@ -4,5 +4,4 @@
 
 private import codeql.rust.frameworks.rustcrypto.RustCrypto
 private import codeql.rust.frameworks.Poem
-private import codeql.rust.frameworks.Sqlx
 private import codeql.rust.frameworks.stdlib.Stdlib

rust/ql/lib/codeql/rust/dataflow/internal/ModelsAsData.qll

@@ -68,9 +68,8 @@ extensible predicate sourceModel(
  *
  * For example, `input = Argument[0]` means the first argument of the call.
  *
- * The following kinds are supported:
- *
- * - `sql-injection`: a flow sink for SQL injection.
+ * The sink kinds supported by queries can be found by searching for uses of
+ * the `sinkNode` predicate.
  */
 extensible predicate sinkModel(
   string path, string input, string kind, string provenance, QlBuiltins::ExtensionId madId

rust/ql/lib/codeql/rust/elements/internal/TraitImpl.qll

@@ -36,5 +36,36 @@ module Impl {
       not this.hasGenericParamList() and
       result = 0
     }
+
+    private int nrOfDirectTypeBounds() {
+      result = this.getTypeBoundList().getNumberOfBounds()
+      or
+      not this.hasTypeBoundList() and
+      result = 0
+    }
+
+    /**
+     * Gets the `index`th type bound of this trait, if any.
+     *
+     * This includes type bounds directly on the trait and bounds from any
+     * `where` clauses for `Self`.
+     */
+    TypeBound getTypeBound(int index) {
+      result = this.getTypeBoundList().getBound(index)
+      or
+      exists(WherePred wp |
+        wp = this.getWhereClause().getAPredicate() and
+        wp.getTypeRepr().(PathTypeRepr).getPath().getText() = "Self" and
+        result = wp.getTypeBoundList().getBound(index - this.nrOfDirectTypeBounds())
+      )
+    }
+
+    /**
+     * Gets a type bound of this trait.
+     *
+     * This includes type bounds directly on the trait and bounds from any
+     * `where` clauses for `Self`.
+     */
+    TypeBound getATypeBound() { result = this.getTypeBound(_) }
   }
 }