Python: Captured variables for type tracking and the API graph #12537

yoff · 2023-03-15T14:05:10Z

We should perhaps check more thoroughly that the jump step is always correct.
From all the fixed results, it looks good, though...

In TypeTrackerSpecific.qll we add a jump step
- to every scope entry definition
- from the value of any defining DefinitionNode
  (In our example, the definition is the class name, Users,
  while the assigned value is the class definition, and it is
  the latter which receives flow in this case.)
In LocalSources.qll we allow scope entry definitions as local sources.
- This feels natural enough, as they are a local source for the value, they represent.
  It is perhaps a bit funne to see an Ssa variable here,
  rather than a control flow node.
- This is necessary in order for type tracking to see the local flow
  from the scope entry definition.
In ApiGraphs.qll we no longer restrict the result of trackUseNode
to be an ExprNode. To keep the positive formulation, we do not
prohibit module variable nodes explicitly. Instead we restrict to the new
LocalSourceNodeNotModule which has a restricted charpred.
Adjusted tracked.ql, the type tracking test:
- no need to annotate results on line 0
  this could happen for global SSA variables
- no need to annotate scope entry definitons
  they look a bit weird, as the annotation goes on the
  line of the function definition.

yoff · 2023-03-16T11:09:10Z

Evaluation showed a modest slow-down. This is expected, since we compute a bigger graph.
It also showed a more dramatic slow-down of py/jump-to-definition in one case. I suppose this is not really surprising.

This is a condensed versio of the user reported example found [here](https://github.com/dsp-testing/apictf/blob/eb377d5918bb4ac316a32361e5e0c082e61036d6/app.py#L278) The `MISSING` annotation indicates where our API graph falls short.

type tracking and the API graph. - In `TypeTrackerSpecific.qll` we add a jump step - to every scope entry definition - from the value of any defining `DefinitionNode` (In our example, the definition is the class name, `Users`, while the assigned value is the class definition, and it is the latter which receives flow in this case.) - In `LocalSources.qll` we allow scope entry definitions as local sources. - This feels natural enough, as they are a local source for the value, they represent. It is perhaps a bit funne to see an Ssa variable here, rather than a control flow node. - This is necessary in order for type tracking to see the local flow from the scope entry definition. - In `ApiGraphs.qll` we no longer restrict the result of `trackUseNode` to be an `ExprNode`. To keep the positive formulation, we do not prohibit module variable nodes. Instead we restrict to the new `LocalSourceNodeNotModule` which avoids those cases.

Adjusted `tracked.ql` - no need to annotate results on line 0 this could happen for global SSA variables - no need to annotate scope entry definitons they look a bit weird, as the annotation goes on the line of the function definition.

RasmusWL

I have not looked at the QL code yet, just skimmed the examples added.

I would like you to add more examples of edge cases, and things we still don't quite handle correctly even with this PR.

I would assume we handle Foo/Bar correctly in the examples below, but not so sure about Baz (can differentiate on whether a subclass of A or B reaches the print statement).

def func()
    if <cond>:
        class Foo(A): pass
    else:
        class Foo(B): pass

    class Bar(A): pass
    class Bar(B): pass

    class Baz(A): pass
    
    def other_func():
        print(Foo)
        print(Bar)
        print(Baz)

    class Baz(B): pass

    other_func()

I guess there are also cases where a reference to a global variables is captured, and the variables is altered in a function (using global modifier) -- but this might be covered by our tests already?

Lastly, do we have tests that cover capturing by value, such as the code below?

def func():
    a = 1
    def inner(a_val=a):
        print(a_val, a)
    a = 2
    inner()

python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

…aptured-variables-for-typetracking

Revert "python: no longer missing" This reverts commit f796177.

also rename `collections.py` so it does not clash with the standard library name. This clash is an issue when testing locally.

yoff · 2023-04-26T13:13:14Z

Nice suggestions for missing tests. Some of them are more dataflow than type-tracking, but good to have so I added them anyway 👍

@RasmusWL

(thanks @RasmusWL)

Instead of reusing `nonSink0` for both captureOut1NotCalled and captureOut2NotCalled tests (I used 1/2 naming scheme to match things up nicely). I also added a comment highlighting that `m` is the function that is not called (since I overlooked that initially :O)

RasmusWL

I made a small set of changes locally to use fresh variables names in some tests, and instead of requesting that you click buttons to apply these changes, I simply pushed the changes on the PR myself.

However, there are a few minor things I don't feel as confident about, so doing this in normal review style 😊

But overall this looks really good, and we have a really nice set of improvements to our results 💪 🎉

We should perhaps check more thoroughly that the jump step is always correct.

In the PR description you said the above, so just want to check whether this is something we need to look at more thoroughly? (to me it looks ok)

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

RasmusWL · 2023-05-02T12:01:24Z

python/ql/test/library-tests/ApiGraphs/py3/test_captured_inheritance.py

+
+    def other_func():
+        print(Foo)  #$ use=moduleImport("foo").getMember("A").getASubclass() use=moduleImport("foo").getMember("B").getASubclass()
+        print(Bar)  #$ use=moduleImport("foo").getMember("B").getASubclass() MISSING: use=moduleImport("foo").getMember("A").getASubclass()  The MISSING here is documenting correct behaviour


I disagree. There is no path where the Bar printed on this line can be a subclass of A.

Suggested change

print(Bar) #$ use=moduleImport("foo").getMember("B").getASubclass() MISSING: use=moduleImport("foo").getMember("A").getASubclass() The MISSING here is documenting correct behaviour

print(Bar) #$ use=moduleImport("foo").getMember("B").getASubclass()

There is no path where the Bar printed on this line can be a subclass of A.

On this we agree, but if I simply leave out the annotation, we do not know if it finds such a path or not. This because we are in the context of "admissible annotations", so it would not complain if it sees something but does not find an annotation. I tried to express that the MISSING annotation is used to document the fact that no path is, correctly, found.

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

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

yoff · 2023-05-04T07:13:52Z

We should perhaps check more thoroughly that the jump step is always correct.

In the PR description you said the above, so just want to check whether this is something we need to look at more thoroughly? (to me it looks ok)

I think it is ok :-)

RasmusWL

Besides a small nit, this looks ready to be merged for me 👍

python/ql/test/library-tests/ApiGraphs/py3/test_captured_inheritance.py

yoff · 2023-05-04T08:47:54Z

Interesting question from @calumgrant: Would a DCA run show, via some meta query, that we find new edges in the call graph?

RasmusWL · 2023-05-04T09:04:13Z

Interesting question from @calumgrant: Would a DCA run show, via some meta query, that we find new edges in the call graph?

yes, if you run this meta query: https://github.com/github/codeql/blob/main/python/ql/src/meta/analysis-quality/CallGraph.ql

…itance.py Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

yoff · 2023-05-09T09:01:46Z

yes, if you run this meta query: https://github.com/github/codeql/blob/main/python/ql/src/meta/analysis-quality/CallGraph.ql

Thanks, I ran that and a few more. It seems to have made a positive difference on the call graph :-)

RasmusWL

From my sampling of the new call-edges, they all look good 👍

yoff added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Mar 15, 2023

github-actions bot added the Python label Mar 15, 2023

yoff force-pushed the python/captured-variables-for-typetracking branch from 73829e8 to 0723a05 Compare March 16, 2023 09:54

github-actions bot added the documentation label Mar 16, 2023

yoff added 4 commits March 16, 2023 12:53

python: add test for flask example

7e003f6

This is a condensed versio of the user reported example found [here](https://github.com/dsp-testing/apictf/blob/eb377d5918bb4ac316a32361e5e0c082e61036d6/app.py#L278) The `MISSING` annotation indicates where our API graph falls short.

python: more results no longer missing

4713ba1

Adjusted `tracked.ql` - no need to annotate results on line 0 this could happen for global SSA variables - no need to annotate scope entry definitons they look a bit weird, as the annotation goes on the line of the function definition.

python: add change note

f9bffb5

yoff force-pushed the python/captured-variables-for-typetracking branch from 66d83fb to f9bffb5 Compare March 16, 2023 11:56

yoff marked this pull request as ready for review March 16, 2023 11:57

yoff requested a review from a team as a code owner March 16, 2023 11:57

RasmusWL requested changes Mar 17, 2023

View reviewed changes

python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py Outdated Show resolved Hide resolved

yoff and others added 12 commits April 11, 2023 14:34

Update python/ql/test/library-tests/ApiGraphs/py3/test_captured_flask.py

9e3d57d

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

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

a25c7f7

…aptured-variables-for-typetracking

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

141c5af

…aptured-variables-for-typetracking

python: no longer missing

f796177

Merge branch 'main' into python/captured-variables-for-typetracking

d4953ef

This was the case locally, but not in CI.. 🤷

0338d4e

Revert "python: no longer missing" This reverts commit f796177.

python: fix test expectations

824d4d5

also rename `collections.py` so it does not clash with the standard library name. This clash is an issue when testing locally.

python: add test for inheritance

b713061

python: remember to update validTest.py

4d95b20

python: add test for capturing via global

003fece

python: add test for capturing by value

66fdf6b

python: no longer missing

843329f

yoff requested a review from RasmusWL April 26, 2023 13:13

yoff added 2 commits April 26, 2023 15:54

python: we want empty expected files

20cbc08

(thanks @RasmusWL)

python: remove blank line

00b85cb

yoff removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Apr 27, 2023

yoff and others added 2 commits April 27, 2023 17:32

Merge branch 'main' into python/captured-variables-for-typetracking

54ced06

RasmusWL requested changes May 2, 2023

View reviewed changes

yoff and others added 3 commits May 3, 2023 18:03

Update python/ql/test/experimental/dataflow/variable-capture/by_value.py

e49f7a5

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

python: update comments

6d9fd24

python: longer name and longer comment

64068f1

yoff requested a review from RasmusWL May 3, 2023 16:24

RasmusWL previously approved these changes May 4, 2023

View reviewed changes

python/ql/test/library-tests/ApiGraphs/py3/test_captured_inheritance.py Outdated Show resolved Hide resolved

yoff dismissed RasmusWL’s stale review via d1206ea May 4, 2023 11:52

yoff and others added 2 commits May 4, 2023 13:52

Update python/ql/test/library-tests/ApiGraphs/py3/test_captured_inher…

d1206ea

…itance.py Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

Merge branch 'main' into python/captured-variables-for-typetracking

42090b5

yoff requested a review from RasmusWL May 9, 2023 09:02

RasmusWL approved these changes May 9, 2023

View reviewed changes

yoff merged commit 1a57f81 into github:main May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python: Captured variables for type tracking and the API graph #12537

Python: Captured variables for type tracking and the API graph #12537

yoff commented Mar 15, 2023 •

edited

Loading

yoff commented Mar 16, 2023

RasmusWL left a comment

yoff commented Apr 26, 2023

RasmusWL left a comment

RasmusWL May 2, 2023

yoff May 2, 2023

yoff commented May 4, 2023

RasmusWL left a comment

yoff commented May 4, 2023

RasmusWL commented May 4, 2023

yoff commented May 9, 2023

RasmusWL left a comment

	print(Bar) #$ use=moduleImport("foo").getMember("B").getASubclass() MISSING: use=moduleImport("foo").getMember("A").getASubclass() The MISSING here is documenting correct behaviour
	print(Bar) #$ use=moduleImport("foo").getMember("B").getASubclass()

Python: Captured variables for type tracking and the API graph #12537

Python: Captured variables for type tracking and the API graph #12537

Conversation

yoff commented Mar 15, 2023 • edited Loading

yoff commented Mar 16, 2023

RasmusWL left a comment

Choose a reason for hiding this comment

yoff commented Apr 26, 2023

RasmusWL left a comment

Choose a reason for hiding this comment

RasmusWL May 2, 2023

Choose a reason for hiding this comment

yoff May 2, 2023

Choose a reason for hiding this comment

yoff commented May 4, 2023

RasmusWL left a comment

Choose a reason for hiding this comment

yoff commented May 4, 2023

RasmusWL commented May 4, 2023

yoff commented May 9, 2023

RasmusWL left a comment

Choose a reason for hiding this comment

yoff commented Mar 15, 2023 •

edited

Loading