Python: Dataflow improvements by RasmusWL · Pull Request #7807 · github/codeql

RasmusWL · 2022-02-02T09:50:12Z

This turned out to be a bit of random bag of things, and the first many commits is refactoring.

Will strongly recommend reading this PR commit by commit.

The main goal was to improve field-flow for attributes, such that we use AttrRead/AttrWrite modeling, and clear content when writing new value to attribute.

Ended up improving a few of the json library modeling, since I added post-update nodes to arguments of calls we can't resolve as well 👍

Before we didn't show how we treated the value _after_ the check. But we do actually handle this nicely 💪

All the same tests are present in `fieldflow/test.py`

Also fixes a bug in the tests

I deleted the old tests, so it's very clear what tests to look for

I went with NormalDataflowTest to signify that if you don't know what you're looking for, this is probably the one. I did not want to just call it DataflowTest, since that becomes a big vague when there are also `FlowTest.qll` and `MaximalFlowTest.qll` -- I'm open to renaming this though 👍

I had to rewrite the SINK1-SINK7 definitions, since this new requirement complained that we had to add this `MISSING: flow` annotation :D Doing this implementation also revealed that there was a bug, since I did not compare files when checking for these `MISSING:` annotations. So fixed that up in the implementation for inline taint tests as well. (extra whitespace in argumentPassing.py to avoid changing line numbers for other tests)

I checked to see that the tests still works. If I deleted the `arg5` annotation, it got failures: ```diff diff --git a/python/ql/test/experimental/dataflow/coverage/argumentPassing.py b/python/ql/test/experimental/dataflow/coverage/argumentPassing.py index e218bdd..71816c1e01 100644 --- a/python/ql/test/experimental/dataflow/coverage/argumentPassing.py +++ b/python/ql/test/experimental/dataflow/coverage/argumentPassing.py @@ -46,7 +46,7 @@ def argument_passing( c, d=arg4, #$ arg4 func=argument_passing *, - e=arg5, #$ arg5 func=argument_passing + e=arg5, f, **g, ): diff --git a/python/ql/test/experimental/dataflow/coverage/argumentRoutingTest.expected b/python/ql/test/experimental/dataflow/coverage/argumentRoutingTest.expected index e69de29..22037a40c3 100644 --- a/python/ql/test/experimental/dataflow/coverage/argumentRoutingTest.expected +++ b/python/ql/test/experimental/dataflow/coverage/argumentRoutingTest.expected @@ -0,0 +1,2 @@ +| argumentPassing.py:49:7:49:10 | ControlFlowNode for arg5 | Unexpected result: arg5= | +| argumentPassing.py:49:7:49:10 | ControlFlowNode for arg5 | Unexpected result: func=argument_passing | ```

I feel like they don't bring any value anymore, since we have the nice inline expectation tests. If I'm wrong, happy to revert this commit though.

Note that this doesn't actually add the desired flow from setattr, due to missing post-update note. This will be fixed in later commit.

Notice the strange thing with treating `mypkg.foo(42)` as a ClassCall, but completely ignoring `mypkg.subpkg.bar(43)` -- due to having the two `ClassValue`s: - `Missing module attribute mypkg.foo` - `Missing module attribute mypkg.subpkg` But not `Missing module attribute mypkg.subpkg` with the current import structure.

This means that DataFlowCall is only for resolvable calls, which might not seem like a big thing in itself, but enables the next commit to actually work :P

Besides solving the problem with `setattr`, it also solved some old problems with json library modeling (yay).

I went with `minorAnalysis` instead of `majorAnalysis`, since I don't think the impact of this change will be major (but that's just my gut feeling).

RasmusWL · 2022-02-08T09:05:23Z

Performance looks ok. There are some changes in py/meta/alerts/remote-flow-sources-reach, but they all seem to be due to the change in 5774459 (LHS of assign no longer an AttrRead, so taint doesn't flow the LHS any more)

yoff

Many good things in here. It is great to see a clean-up of the test files in addition to the data-flow improvements 💪
(also nice trick with whitespaces)

yoff · 2022-02-14T11:32:35Z

-        s = TAINTED_STRING
-        ensure_tainted(s) # $ tainted


I am not sure these are without value. I think they check that the barrier guard did not conclude the entire block (everything dominated by the true edge) taint-free..

Fair point. I think the division between the tests in that file and test_logical is not super clear, but I've expanded the test in both files a bit, so everything should be covered with tests now 👍

yoff · 2022-02-14T11:35:35Z

@@ -1,59 +0,0 @@
-from python.ql.test.experimental.dataflow.testDefinitions import *


The tests in this file are not quite duplicates of those in test.py, since here it all happens in the global scope. It is possible that the tests could be arranged better, but just deleting these is perhaps not the best way.

Does field-flow work in a different way when things are on the global scope?

I must admit I did not consider if global scope could play a part here, but if the only difference is that things are in a global scope vs. defined in functions, I don't see the extra value from having both.

Global variables are distinct from local ones in our analysis, so I think we need to test both contexts.

yoff · 2022-02-14T11:40:41Z

-        case 42 as x:
-            SINK(x) #$ flow="SOURCE, l:-2 -> x" flow="42, l:-1 -> x"


I am not sure I understand why this is preferred. In fact, it might be good to not just test strings all the time? (42 is a documented source)

Because you're matching against SOURCE, which is the string source -- so without this change, running validTest.py fails 😬

yoff · 2022-02-14T12:13:07Z

    obj.foo = x

-
-def test_example1():


The names here used to refer to the docs (but I can see there were no link in the code), but it might be time to move on from that...

I see. Unless you're feeling very strongly about it, I think the new names are a bit more easy 😊

Agreed, you have expanded the tests beyond those examples, so telling names are more valuable now.

Based on review conversation

RasmusWL

I think I have been able to answer/fix all the review comments

RasmusWL · 2022-02-18T12:32:51Z

-        case 42 as x:
-            SINK(x) #$ flow="SOURCE, l:-2 -> x" flow="42, l:-1 -> x"


Because you're matching against SOURCE, which is the string source -- so without this change, running validTest.py fails 😬

RasmusWL · 2022-02-18T12:34:21Z

+  override predicate hasActualResult(Location location, string element, string tag, string value) {
+    super.hasActualResult(location, element, tag, value)
+  }


no, that was a leftover from being able to quick-eval it 😄 I've removed it again

RasmusWL · 2022-02-18T12:41:41Z

@@ -1 +0,0 @@
-import semmle.python.dataflow.new.internal.DataFlowImplConsistency::Consistency


No, I've re-added it. I looked into adding consistency queries to our codeql test run setup, so that these would be part of ALL tests, but I ended up not doing this -- but that's why they were deleted.

RasmusWL · 2022-02-18T12:49:50Z

    obj.foo = x

-
-def test_example1():


I see. Unless you're feeling very strongly about it, I think the new names are a bit more easy 😊

RasmusWL · 2022-02-18T12:53:52Z

@@ -1,59 +0,0 @@
-from python.ql.test.experimental.dataflow.testDefinitions import *


Does field-flow work in a different way when things are on the global scope?

I must admit I did not consider if global scope could play a part here, but if the only difference is that things are in a global scope vs. defined in functions, I don't see the extra value from having both.

RasmusWL · 2022-02-18T13:12:27Z

-        s = TAINTED_STRING
-        ensure_tainted(s) # $ tainted


Fair point. I think the division between the tests in that file and test_logical is not super clear, but I've expanded the test in both files a bit, so everything should be covered with tests now 👍

yoff

Nearly there. I do think that testing in both local and global scope is necessary, due to how different we treat those two contexts (separate classes for local and global variables, different behaviour with respect to import time and runtime). I agree, though, that this distinction is not at all clear in the original test-files. I look forward to seeing this merged :-)

I thought it was interesting that it did not propagate flow to the uses inside the functions :O

Notice that these tests don't pass, to show how they differ in the next commit!

Yikes

yoff

Thanks for organising the missing bits. I am happy to merge this once the tests pass 👍

RasmusWL · 2022-02-25T09:13:00Z

Ohhh... I had not accounted for validTest.py actually running those tests 😂 I'll fix that up 👍

This is very suspicious

By giving all variables unique names I also added a comment with the function name from the normal tests, so its' easily visible what these tests are testing

Since that apparently impacts call graph resolution with points-to :O Also interesting that global flow was only not working for those cases because of the tricky ifs... still need to 100% figure out how those ifs are messing up the analysis :|

TL;DR; we used a too low value for `--max-import-depth` :(

I think we should write our tests in a way that puts points-to in the best condition to resolve calls. Although this specific change did not change much, it should help set us up for success in the future 👍

Apparently there are minor differences with `test-6-max-import-depth-2` where under Python 2 `isfile_no_problem.py` still works as before

yoff

Wow, quite a twist at the end. Do you want to leave the investigations in this open state with all the monitoring in place, or would you rather put it on a branch until it is concluded? I feel it could take some time to sort out, and that we might not want to invest too much effort on the points-to implementation, so perhaps it is indeed better close it off and just leave the evidence/monitoring as is...

RasmusWL · 2022-02-28T12:50:52Z

Wow, quite a twist at the end. Do you want to leave the investigations in this open state with all the monitoring in place, or would you rather put it on a branch until it is concluded? I feel it could take some time to sort out, and that we might not want to invest too much effort on the points-to implementation, so perhaps it is indeed better close it off and just leave the evidence/monitoring as is...

My intention was not to do any more investigation, but just leave the "evidence" as is 😊

yoff

LGTM, thanks for the ride :-)

RasmusWL added 11 commits February 1, 2022 15:09

Python: Improve customSanitizer tests

1394b38

Before we didn't show how we treated the value _after_ the check. But we do actually handle this nicely 💪

Python: Delete duplicated tests

1390f03

All the same tests are present in `fieldflow/test.py`

Python: Run tests for fieldflow/test.py

a4bb0cc

Python: Run match tests if Python 3.10 or newer

d6f415b

Also fixes a bug in the tests

Python: Use InlineExpectationsTest for field-flow tests

4131960

I deleted the old tests, so it's very clear what tests to look for

Python: Remove extra whitespace from argumentPassing.py

76f3d74

Python: Delete dedicated argumentRouting<N> tests

cc4fe38

I feel like they don't bring any value anymore, since we have the nice inline expectation tests. If I'm wrong, happy to revert this commit though.

Python: Refactor field-flow test

f6215f2

github-actions Bot added the Python label Feb 2, 2022

RasmusWL added 8 commits February 2, 2022 11:19

Python: Expand fieldflow tests

d2b72a7

Python: Add attributeClearStep

51bc6dc

Python: Add simple test of AttrRead/AttrWrite

fb6b8eb

Python: restrict AttrRead with AttrNode.isLoad()

5774459

Python: Use AttrRead/AttrWrite for attr read/store steps

49b5d60

Note that this doesn't actually add the desired flow from setattr, due to missing post-update note. This will be fixed in later commit.

Python: Handle SyntheticPreUpdateNode in PrintNode

48aa07d

Python: Ignore .isAbsent() from ClassCall

5cd08b8

This means that DataFlowCall is only for resolvable calls, which might not seem like a big thing in itself, but enables the next commit to actually work :P

RasmusWL force-pushed the dataflow-improvements branch from 47db18d to dd3e94d Compare February 3, 2022 13:59

RasmusWL added 2 commits February 4, 2022 11:51

Python: Add post-update nodes to args of unresolved calls

b2ce0fc

Besides solving the problem with `setattr`, it also solved some old problems with json library modeling (yay).

Python: Add change-note

14a1aa0

I went with `minorAnalysis` instead of `majorAnalysis`, since I don't think the impact of this change will be major (but that's just my gut feeling).

RasmusWL force-pushed the dataflow-improvements branch from dd3e94d to 14a1aa0 Compare February 4, 2022 11:01

github-actions Bot added the documentation label Feb 4, 2022

RasmusWL marked this pull request as ready for review February 4, 2022 11:57

RasmusWL requested a review from a team as a code owner February 4, 2022 11:57

RasmusWL requested a review from yoff February 4, 2022 11:57

yoff requested changes Feb 14, 2022

View reviewed changes

RasmusWL added 2 commits February 18, 2022 13:47

Python: Restore dataflow consistency queries

7aa559f

Python: Improve sanitizer/guards tests

9d81fd3

Based on review conversation

RasmusWL commented Feb 18, 2022

View reviewed changes

RasmusWL requested a review from yoff February 18, 2022 13:14

Merge branch 'main' into dataflow-improvements

d2cd77a

yoff requested changes Feb 23, 2022

View reviewed changes

yoff mentioned this pull request Feb 23, 2022

Python: expand ArgumentNode to include arguments of non-resolved calls #6188

Closed

RasmusWL added 3 commits February 24, 2022 13:15

Python: Add global field-flow tests

2da4b39

I thought it was interesting that it did not propagate flow to the uses inside the functions :O

Python: Also do all field-flow tests in global scope

94d23f3

Notice that these tests don't pass, to show how they differ in the next commit!

Python: Accept global field-flow inconsistencies

abe4d8d

Yikes

RasmusWL requested a review from yoff February 24, 2022 14:08

yoff reviewed Feb 25, 2022

View reviewed changes

RasmusWL added 9 commits February 25, 2022 13:35

Python: Fix global field-flow for validTest.py

d83a9ef

Python: Global flow works when in own file???

0642610

This is very suspicious

Python: Ensure no cross-talk in global tests

faaa63a

By giving all variables unique names I also added a comment with the function name from the normal tests, so its' easily visible what these tests are testing

Python: Replicate global field-flow failures

2d0034c

Python: Show unresolved calls for field-flow tests

999af15

Python: Add investigation of field-flow problem

a0d1cea

TL;DR; we used a too low value for `--max-import-depth` :(

Python: Fix fieldflow tests by increasing max-import-depth

ead0b65

Python: Replace rest of from testlib import *

01d426d

I think we should write our tests in a way that puts points-to in the best condition to resolve calls. Although this specific change did not change much, it should help set us up for success in the future 👍

RasmusWL requested a review from yoff February 28, 2022 10:20

Python: Use Python 3 for investigation tests

0e0f159

Apparently there are minor differences with `test-6-max-import-depth-2` where under Python 2 `isfile_no_problem.py` still works as before

yoff reviewed Feb 28, 2022

View reviewed changes

RasmusWL requested a review from yoff February 28, 2022 13:57

yoff approved these changes Feb 28, 2022

View reviewed changes

yoff merged commit d953382 into github:main Feb 28, 2022

RasmusWL deleted the dataflow-improvements branch February 28, 2022 15:27

		@@ -1,59 +0,0 @@
		from python.ql.test.experimental.dataflow.testDefinitions import *

		case 42 as x:
		SINK(x) #$ flow="SOURCE, l:-2 -> x" flow="42, l:-1 -> x"

		@@ -1 +0,0 @@
		import semmle.python.dataflow.new.internal.DataFlowImplConsistency::Consistency

Conversation

RasmusWL commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RasmusWL commented Feb 8, 2022

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

RasmusWL commented Feb 25, 2022

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

RasmusWL commented Feb 28, 2022

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

RasmusWL commented Feb 2, 2022 •

edited

Loading