From 9d59f5034034f40c7d7fbf3a2b849937601ae314 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Tue, 8 Aug 2023 13:37:40 +0200 Subject: [PATCH] Java: Review fixes. --- .../java/dataflow/internal/DataFlowNodes.qll | 4 +- .../dataflow/internal/DataFlowPrivate.qll | 10 ++- .../java/dataflow/internal/DataFlowUtil.qll | 4 +- .../codeql/dataflow/VariableCapture.qll | 84 +++++++++++-------- 4 files changed, 59 insertions(+), 43 deletions(-) diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowNodes.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowNodes.qll index 31602edece4..7ee703808e2 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowNodes.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowNodes.qll @@ -65,7 +65,7 @@ private module Cached { TCollectionContent() or TMapKeyContent() or TMapValueContent() or - TClosureContent(CapturedVariable v) or + TCapturedVariableContent(CapturedVariable v) or TSyntheticFieldContent(SyntheticField s) cached @@ -75,7 +75,7 @@ private module Cached { TCollectionContentApprox() or TMapKeyContentApprox() or TMapValueContentApprox() or - TClosureContentApprox(CapturedVariable v) or + TCapturedVariableContentApprox(CapturedVariable v) or TSyntheticFieldApproxContent() } diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll index c23469ac49d..212232e077a 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll @@ -154,7 +154,7 @@ class CapturedParameter = CaptureInput::CapturedParameter; module CaptureFlow = VariableCapture::Flow; -CaptureFlow::ClosureNode asClosureNode(Node n) { +private CaptureFlow::ClosureNode asClosureNode(Node n) { result = n.(CaptureNode).getSynthesizedCaptureNode() or result.(CaptureFlow::ExprNode).getExpr() = n.asExpr() or result.(CaptureFlow::ExprPostUpdateNode).getExpr() = @@ -165,11 +165,11 @@ CaptureFlow::ClosureNode asClosureNode(Node n) { n } -private predicate captureStoreStep(Node node1, ClosureContent c, Node node2) { +private predicate captureStoreStep(Node node1, CapturedVariableContent c, Node node2) { CaptureFlow::storeStep(asClosureNode(node1), c.getVariable(), asClosureNode(node2)) } -private predicate captureReadStep(Node node1, ClosureContent c, Node node2) { +private predicate captureReadStep(Node node1, CapturedVariableContent c, Node node2) { CaptureFlow::readStep(asClosureNode(node1), c.getVariable(), asClosureNode(node2)) } @@ -565,7 +565,9 @@ ContentApprox getContentApprox(Content c) { or c instanceof MapValueContent and result = TMapValueContentApprox() or - exists(CapturedVariable v | c = TClosureContent(v) and result = TClosureContentApprox(v)) + exists(CapturedVariable v | + c = TCapturedVariableContent(v) and result = TCapturedVariableContentApprox(v) + ) or c instanceof SyntheticFieldContent and result = TSyntheticFieldApproxContent() } 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 aeb44d892f6..080724d8f0c 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll @@ -275,10 +275,10 @@ class MapValueContent extends Content, TMapValueContent { } /** A captured variable. */ -class ClosureContent extends Content, TClosureContent { +class CapturedVariableContent extends Content, TCapturedVariableContent { CapturedVariable v; - ClosureContent() { this = TClosureContent(v) } + CapturedVariableContent() { this = TCapturedVariableContent(v) } CapturedVariable getVariable() { result = v } diff --git a/shared/dataflow/codeql/dataflow/VariableCapture.qll b/shared/dataflow/codeql/dataflow/VariableCapture.qll index 38ad32ddbd6..0b69f1ffcdd 100644 --- a/shared/dataflow/codeql/dataflow/VariableCapture.qll +++ b/shared/dataflow/codeql/dataflow/VariableCapture.qll @@ -151,6 +151,11 @@ signature module OutputSig { /** * A data flow node that we need to reference in the step relations for * captured variables. + * + * Note that only the `SynthesizedCaptureNode` subclass is expected to be + * added as additional nodes in `DataFlow::Node`. The other subclasses are + * expected to already be present and are included here in order to reference + * them in the step relations. */ class ClosureNode; @@ -229,53 +234,65 @@ module Flow implements OutputSig { private import Input additional module ConsistencyChecks { - private predicate relevantExpr(Expr e) { - e instanceof VariableRead or - any(VariableWrite vw).getSource() = e or - e instanceof ClosureExpr or - any(ClosureExpr ce).hasAliasedAccess(e) + final private class FinalExpr = Expr; + + private class RelevantExpr extends FinalExpr { + RelevantExpr() { + this instanceof VariableRead or + any(VariableWrite vw).getSource() = this or + this instanceof ClosureExpr or + any(ClosureExpr ce).hasAliasedAccess(this) + } } - private predicate relevantBasicBlock(BasicBlock bb) { - exists(Expr e | relevantExpr(e) and e.hasCfgNode(bb, _)) - or - exists(VariableWrite vw | vw.hasCfgNode(bb, _)) + final private class FinalBasicBlock = BasicBlock; + + private class RelevantBasicBlock extends FinalBasicBlock { + RelevantBasicBlock() { + exists(RelevantExpr e | e.hasCfgNode(this, _)) + or + exists(VariableWrite vw | vw.hasCfgNode(this, _)) + } } - private predicate relevantCallable(Callable c) { - exists(BasicBlock bb | relevantBasicBlock(bb) and bb.getEnclosingCallable() = c) - or - exists(CapturedVariable v | v.getCallable() = c) - or - exists(ClosureExpr ce | ce.hasBody(c)) + final private class FinalCallable = Callable; + + private class RelevantCallable extends FinalCallable { + RelevantCallable() { + exists(RelevantBasicBlock bb | bb.getEnclosingCallable() = this) + or + exists(CapturedVariable v | v.getCallable() = this) + or + exists(ClosureExpr ce | ce.hasBody(this)) + } } query predicate uniqueToString(string msg, int n) { exists(string elem | - n = strictcount(BasicBlock bb | relevantBasicBlock(bb) and not exists(bb.toString())) and + n = strictcount(RelevantBasicBlock bb | not exists(bb.toString())) and elem = "BasicBlock" or n = strictcount(CapturedVariable v | not exists(v.toString())) and elem = "CapturedVariable" or - n = strictcount(Expr e | relevantExpr(e) and not exists(e.toString())) and elem = "Expr" + n = strictcount(RelevantExpr e | not exists(e.toString())) and elem = "Expr" or - n = strictcount(Callable c | relevantCallable(c) and not exists(c.toString())) and + n = strictcount(RelevantCallable c | not exists(c.toString())) and elem = "Callable" | msg = n + " " + elem + "(s) are missing toString" ) or exists(string elem | - n = strictcount(BasicBlock bb | relevantBasicBlock(bb) and 2 <= strictcount(bb.toString())) and + n = strictcount(RelevantBasicBlock bb | 2 <= strictcount(bb.toString())) and elem = "BasicBlock" or n = strictcount(CapturedVariable v | 2 <= strictcount(v.toString())) and elem = "CapturedVariable" or - n = strictcount(Expr e | relevantExpr(e) and 2 <= strictcount(e.toString())) and + n = strictcount(RelevantExpr e | 2 <= strictcount(e.toString())) and elem = "Expr" or - n = strictcount(Callable c | relevantCallable(c) and 2 <= strictcount(c.toString())) and + n = strictcount(RelevantCallable c | 2 <= strictcount(c.toString())) and elem = "Callable" | msg = n + " " + elem + "(s) have multiple toStrings" @@ -283,7 +300,7 @@ module Flow implements OutputSig { } query predicate uniqueEnclosingCallable(BasicBlock bb, string msg) { - relevantBasicBlock(bb) and + bb instanceof RelevantBasicBlock and ( msg = "BasicBlock has no enclosing callable" and not exists(bb.getEnclosingCallable()) or @@ -293,19 +310,19 @@ module Flow implements OutputSig { } query predicate uniqueDominator(BasicBlock bb, string msg) { - relevantBasicBlock(bb) and + bb instanceof RelevantBasicBlock and msg = "BasicBlock has multiple immediate dominators" and 2 <= strictcount(getImmediateBasicBlockDominator(bb)) } query predicate localDominator(BasicBlock bb, string msg) { - relevantBasicBlock(bb) and + bb instanceof RelevantBasicBlock and msg = "BasicBlock has non-local dominator" and bb.getEnclosingCallable() != getImmediateBasicBlockDominator(bb).getEnclosingCallable() } query predicate localSuccessor(BasicBlock bb, string msg) { - relevantBasicBlock(bb) and + bb instanceof RelevantBasicBlock and msg = "BasicBlock has non-local successor" and bb.getEnclosingCallable() != getABasicBlockSuccessor(bb).getEnclosingCallable() } @@ -322,7 +339,7 @@ module Flow implements OutputSig { } query predicate uniqueLocation(Expr e, string msg) { - relevantExpr(e) and + e instanceof RelevantExpr and ( msg = "Expr has no location" and not exists(e.getLocation()) or @@ -331,7 +348,7 @@ module Flow implements OutputSig { } query predicate uniqueCfgNode(Expr e, string msg) { - relevantExpr(e) and + e instanceof RelevantExpr and ( msg = "Expr has no cfg node" and not e.hasCfgNode(_, _) or @@ -413,7 +430,7 @@ module Flow implements OutputSig { } query predicate uniqueCallableLocation(Callable c, string msg) { - relevantCallable(c) and + c instanceof RelevantCallable and ( msg = "Callable has no location" and not exists(c.getLocation()) or @@ -622,13 +639,12 @@ module Flow implements OutputSig { entryBlock(bb) and pragma[only_bind_out](bb.getEnclosingCallable()) = c and i = - -1 + - min(int j | + min(int j | j = 1 or captureRead(_, bb, j, _, _) or captureWrite(_, bb, j, _, _) or synthRead(_, bb, j, _, _) - ) + ) - 1 | cc = TThis(c) or @@ -637,9 +653,7 @@ module Flow implements OutputSig { } private module CaptureSsaInput implements Ssa::InputSig { - class BasicBlock instanceof Input::BasicBlock { - string toString() { result = super.toString() } - } + final class BasicBlock = Input::BasicBlock; BasicBlock getImmediateBasicBlockDominator(BasicBlock bb) { result = Input::getImmediateBasicBlockDominator(bb) @@ -666,7 +680,7 @@ module Flow implements OutputSig { predicate variableRead(BasicBlock bb, int i, SourceVariable cc, boolean certain) { ( - synthThisQualifier(bb, i) and cc = TThis(bb.(Input::BasicBlock).getEnclosingCallable()) + synthThisQualifier(bb, i) and cc = TThis(bb.getEnclosingCallable()) or exists(CapturedVariable v | cc = TVariable(v) | captureRead(v, bb, i, true, _) or synthRead(v, bb, i, true, _)