Python: Basic implementation of variable capture #14944

yoff · 2023-11-28T23:16:40Z

Instantiates the shared library to handle variable capture in basic cases.

The last commit is superseded by #15080. Ideally that should be merged first, and this should be rebased. But I think that this can be meaningfully reviewed already now.

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll

This provides variable capture in standard situations: - nested functions - lambdas There are some deficiencies: - we do not yet handle objects capturing variables. - we do not handle variables captured via the `nonlocal` keyword. This should be solved at the AST level, though, and then it should "just work". There are still inconsistencies in the case where a `SynthesizedCaptureNode` has a comprehensions as its enclosing callable. In this case, `TFunction(cn.getEnclosingCallable())` is not defined and so getEnclosingCallable does not exist for the `CaptureNode`.

TODO: The member predicate `LibraryLambdaMethod::getACall` is currently too permissive. Ideally, we would have `libraryCallHasLambdaArg` as in Ruby. But even a more precise `libraryCall` predicate might be fine.

On the small test project, this reduces the number of instances from 285 to 22.

as in Ruby

These are the labmda self references. This is similar to how `BlockParameterArgumentNode` is excluded for Ruby. It is important that we restrict `call` in this logic. Otherwise, we get a cartesian product and the consistency check runs for a very long time...

This depends on the extractor fix

this must have been lost in my clean-up rebase.

RasmusWL

I've reviewed the first 3 commits so far. Since the second commit is the major part of this PR, I'm submitting my review-in-progress comments so you can start looking into them, and we can have further discussions as needed 😊

Lambda self naming

My immediate thoughts are is "lambda self" a good name for this? I know in some languages you can refer to the function itself with this/self inside a function, so it might make sense there... but since self already has established semantics in Python, I'm not convinced about it.

How about something like "capture context"? or potentially CapturedVariablesArgument + CapturedVariablesParameter ... makes it very clear what this is about. (potentially with Synth in there as suggested in inline comments, so it becomes SynthCapturedVariablesArgument + SynthCapturedVariablesParameter).

Variable capture code

The meaty bits has been added to DataFlowPrivate.qll. How about we move it into it's own file instead? (for a bit more modularity 😊)

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowDispatch.qll

RasmusWL · 2023-12-14T15:36:34Z

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowDispatch.qll

+  override Node getPreUpdateNode() { result = pre }
+}
+
+class CaptureArgumentNode extends CfgNode, ArgumentNode {


I'm surprised to see there isn't a TSynthCaptureArgumentNode, like we have for TSynthDictSplatArgumentNode -- is there some underlying reason for this I'm not aware of?

I'm raising this since I think adding synthetic nodes is a safer approach. If all such synthetic nodes extended CfgNode, we would get cross-talk due to the shared super-class.

Good point, I will do that.

Hm, that did not work out. I ran into problems with non-monotonic recursion; both defining the node in a restricted way (say not a new node for every CallNode) and also adding flow into it. I will note that other languages do not have a synthetic node here, so we can probably get away with it too for now.

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll

RasmusWL · 2023-12-14T16:57:34Z

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll

+    // simpleAstFlowStep(e1, e2)
+    // or


How about this commented out code?

Turned into a proper comment.

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowImplConsistency.qll

RasmusWL · 2023-12-14T17:21:04Z

python/ql/consistency-queries/DataFlowConsistency.ql

+    arg = call.getArgument(_) and
+    arg instanceof CaptureArgumentNode


Please add a comment as to why this is needed

Thanks. I don't quite understand why it isn't covered by

exists(DataFlowCall other, CallNode cfgCall | other != call | call.getNode() = cfgCall and other.getNode() = cfgCall and isArgumentNode(arg, call, _) and isArgumentNode(arg, other, _) )

I've tried to come up with an example where the same ("lambda self") argument would be used for multiple calls:

def outer(): var = 42 def foo(): print("foo", var) def bar(): print("bar", var) var = 43 f = foo if <cond> else bar f()

In the f() call we would either call foo or bar, who would have captured var, and with the changes in this PR, we should be able to correctly identify that the value printed is 43.

There will be TWO DataFlowCall for f(), one that targets foo and one that targets bar.

There will be ONE CaptureArgumentNode which is based on the cfg-node for .getFunction, meaning f in f().

From the way I see it, that should 100% be covered by the exception above.

Interplay with self argument

In the char-pred of ExtractedArgumentNode we have:

codeql/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll

Lines 310 to 311 in f96c52e

// and self arguments

this.asCfgNode() = any(CallNode c).getFunction().(AttrNode).getObject()

~~So RIGHT NOW it's the case that forall(CaptureArgumentNode arg | | arg instanceof ExtractedArgumentNode) holds.~~

☝️ certainly wasn't true, had overlooked the AttrNode part 🙈

The exclusion case you reference require the two calls to be the same CallNode, I think?

You were right, it does not seem to be needed. It is removed in d6544cc without any consistency failures appearing..

Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

also improve comment

I chose `category: majorAnalysis`, the description is "An API has changed in a way that may affect the results produced by a query that consumes the API." The API in question here is `flowPath` which is used by all our data flow queries.

RasmusWL

Overall I think this looks ok to merge now, given that the performance in the new experiments looks OK.

I just hope we will not need more synthetic parameters related to captured variables in the future 😁

Well if we do, you know who to point your finger at 😂

Anyway, thanks for enduring me in the naming thing 🙏 (and all the other fixes)

python/ql/lib/change-notes/2023-12-18-support-variable-capture.md

RasmusWL · 2023-12-19T09:01:14Z

python/ql/consistency-queries/DataFlowConsistency.ql

+    arg = call.getArgument(_) and
+    arg instanceof CaptureArgumentNode


python/ql/lib/semmle/python/dataflow/new/internal/VariableCapture.qll

Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

RasmusWL · 2023-12-19T09:10:36Z

New call graph edges looks good to me (although I've not 100% understood why we gain them).

The new py/meta/summarized-callable-call-sites results looks suspicious to me, so looking forward to a breakdown of those results from you 😊

yoff · 2023-12-19T13:07:20Z

The new py/meta/summarized-callable-call-sites results looks suspicious to me, so looking forward to a breakdown of those results from you 😊

Indeed, those look wrong. I believe they are surfaced now because of the variable capture parameter, but ultimately caused by this diff from #14777. We now confuse all entry definitions into one entry node. I have pushed a fix which is to jump past the entry node and directly to all uses. I have started a DCA run to check that this does not kill performance.

yoff · 2023-12-19T13:44:52Z

I have pushed a fix which is to jump past the entry node and directly to all uses.

That fix caused test failures. It also may not be right for type tracking, where we would ideally just want a single node for each captured variable. I think the right fix is to reintroduce ScopeEntryDefinitions as data flow nodes for this purpose.

yoff · 2023-12-19T20:43:05Z

It also may not be right for type tracking, where we would ideally just want a single node for each captured variable

Performance evaluations confirmed this. We saw an explosion for some projects (worst was 35x analysis time), so it is indeed important to have a single target. I have rebased this last commit away. A proper fix will be implemented in a separate PR (where it can be tested on its own). Then this PR can merge from main and everything should look reasonable again.

…aptured-variables-basic

hvitved

Overall LGTM. I have a few questions.

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowDispatch.qll

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPublic.qll

python/ql/lib/semmle/python/dataflow/new/internal/FlowSummaryImpl.qll

python/ql/test/experimental/dataflow/variable-capture/dataflow-capture-consistency.ql

hvitved · 2023-12-20T12:55:58Z

python/ql/test/experimental/dataflow/variable-capture/nonlocal.py

@@ -83,7 +83,7 @@ def m():
            sinkO2 = tainted
        m()
    captureOut2()
-    SINK(sinkO2) #$ MISSING:captured
+    SINK(sinkO2) #$ captured


Unless you already have tests for this, I suggest adding tests that mix variable capture flow and field flow, see e.g. https://github.com/github/codeql/pull/11725/files#diff-a1db6555e7c41714eebba0d4e0f95f8cfefbdebcd13edd6135f070b08e53e1a6R56-R81

We have some tests mixing capture flow and content flow (dict and list but not fields).

Cool, out of interest, where are they?

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowDispatch.qll

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll

hvitved · 2023-12-20T14:27:38Z

python/ql/test/experimental/dataflow/validTest.py

@@ -75,6 +75,7 @@ def check_tests_valid_after_version(testFile, version):
    check_tests_valid("variable-capture.test_collections")
    check_tests_valid("variable-capture.by_value")
    check_tests_valid("variable-capture.test_library_calls")
+    check_tests_valid("variable-capture.test_fields")


Does Python have its own test framework? I was expecting a normal QL (inline expectation) test.

Yes. This is an extra layer that ensures that our test expectations agree with the Python interpreter :-)

(The variable capture test is also slightly custom inline dataflow test, only annotating with "captured", but that is for historical reasons. We should probably convert it to a standard inline dataflow test at some point.)

yoff · 2023-12-20T15:00:15Z

Evaluation looks as expected. New results now look reasonable.

This reverts commit 45411f4.

hvitved · 2023-12-20T15:37:56Z

python/ql/lib/semmle/python/dataflow/new/internal/VariableCapture.qll

+  class VariableWrite extends ControlFlowNode {
+    CapturedVariable v;
+
+    VariableWrite() { this = v.getAStore().getAFlowNode().(DefinitionNode).getValue() }


I think this should really be the assignment node, as that is where the assignment takes place, and not the RHS of the assignment. Note that you will then have to change

result.(Flow::VariableWriteSourceNode).getVariableWrite() = n.(CfgNode).getNode()

in asClosureNode as well.

As discussed offline, we will change this follow-up, once Python has gained cfg nodes for assignments.

Changes have been addressed.

github-actions bot added documentation Python labels Nov 28, 2023

yoff force-pushed the python/captured-variables-basic branch from 96ac77b to 76bb328 Compare November 29, 2023 08:00

github-advanced-security bot found potential problems Nov 29, 2023

View reviewed changes

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll Fixed Show fixed Hide fixed

yoff force-pushed the python/captured-variables-basic branch 2 times, most recently from c043f0c to a6e3746 Compare December 5, 2023 19:03

github-actions bot removed the documentation label Dec 5, 2023

yoff added 13 commits December 14, 2023 10:20

Python: add missing annotation

6db55cd

Python: add consistency exclusions

b513871

Python: exclude CaptureNodes

797deeb

Python: test callbacks to library calls

7565873

Python: support callbacks to library calls

17a0029

TODO: The member predicate `LibraryLambdaMethod::getACall` is currently too permissive. Ideally, we would have `libraryCallHasLambdaArg` as in Ruby. But even a more precise `libraryCall` predicate might be fine.

Python: restrict LibraryLambdaMethod

453ab9c

Python: further restrict LibraryLambdaMethod

061fd01

On the small test project, this reduces the number of instances from 285 to 22.

Python: exclusion for summary nodes

5471c92

as in Ruby

Python: filter local flow from a node to itself

efcdb3e

Python: typo

f32d5e4

Python: fix nonlocal captured variables

479d81f

This depends on the extractor fix

yoff force-pushed the python/captured-variables-basic branch from 7f30996 to 479d81f Compare December 14, 2023 09:37

Python: add consistency exception

2a5736e

this must have been lost in my clean-up rebase.

RasmusWL requested changes Dec 14, 2023

View reviewed changes

yoff and others added 7 commits December 15, 2023 00:34

Python: simplify assignments to captured variables

b6123de

Python: consistency failure gone

abd544d

Apply suggestions from code review

c395d2d

Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

Python: make compile again

f96c52e

also improve comment

Python: hide synthesized capture nodes

2051ba3

Python: Make compile and add comment

262d43a

Python : P -> PY

bfdcae4

yoff added 2 commits December 18, 2023 23:42

Python: add change-note

8b7b582

I chose `category: majorAnalysis`, the description is "An API has changed in a way that may affect the results produced by a query that consumes the API." The API in question here is `flowPath` which is used by all our data flow queries.

Merge branch 'main' into python/captured-variables-basic

a60c52b

github-actions bot added the documentation label Dec 18, 2023

yoff requested a review from RasmusWL December 18, 2023 23:11

RasmusWL reviewed Dec 19, 2023

View reviewed changes

Update python/ql/lib/change-notes/2023-12-18-support-variable-capture.md

1417c2c

Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

yoff requested a review from RasmusWL December 19, 2023 09:09

yoff force-pushed the python/captured-variables-basic branch from 69cecdd to 1417c2c Compare December 19, 2023 20:36

Merge branch 'main' of https://github.com/github/codeql into python/c…

f8417b0

…aptured-variables-basic

hvitved reviewed Dec 20, 2023

View reviewed changes

yoff added 7 commits December 20, 2023 14:35

Python: fix typos

3cea46f

Python: move capture node to DataFlowPrivate

afb3d1d

Python: hide synthetic variable node

491ca3f

Python: remove unused member predicate

215b146

Python: make it a real consistency check

45411f4

Python: fix compilation

706e9dc

Python: add test for fields

d039ceb

hvitved reviewed Dec 20, 2023

View reviewed changes

Python: fix VariableWrite and remove unneded step

0f89f69

yoff requested a review from hvitved December 20, 2023 14:47

Revert "Python: make it a real consistency check"

da4aef8

This reverts commit 45411f4.

hvitved reviewed Dec 20, 2023

View reviewed changes

hvitved approved these changes Dec 20, 2023

View reviewed changes

yoff merged commit b83c743 into github:main Dec 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python: Basic implementation of variable capture #14944

Python: Basic implementation of variable capture #14944

yoff commented Nov 28, 2023 •

edited

Loading

RasmusWL left a comment

RasmusWL Dec 14, 2023

yoff Dec 15, 2023

yoff Dec 18, 2023

RasmusWL Dec 14, 2023

yoff Dec 18, 2023

RasmusWL Dec 14, 2023

yoff Dec 15, 2023

RasmusWL Dec 15, 2023 •

edited

Loading

yoff Dec 15, 2023

yoff Dec 18, 2023

RasmusWL Dec 19, 2023

RasmusWL left a comment

RasmusWL Dec 19, 2023

RasmusWL commented Dec 19, 2023

yoff commented Dec 19, 2023

yoff commented Dec 19, 2023

yoff commented Dec 19, 2023

hvitved left a comment

hvitved Dec 20, 2023

yoff Dec 20, 2023

hvitved Dec 20, 2023

hvitved Dec 20, 2023

yoff Dec 20, 2023

yoff Dec 20, 2023

yoff commented Dec 20, 2023

hvitved Dec 20, 2023

yoff Dec 20, 2023

		arg = call.getArgument(_) and
		arg instanceof CaptureArgumentNode

	// and self arguments
	this.asCfgNode() = any(CallNode c).getFunction().(AttrNode).getObject()

Python: Basic implementation of variable capture #14944

Python: Basic implementation of variable capture #14944

Conversation

yoff commented Nov 28, 2023 • edited Loading

RasmusWL left a comment

Choose a reason for hiding this comment

Lambda self naming

Variable capture code

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RasmusWL Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Interplay with self argument

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RasmusWL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RasmusWL commented Dec 19, 2023

yoff commented Dec 19, 2023

yoff commented Dec 19, 2023

yoff commented Dec 19, 2023

hvitved left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoff commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoff commented Nov 28, 2023 •

edited

Loading

RasmusWL Dec 15, 2023 •

edited

Loading