From 3d980b168476a40cc4d00580243e2a1b2b7bc174 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Wed, 15 Nov 2023 15:59:21 +0000 Subject: [PATCH] Switch to using VariableAssign for instanceof and switch dataflow --- java/ql/lib/semmle/code/java/Expr.qll | 59 ++++++++++++++++--- .../semmle/code/java/dataflow/TypeFlow.qll | 16 ----- .../java/dataflow/internal/DataFlowUtil.qll | 11 ++-- .../code/java/dispatch/DispatchFlow.qll | 14 ----- .../lib/semmle/code/java/dispatch/ObjFlow.qll | 14 ----- 5 files changed, 58 insertions(+), 56 deletions(-) diff --git a/java/ql/lib/semmle/code/java/Expr.qll b/java/ql/lib/semmle/code/java/Expr.qll index 27bb2c4f1e2..f849fe62f4e 100644 --- a/java/ql/lib/semmle/code/java/Expr.qll +++ b/java/ql/lib/semmle/code/java/Expr.qll @@ -1682,15 +1682,51 @@ class LocalVariableDeclExpr extends Expr, @localvariabledeclexpr { /** Holds if this is a declaration stemming from a pattern instanceof expression. */ predicate hasAssociatedInstanceOfExpr() { exists(this.getAssociatedInstanceOfExpr()) } - /** Gets the initializer expression of this local variable declaration expression, if any. */ + /** + * Gets the initializer expression of this local variable declaration expression, if any. + * + * Note this applies specifically to a syntactic initialization like `T varname = init`; + * to include also `e instanceof T varname` and `switch(e) ... case T varname`, which both + * have the effect of initializing `varname` to a known local expression without using + * that syntax, use `getInitOrPatternSource`. + */ Expr getInit() { result.isNthChildOf(this, 0) } + /** + * Gets the local expression that initializes this variable declaration, if any. + * + * Note this includes explicit `T varname = init;`, as well as `e instanceof T varname` + * and `switch(e) ... case T varname`. To get only explicit initializers, use `getInit`. + * + * Note that record pattern variables like `e instance of T Record(T varname)` do not have + * either an explicit initializer or a pattern source. + */ + Expr getInitOrPatternSource() { + result = this.getInit() + or + exists(SwitchStmt switch | + result = switch.getExpr() and + this = switch.getAPatternCase().getPattern().asBindingPattern() + ) + or + exists(SwitchExpr switch | + result = switch.getExpr() and + this = switch.getAPatternCase().getPattern().asBindingPattern() + ) + or + exists(InstanceOfExpr ioe | + result = ioe.getExpr() and + this = ioe.getPattern().asBindingPattern() + ) + } + /** Holds if this variable declaration implicitly initializes the variable. */ predicate hasImplicitInit() { - exists(CatchClause cc | cc.getVariable() = this) or - exists(EnhancedForStmt efs | efs.getVariable() = this) or - this.hasAssociatedSwitch() or - this.hasAssociatedInstanceOfExpr() + exists(CatchClause cc | cc.getVariable() = this) + or + exists(EnhancedForStmt efs | efs.getVariable() = this) + or + this.getParent() instanceof RecordPatternExpr } /** Gets a printable representation of this expression. */ @@ -1699,6 +1735,13 @@ class LocalVariableDeclExpr extends Expr, @localvariabledeclexpr { override string getAPrimaryQlClass() { result = "LocalVariableDeclExpr" } } +/** A local variable declaration that occurs within a record pattern. */ +class RecordBindingVariableExpr extends LocalVariableDeclExpr { + RecordBindingVariableExpr() { + this.getParent() instanceof RecordPatternExpr + } +} + /** An update of a variable or an initialization of the variable. */ class VariableUpdate extends Expr { VariableUpdate() { @@ -1727,12 +1770,12 @@ class VariableAssign extends VariableUpdate { /** * Gets the source (right-hand side) of this assignment, if any. * - * An initialization in a `CatchClause` or `EnhancedForStmt` is implicit and - * does not have a source. + * An initialization in a `CatchClause`, `EnhancedForStmt` or `RecordPatternExpr` + * is implicit and does not have a source. */ Expr getSource() { result = this.(AssignExpr).getSource() or - result = this.(LocalVariableDeclExpr).getInit() + result = this.(LocalVariableDeclExpr).getInitOrPatternSource() } } diff --git a/java/ql/lib/semmle/code/java/dataflow/TypeFlow.qll b/java/ql/lib/semmle/code/java/dataflow/TypeFlow.qll index dfbf957f18c..543bc5d3f44 100644 --- a/java/ql/lib/semmle/code/java/dataflow/TypeFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/TypeFlow.qll @@ -116,22 +116,6 @@ private predicate step(TypeFlowNode n1, TypeFlowNode n2) { n2.asSsa().(BaseSsaUpdate).getDefiningExpr().(VariableAssign).getSource() = n1.asExpr() or n2.asSsa().(BaseSsaImplicitInit).captures(n1.asSsa()) - or - exists(PatternCase pc, LocalVariableDeclExpr patternVar | - patternVar = pc.getPattern().asBindingPattern() and - n2.asSsa().(BaseSsaUpdate).getDefiningExpr() = patternVar and - ( - pc.getSwitch().getExpr() = n1.asExpr() - or - pc.getSwitchExpr().getExpr() = n1.asExpr() - ) - ) - or - exists(InstanceOfExpr ioe, LocalVariableDeclExpr patternVar | - patternVar = ioe.getPattern().asBindingPattern() and - n2.asSsa().(BaseSsaUpdate).getDefiningExpr() = patternVar and - ioe.getExpr() = n1.asExpr() - ) } /** diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll index 3a2cf884bed..0fa9412cb5b 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll @@ -191,11 +191,14 @@ predicate simpleAstFlowStep(Expr e1, Expr e2) { or e2.(WhenExpr).getBranch(_).getAResult() = e1 or - exists(SwitchExpr se | e1 = se.getExpr() and e2 = se.getACase().(PatternCase).getPattern()) + // In the following three cases only record patterns need this flow edge, leading from the bound instanceof + // or switch tested expression to a record pattern that will read its fields. Simple binding patterns are + // handled via VariableAssign.getSource instead. + exists(SwitchExpr se | e1 = se.getExpr() and e2 = se.getACase().(PatternCase).getPattern().asRecordPattern()) or - exists(SwitchStmt ss | e1 = ss.getExpr() and e2 = ss.getACase().(PatternCase).getPattern()) + exists(SwitchStmt ss | e1 = ss.getExpr() and e2 = ss.getACase().(PatternCase).getPattern().asRecordPattern()) or - exists(InstanceOfExpr ioe | e1 = ioe.getExpr() and e2 = ioe.getPattern()) + exists(InstanceOfExpr ioe | e1 = ioe.getExpr() and e2 = ioe.getPattern().asRecordPattern()) } private predicate simpleLocalFlowStep0(Node node1, Node node2) { @@ -204,7 +207,7 @@ private predicate simpleLocalFlowStep0(Node node1, Node node2) { exists(SsaExplicitUpdate upd | upd.getDefiningExpr().(VariableAssign).getSource() = node1.asExpr() or upd.getDefiningExpr().(AssignOp) = node1.asExpr() or - upd.getDefiningExpr().(PatternExpr).asBindingPattern() = node1.asExpr() + upd.getDefiningExpr().(RecordBindingVariableExpr) = node1.asExpr() | node2.asExpr() = upd.getAFirstUse() and not capturedVariableRead(node2) diff --git a/java/ql/lib/semmle/code/java/dispatch/DispatchFlow.qll b/java/ql/lib/semmle/code/java/dispatch/DispatchFlow.qll index ae7ef09275a..82bda033bc6 100644 --- a/java/ql/lib/semmle/code/java/dispatch/DispatchFlow.qll +++ b/java/ql/lib/semmle/code/java/dispatch/DispatchFlow.qll @@ -167,20 +167,6 @@ private module TypeTrackingSteps { def.(BaseSsaUpdate).getDefiningExpr().(VariableAssign).getSource() = n1.asExpr() or def.(BaseSsaImplicitInit).isParameterDefinition(n1.asParameter()) - or - exists(PatternCase pc | - pc.getPattern().asBindingPattern() = def.(BaseSsaUpdate).getDefiningExpr() and - ( - pc.getSwitch().getExpr() = n1.asExpr() - or - pc.getSwitchExpr().getExpr() = n1.asExpr() - ) - ) - or - exists(InstanceOfExpr ioe | - ioe.getPattern().asBindingPattern() = def.(BaseSsaUpdate).getDefiningExpr() and - ioe.getExpr() = n1.asExpr() - ) | v.getAnUltimateDefinition() = def and v.getAUse() = n2.asExpr() diff --git a/java/ql/lib/semmle/code/java/dispatch/ObjFlow.qll b/java/ql/lib/semmle/code/java/dispatch/ObjFlow.qll index 446885252f6..293ba894fdf 100644 --- a/java/ql/lib/semmle/code/java/dispatch/ObjFlow.qll +++ b/java/ql/lib/semmle/code/java/dispatch/ObjFlow.qll @@ -80,20 +80,6 @@ private predicate step(Node n1, Node n2) { for.getVariable() = def.(BaseSsaUpdate).getDefiningExpr() and for.getExpr() = n1.asExpr() ) - or - exists(PatternCase pc | - pc.getPattern().asBindingPattern() = def.(BaseSsaUpdate).getDefiningExpr() and - ( - pc.getSwitch().getExpr() = n1.asExpr() - or - pc.getSwitchExpr().getExpr() = n1.asExpr() - ) - ) - or - exists(InstanceOfExpr ioe | - ioe.getPattern().asBindingPattern() = def.(BaseSsaUpdate).getDefiningExpr() and - ioe.getExpr() = n1.asExpr() - ) | v.getAnUltimateDefinition() = def and v.getAUse() = n2.asExpr()