From 8aec87ea57a889ce87abf619b196e878a2256a83 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 23 Aug 2023 14:56:13 +0200 Subject: [PATCH 1/4] Update VariableCapture.qll --- .../codeql/dataflow/VariableCapture.qll | 45 ++++++++++++++----- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/shared/dataflow/codeql/dataflow/VariableCapture.qll b/shared/dataflow/codeql/dataflow/VariableCapture.qll index 8cee2b4c042..4ed3a0c3996 100644 --- a/shared/dataflow/codeql/dataflow/VariableCapture.qll +++ b/shared/dataflow/codeql/dataflow/VariableCapture.qll @@ -391,16 +391,6 @@ module Flow implements OutputSig { msg = "ClosureExpr has no body" and not ce.hasBody(_) } - query predicate closureAliasMustBeLocal(ClosureExpr ce, Expr access, string msg) { - exists(BasicBlock bb1, BasicBlock bb2 | - ce.hasAliasedAccess(access) and - ce.hasCfgNode(bb1, _) and - access.hasCfgNode(bb2, _) and - bb1.getEnclosingCallable() != bb2.getEnclosingCallable() and - msg = "ClosureExpr has non-local alias - these are ignored" - ) - } - private predicate astClosureParent(Callable closure, Callable parent) { exists(ClosureExpr ce, BasicBlock bb | ce.hasBody(closure) and ce.hasCfgNode(bb, _) and parent = bb.getEnclosingCallable() @@ -440,7 +430,6 @@ module Flow implements OutputSig { n = strictcount(VariableWrite vw | localWriteStep(vw, msg)) or n = strictcount(VariableRead vr | uniqueReadVariable(vr, msg)) or n = strictcount(ClosureExpr ce | closureMustHaveBody(ce, msg)) or - n = strictcount(ClosureExpr ce, Expr access | closureAliasMustBeLocal(ce, access, msg)) or n = strictcount(CapturedVariable v, Callable c | variableAccessAstNesting(v, c, msg)) or n = strictcount(Callable c | uniqueCallableLocation(c, msg)) } @@ -518,10 +507,42 @@ module Flow implements OutputSig { } /** Gets the enclosing callable of `ce`. */ - private Callable closureExprGetCallable(ClosureExpr ce) { + private Callable closureExprGetEnclosingCallable(ClosureExpr ce) { exists(BasicBlock bb | ce.hasCfgNode(bb, _) and result = bb.getEnclosingCallable()) } + /** Gets the enclosing callable of `inner`. */ + pragma[nomagic] + private Callable callableGetEnclosingCallable(Callable inner) { + exists(ClosureExpr closure | + closure.hasBody(inner) and + result = closureExprGetEnclosingCallable(closure) + ) + } + + /** + * Gets a callable that contains a reference to `ce` into which `ce` could be inlined without + * bringing any variables out of scope. + * + * If `ce` was to be inlined into that reference, the resulting callable + * would become the enclosing callable, and thus capture the same variables as `ce`. + * In some sense, we model captured aliases as if this inlining has happened. + */ + private Callable closureExprGetAReferencingCallable(ClosureExpr ce) { + exists(Expr expr, BasicBlock bb | + ce.hasAliasedAccess(expr) and + expr.hasCfgNode(bb, _) and + result = bb.getEnclosingCallable() and + // The reference to `ce` is allowed to occur in a more deeply nested context + closureExprGetEnclosingCallable(ce) = callableGetEnclosingCallable*(result) + ) + } + + /** Gets a callable containing `ce` or one of its aliases. */ + private Callable closureExprGetCallable(ClosureExpr ce) { + result = [closureExprGetEnclosingCallable(ce), closureExprGetAReferencingCallable(ce)] + } + /** * Holds if `v` is available in `c` through capture. This can either be due to * an explicit variable reference or through the construction of a closure From ee1b3fd7e9558254de1b5405886840db5a47cd81 Mon Sep 17 00:00:00 2001 From: Asger F Date: Wed, 23 Aug 2023 14:52:04 +0200 Subject: [PATCH 2/4] Java: update test after VariableCapture.qll change --- java/ql/test/library-tests/dataflow/capture/B.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/test/library-tests/dataflow/capture/B.java b/java/ql/test/library-tests/dataflow/capture/B.java index 0d43a0dba17..8909358b8a4 100644 --- a/java/ql/test/library-tests/dataflow/capture/B.java +++ b/java/ql/test/library-tests/dataflow/capture/B.java @@ -210,7 +210,7 @@ public class B { r1.run(); }; r2.run(); - sink(out.get(0)); // $ MISSING: hasValueFlow=double.capture.out + sink(out.get(0)); // $ hasValueFlow=double.capture.out } void testEnhancedForStmtCapture() { From b424f3fe83c5189d1d9ea4791bc8a427b668d852 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 24 Aug 2023 11:11:45 +0200 Subject: [PATCH 3/4] Update a comment to be more accurate --- shared/dataflow/codeql/dataflow/VariableCapture.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shared/dataflow/codeql/dataflow/VariableCapture.qll b/shared/dataflow/codeql/dataflow/VariableCapture.qll index 4ed3a0c3996..5b8bcb6f809 100644 --- a/shared/dataflow/codeql/dataflow/VariableCapture.qll +++ b/shared/dataflow/codeql/dataflow/VariableCapture.qll @@ -583,7 +583,7 @@ module Flow implements OutputSig { /** * Holds if `access` is a reference to `ce` evaluated in the `i`th node of `bb`. - * The reference is restricted to be in the same callable as `ce` as a + * The reference is restricted to be nested within the same callable as `ce` as a * precaution, even though this is expected to hold for all the given aliased * accesses. */ From 1286235773d8c781d4fa2097c4e7e8485fb34f51 Mon Sep 17 00:00:00 2001 From: Asger F Date: Thu, 24 Aug 2023 13:58:33 +0200 Subject: [PATCH 4/4] Address review comments --- .../codeql/dataflow/VariableCapture.qll | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/shared/dataflow/codeql/dataflow/VariableCapture.qll b/shared/dataflow/codeql/dataflow/VariableCapture.qll index 5b8bcb6f809..c94e06b8731 100644 --- a/shared/dataflow/codeql/dataflow/VariableCapture.qll +++ b/shared/dataflow/codeql/dataflow/VariableCapture.qll @@ -391,6 +391,17 @@ module Flow implements OutputSig { msg = "ClosureExpr has no body" and not ce.hasBody(_) } + query predicate closureAliasMustBeInSameScope(ClosureExpr ce, Expr access, string msg) { + exists(BasicBlock bb1, BasicBlock bb2 | + ce.hasAliasedAccess(access) and + ce.hasCfgNode(bb1, _) and + access.hasCfgNode(bb2, _) and + not bb1.getEnclosingCallable() = callableGetEnclosingCallable*(bb2.getEnclosingCallable()) and + msg = + "ClosureExpr has an alias outside the scope of its enclosing callable - these are ignored" + ) + } + private predicate astClosureParent(Callable closure, Callable parent) { exists(ClosureExpr ce, BasicBlock bb | ce.hasBody(closure) and ce.hasCfgNode(bb, _) and parent = bb.getEnclosingCallable() @@ -430,6 +441,7 @@ module Flow implements OutputSig { n = strictcount(VariableWrite vw | localWriteStep(vw, msg)) or n = strictcount(VariableRead vr | uniqueReadVariable(vr, msg)) or n = strictcount(ClosureExpr ce | closureMustHaveBody(ce, msg)) or + n = strictcount(ClosureExpr ce, Expr access | closureAliasMustBeInSameScope(ce, access, msg)) or n = strictcount(CapturedVariable v, Callable c | variableAccessAstNesting(v, c, msg)) or n = strictcount(Callable c | uniqueCallableLocation(c, msg)) } @@ -521,7 +533,7 @@ module Flow implements OutputSig { } /** - * Gets a callable that contains a reference to `ce` into which `ce` could be inlined without + * Gets a callable that contains `ce`, or a reference to `ce` into which `ce` could be inlined without * bringing any variables out of scope. * * If `ce` was to be inlined into that reference, the resulting callable @@ -529,6 +541,8 @@ module Flow implements OutputSig { * In some sense, we model captured aliases as if this inlining has happened. */ private Callable closureExprGetAReferencingCallable(ClosureExpr ce) { + result = closureExprGetEnclosingCallable(ce) + or exists(Expr expr, BasicBlock bb | ce.hasAliasedAccess(expr) and expr.hasCfgNode(bb, _) and @@ -538,11 +552,6 @@ module Flow implements OutputSig { ) } - /** Gets a callable containing `ce` or one of its aliases. */ - private Callable closureExprGetCallable(ClosureExpr ce) { - result = [closureExprGetEnclosingCallable(ce), closureExprGetAReferencingCallable(ce)] - } - /** * Holds if `v` is available in `c` through capture. This can either be due to * an explicit variable reference or through the construction of a closure @@ -555,7 +564,7 @@ module Flow implements OutputSig { ) or exists(ClosureExpr ce | - c = closureExprGetCallable(ce) and + c = closureExprGetAReferencingCallable(ce) and closureCaptures(ce, v) and c != v.getCallable() ) @@ -587,11 +596,11 @@ module Flow implements OutputSig { * precaution, even though this is expected to hold for all the given aliased * accesses. */ - private predicate localClosureAccess(ClosureExpr ce, Expr access, BasicBlock bb, int i) { + private predicate localOrNestedClosureAccess(ClosureExpr ce, Expr access, BasicBlock bb, int i) { ce.hasAliasedAccess(access) and access.hasCfgNode(bb, i) and pragma[only_bind_out](bb.getEnclosingCallable()) = - pragma[only_bind_out](closureExprGetCallable(ce)) + pragma[only_bind_out](closureExprGetAReferencingCallable(ce)) } /** @@ -608,7 +617,7 @@ module Flow implements OutputSig { exists(ClosureExpr ce | closureCaptures(ce, v) | ce.hasCfgNode(bb, i) and ce = closure or - localClosureAccess(ce, closure, bb, i) + localOrNestedClosureAccess(ce, closure, bb, i) ) and if v.getCallable() != bb.getEnclosingCallable() then topScope = false else topScope = true }