Java: IPA the CFG.

Author: aschackmull

change-notes/1.20/analysis-java.md

@@ -17,4 +17,15 @@
 
 ## Changes to QL libraries
 
+* The class `ControlFlowNode` (and by extension `BasicBlock`) is no longer
+  directly equatable to `Expr` and `Stmt`. Any queries that have been
+  exploiting these equalities, for example by using casts, will need minor
+  updates in order to fix any compilation errors. Conversions can be inserted
+  in either direction depending on what is most convenient. Available
+  conversions include `Expr.getControlFlowNode()`, `Stmt.getControlFlowNode()`,
+  `ControlFlowNode.asExpr()`, `ControlFlowNode.asStmt()`, and
+  `ControlFlowNode.asCall()`. Exit nodes were until now modelled as a
+  `ControlFlowNode` equal to its enclosing `Callable`; these are now instead
+  modelled by the class `ControlFlow::ExitNode`.
+
 

java/ql/src/Likely Bugs/Comparison/UselessComparisonTest.qll

@@ -35,7 +35,7 @@ predicate uselessTest(ConditionNode s1, BinaryExpr test, boolean testIsTrue) {
     ConditionBlock cb, SsaVariable v, BinaryExpr cond, boolean condIsTrue, int k1, int k2,
     CompileTimeConstantExpr c1, CompileTimeConstantExpr c2
   |
-    s1 = cond and
+    s1.getCondition() = cond and
     cb.getCondition() = cond and
     cond.hasOperands(v.getAUse(), c1) and
     c1.getIntValue() = k1 and

java/ql/src/Likely Bugs/Concurrency/DoubleCheckedLockingWithInitRace.ql

@@ -36,7 +36,7 @@ where
   doubleCheckedLocking(if1, if2, sync, f) and
   a.getEnclosingStmt().getParent*() = if2.getThen() and
   se.getEnclosingStmt().getParent*() = sync.getBlock() and
-  a.(ControlFlowNode).getASuccessor+() = se and
+  a.getControlFlowNode().getASuccessor+() = se.getControlFlowNode() and
   a.getDest().(FieldAccess).getField() = f
 select a,
   "Potential race condition. This assignment to $@ is visible to other threads before the subsequent statements are executed.",

java/ql/src/Likely Bugs/Concurrency/LazyInitStaticField.ql

@@ -63,12 +63,12 @@ class ValidSynchStmt extends Stmt {
       exists(MethodAccess lockAction |
         lockAction.getQualifier() = lockField.getAnAccess() and
         lockAction.getMethod().getName() = "lock" and
-        dominates(lockAction, this)
+        dominates(lockAction.getControlFlowNode(), this.getControlFlowNode())
       ) and
       exists(MethodAccess unlockAction |
         unlockAction.getQualifier() = lockField.getAnAccess() and
         unlockAction.getMethod().getName() = "unlock" and
-        postDominates(unlockAction, this)
+        postDominates(unlockAction.getControlFlowNode(), this.getControlFlowNode())
       )
     )
   }

java/ql/src/Likely Bugs/Concurrency/UnreleasedLock.ql

@@ -58,11 +58,11 @@ class LockType extends RefType {
 }
 
 predicate lockBlock(LockType t, BasicBlock b, int locks) {
-  locks = strictcount(int i | b.getNode(i) = t.getLockAccess())
+  locks = strictcount(int i | b.getNode(i) = t.getLockAccess().getControlFlowNode())
 }
 
 predicate unlockBlock(LockType t, BasicBlock b, int unlocks) {
-  unlocks = strictcount(int i | b.getNode(i) = t.getUnlockAccess())
+  unlocks = strictcount(int i | b.getNode(i) = t.getUnlockAccess().getControlFlowNode())
 }
 
 /**
@@ -89,11 +89,11 @@ predicate failedLock(LockType t, BasicBlock lockblock, BasicBlock exblock) {
   exists(ControlFlowNode lock |
     lock = lockblock.getLastNode() and
     (
-      lock = t.getLockAccess()
+      lock = t.getLockAccess().getControlFlowNode()
       or
       exists(SsaExplicitUpdate lockbool |
         // Using the value of `t.getLockAccess()` ensures that it is a `tryLock` call.
-        lock = lockbool.getAUse() and
+        lock = lockbool.getAUse().getControlFlowNode() and
         lockbool.getDefiningExpr().(VariableAssign).getSource() = t.getLockAccess()
       )
     ) and
@@ -151,7 +151,7 @@ where
   // Restrict results to those methods that actually attempt to unlock.
   t.getUnlockAccess().getEnclosingCallable() = c and
   blockIsLocked(t, src, exit, _) and
-  exit.getLastNode() = c and
-  lock = src.getANode() and
+  exit.getLastNode().(ControlFlow::ExitNode).getEnclosingCallable() = c and
+  lock.getControlFlowNode() = src.getANode() and
   lock = t.getLockAccess()
 select lock, "This lock might not be unlocked or might be locked more times than it is unlocked."

java/ql/src/Likely Bugs/Termination/ConstantLoopCondition.ql

@@ -61,7 +61,7 @@ predicate mainLoopCondition(LoopStmt loop, Expr cond) {
     else loopReentry = cond
   |
     last.getEnclosingStmt().getParent*() = loop.getBody() and
-    last.getASuccessor().(Expr).getParent*() = loopReentry
+    last.getASuccessor().asExpr().getParent*() = loopReentry
   )
 }
 
@@ -75,7 +75,7 @@ where
   // None of the ssa variables in `cond` are updated inside the loop.
   forex(SsaVariable ssa, RValue use | ssa.getAUse() = use and use.getParent*() = cond |
     not ssa.getCFGNode().getEnclosingStmt().getParent*() = loop or
-    ssa.getCFGNode().(Expr).getParent*() = loop.(ForStmt).getAnInit()
+    ssa.getCFGNode().asExpr().getParent*() = loop.(ForStmt).getAnInit()
   ) and
   // And `cond` does not use method calls, field reads, or array reads.
   not exists(MethodAccess ma | ma.getParent*() = cond) and

java/ql/src/Security/CWE/CWE-022/ZipSlip.ql

@@ -130,14 +130,14 @@ predicate validateFilePath(SsaVariable var, Guard check) {
  * Holds if `m` validates its `arg`th parameter.
  */
 predicate validationMethod(Method m, int arg) {
-  exists(Guard check, SsaImplicitInit var, ControlFlowNode exit, ControlFlowNode normexit |
+  exists(Guard check, SsaImplicitInit var, ControlFlow::ExitNode exit, ControlFlowNode normexit |
     validateFilePath(var, check) and
     var.isParameterDefinition(m.getParameter(arg)) and
-    exit = m and
+    exit.getEnclosingCallable() = m and
     normexit.getANormalSuccessor() = exit and
     1 = strictcount(ControlFlowNode n | n.getANormalSuccessor() = exit)
   |
-    check.(ConditionNode).getATrueSuccessor() = exit or
+    check.hasBranchEdge(_, exit.getBasicBlock(), true) or
     check.controls(normexit.getBasicBlock(), true)
   )
 }

java/ql/src/Security/CWE/CWE-833/LockOrderInconsistency.ql

@@ -77,8 +77,8 @@ predicate badReentrantLockOrder(MethodAccess first, MethodAccess second, MethodA
     otherSecond = v1.getLockAction() and
     second = v2.getLockAction() and
     otherFirst = v2.getLockAction() and
-    first.(ControlFlowNode).getASuccessor+() = second and
-    otherFirst.(ControlFlowNode).getASuccessor+() = otherSecond
+    first.getControlFlowNode().getASuccessor+() = second.getControlFlowNode() and
+    otherFirst.getControlFlowNode().getASuccessor+() = otherSecond.getControlFlowNode()
   |
     v1 != v2
   )

java/ql/src/Violations of Best Practice/Declarations/BreakInSwitchCase.ql

@@ -17,8 +17,8 @@ import Common
 from SwitchStmt s, Stmt c
 where
   c = s.getACase() and
-  not c.(ControlFlowNode).getASuccessor() instanceof ConstCase and
-  not c.(ControlFlowNode).getASuccessor() instanceof DefaultCase and
+  not c.getControlFlowNode().getASuccessor().asStmt() instanceof ConstCase and
+  not c.getControlFlowNode().getASuccessor().asStmt() instanceof DefaultCase and
   not s.(Annotatable).suppressesWarningsAbout("fallthrough") and
   mayDropThroughWithoutComment(s, c)
 select c,

java/ql/src/Violations of Best Practice/Declarations/Common.qll

@@ -24,15 +24,15 @@ predicate switchCaseControlFlowPlus(SwitchStmt switch, BasicBlock b1, BasicBlock
   exists(BasicBlock mid |
     switchCaseControlFlowPlus(switch, mid, b2) and
     switchCaseControlFlow(switch, b1, mid) and
-    not mid.getFirstNode() = switch.getACase()
+    not mid.getFirstNode() = switch.getACase().getControlFlowNode()
   )
 }
 
 predicate mayDropThroughWithoutComment(SwitchStmt switch, Stmt switchCase) {
   switchCase = switch.getACase() and
   exists(Stmt other, BasicBlock b1, BasicBlock b2 |
-    b1.getFirstNode() = switchCase and
-    b2.getFirstNode() = other and
+    b1.getFirstNode() = switchCase.getControlFlowNode() and
+    b2.getFirstNode() = other.getControlFlowNode() and
     switchCaseControlFlowPlus(switch, b1, b2) and
     other = switch.getACase() and
     not fallThroughCommented(other)