New points-to and object model for Python. by markshannon · Pull Request #1277 · github/codeql

markshannon · 2019-04-24T13:09:22Z

New points-to and object model for Python.

Well factored object model separating internal and external API for objects
Points-to is simplified a bit as it is more focussed on flow and inheritance and less on object semantics.
Minimal changes to queries and query tests, queries still use old API which is almost completely backwards compatible

…ans.

…n functions. Move attribute lookup handling to objects.

…tests.

… Tweak API to be a bit more backward-compatible.

…patibility.

taus-semmle

First part of review.

taus-semmle

Finished review of ObjectInternal.qll.

…arifying a few comments.

taus-semmle

This completes the review of everything in /objects (and also includes a bunch of comments in PointsTo.qll, as there was no easy way to separate these).

taus-semmle · 2019-05-23T11:21:15Z

+    /** Gets the `n`th item of this sequence, if one exists. */
+    abstract ObjectInternal getItem(int n);
+
+    /** The boolean value of this object, this may be both


This and all other docstrings that are identical to the ones on the classes they extend would be better if removed altogether. As I understand it, the docstring on the base class will be shown instead.

taus-semmle · 2019-05-23T11:26:53Z

+    override string toString() {
+        this.length() = 0 and result = "()"
+        or
+        result = "(" + this.contents(0) + ")"


Minor comment, but should we show 1-element tuples as (foo,) the way Python does it? I know it's not the prettiest syntax, but it is unambiguous (and possibly more familiar to Python users).

taus-semmle · 2019-05-23T11:36:25Z

+
+    override predicate calleeAndOffset(Function scope, int paramOffset) { none() }
+
+    override predicate attribute(string name, ObjectInternal value, CfgOrigin origin) { none() }


Would we need to extend this in order to support e.g. sys.version_info.major, or is that handled through some other means?

Yes.
I've not seen any tests on sys.version_info.major, so I doubt it matters in practice.

We want to treat sys.version_info as an opaque object, to prevent premature pruning and
false positives when we are unsure of the actual version of Python that the code is expecting.
I've added a QL comment.

taus-semmle · 2019-05-27T12:55:56Z

        reverse_step(this, Empty(), result)
    }
+
+    boolean isSpecial() {


This should maybe be called containsSpecial or hasSpecial, and have an explanatory comment?

taus-semmle · 2019-05-28T12:24:01Z

+
+    /** Holds if this is a subclass of the `Iterable` abstract base class. */
+    boolean isIterableSubclass() {
+        this = ObjectInternal::builtin("list") and result = true


Minor point, but this could be written more succinctly by putting result = true in a conjunction outside the first three disjuncts. This way is probably better for readability, though.

taus-semmle · 2019-05-28T12:41:56Z

+        origin = CfgOrigin::unknown()
+    }
+
+    pragma [noinline] override predicate binds(ObjectInternal cls, string name, ObjectInternal descriptor) {


cls should be named instance in order to match up with the inherited docstring. (Alternatively this could have its own docstring.)

taus-semmle · 2019-05-28T13:55:16Z

+        exists(Module init |
+            init = this.getSourceModule() and
+            (
+                not exists(EssaVariable var | var.getAUse() = init.getANormalExit() and var.getSourceVariable().getName() = name)


This line seems somewhat mysterious. Could you add an explanatory comment?

taus-semmle · 2019-05-28T14:01:59Z

+
+    /** Gets a call to this object */
+    CallNode getACall() {
+        PointsToInternal::pointsTo(result.getFunction(), _, this, _)


It looks to me like this body could be replaced with simply result = this.getACall(_).

taus-semmle · 2019-05-28T14:14:01Z

+     *        @classmethod
+     *        def f(cls): pass
+     * ```
+     * `this.lookup("f")` is equivent to `C.__dict__['f']`, which is the class-method


equivalent

taus-semmle

Done with the review of the test files.

taus-semmle · 2019-05-28T15:06:04Z

@@ -1,3 +1,5 @@
+WARNING: Predicate points_to has been deprecated and may be removed in future (BooleanConstants.ql:8,5-24)


This should probably not be in the .expected file.

qltest emits the warning so it has to be in the expected output.
We could update the test to use the new API, but I want to keep the diff on expected outputs small for this PR.

taus-semmle · 2019-05-29T09:22:03Z

@@ -0,0 +1,27 @@
+| 2 | bool False |


This seems to be a false positive.

It is. Let me see if it is feasible to fix.

taus-semmle · 2019-05-29T09:30:34Z

@@ -1,3 +1,6 @@
+WARNING: Type CustomPointsToAttribute has been deprecated and may be removed in future (Extend.ql:26,35-58)


I think it would be better if our tests did not rely on deprecated features (and emit warnings about this). Would rewriting these to use PointsToExtension be too time-consuming?
(If so, I'm happy to accept these warnings temporarily, given the time constraints.)

We still need to test the deprecated API, so we have to endure the warnings.

taus-semmle · 2019-05-29T12:39:57Z

@@ -1,2 +1 @@
 | expressions_test.py:51:4:51:11 | Compare | The result of this comparison with 'is' may differ between implementations of Python. |
-| expressions_test.py:56:4:56:16 | Compare | The result of this comparison with 'is' may differ between implementations of Python. |


This looks to (now) be a false negative, but it doesn't look terribly serious. Is this right?

That was arguably a false positive and is now correct.
The assumption being that if an object is declared locally then an is comparison is OK as it is testing for identity. E.g.

SENTINEL = object() ... if x is SENTINEL: ...

…formatting.

taus-semmle

Finished review of PointsTo.qll. I believe this completes the reviewing.

taus-semmle · 2019-05-30T14:11:50Z

-        /** Helper for `scope_entry_value_transfer`. */
-        private predicate class_entry_value_transfer(EssaVariable pred_var, ScopeEntryDefinition succ_def) {
-            exists(ImportTimeScope scope, ControlFlowNode class_def |
-                class_def = pred_var.


base classes

markshannon added WIP This is a work-in-progress, do not merge yet! Python labels Apr 24, 2019

markshannon force-pushed the python-ipa-objects-bottom-up branch from 5278da6 to a7029ab Compare April 24, 2019 13:20

markshannon added 27 commits April 26, 2019 16:21

Python: First draft of ADT based objects and attendant points-to.

c48d63f

Python: Break-up internal object modules.

051683f

Python: Add ADTs for ints and strings. Add some global data-flow.

5a46df2

Python: Add better class support, including inheritance.

bf692f4

Python: Add support for bound-methods.

39b9723

Python points-to: Convert two pairs of predicates to methods on boole…

ec151e9

…ans.

Python points-to: Add support for some more ESSA definitions.

ce9d0f1

Python points-to: Add generic instances and handle returns for builti…

84c9866

…n functions. Move attribute lookup handling to objects.

Python points-to: Simplify handling of booleans and comparisons.

e3ed8c6

Python points-to: Further types and flow.

aa30745

Python points-to: Port old API classes to use new points-to.

dd83149

Python points-to: Add support for tuples.

12853cc

Python points-to: make 'self' instances distinct from other instances.

85a9016

Python points-to: Improve handling of 'type' object.

48297e2

Python points-to: Handle list, dict and float literals as instances.

f5c3242

Python points-to: Better handling of *args, **kwargs and procedures.

dbf228d

Python points-to: Support descriptor protocols, particularly functions.

0b0a633

Python: refactor ConstantObjects.

e9f58ba

Python points-to: Add float objects for better backwards compatibility.

931100c

Rename 'points_to' to 'pointsTo'.

d3762ac

Python points-to: Clean up interface, and deprecate old interface.

f7edbcc

Python points-to: Improve backwards compatibility for comparisons.

610a35c

Python points-to: Use objects, not booleans when doing evaluation of …

8af6cb6

…tests.

Python points-to: Understand callable and hasattr.

23ca403

Python points-to: Various minor performance tweaks.

54c27e1

Python points-to: Rationalize handling of expressions and conditions.…

ef0a6b6

… Tweak API to be a bit more backward-compatible.

Python points-to: Assorted improvements to performance and better com…

162bf51

…patibility.

Python points-to: Update Overview.qll to reflect new object model.

8558cb3

markshannon removed the WIP This is a work-in-progress, do not merge yet! label May 16, 2019

markshannon assigned taus-semmle May 16, 2019

markshannon added 2 commits May 17, 2019 14:04

Python points-to: Prevent bad magic.

685826e

Python: Speed up MRO calculation a bit.

f057d5c

taus-semmle suggested changes May 22, 2019

View reviewed changes

Python points-to: Add more comments about TObject and ObjectInternal.

1018f27

taus-semmle suggested changes May 22, 2019

View reviewed changes

Python points-to. Improve API bey renaming a couple of methods and cl…

e161488

…arifying a few comments.

taus-semmle suggested changes May 28, 2019

View reviewed changes

taus-semmle added this to the 1.21.0 milestone May 28, 2019

markshannon added 3 commits May 28, 2019 17:17

Python points-to: Improve qldoc and internal API a bit.

66e6461

Python points-to: Make purpose of '$' variable explicit.

3adaf07

Python points-to: More comment clarifications and typo fixes.

77c508f

taus-semmle suggested changes May 29, 2019

View reviewed changes

markshannon added 3 commits May 29, 2019 15:02

Python points-to: Update expected results to account for fixed tuple …

7181a02

…formatting.

Python points-to: Fix ordering of string constants.

1ed3e5d

Python points-to: Remove duplicate import.

71ae61c

markshannon force-pushed the python-ipa-objects-bottom-up branch from 5e64b9b to 71ae61c Compare May 29, 2019 14:05

taus-semmle suggested changes May 30, 2019

View reviewed changes

Python points-to: Minor refactoring for clarity.

6689994

markshannon force-pushed the python-ipa-objects-bottom-up branch from b4eae7f to b63d031 Compare May 30, 2019 16:49

taus-semmle reviewed May 31, 2019

View reviewed changes

Comment thread python/ql/src/semmle/python/pointsto/PointsTo.qll Outdated

Python points-to: Simplify logic w.r.t. comparisons.

8e2d6c4

markshannon force-pushed the python-ipa-objects-bottom-up branch from b63d031 to 8e2d6c4 Compare May 31, 2019 14:59

markshannon mentioned this pull request Jun 3, 2019

Python: New points-to and object model with performance fixes #1389

Merged

taus-semmle merged commit 8e2d6c4 into github:master Jun 3, 2019

markshannon deleted the python-ipa-objects-bottom-up branch June 3, 2019 17:35

markshannon mentioned this pull request Jun 7, 2019

Python: Change note for new points-to implementation. #1425

Merged

kamarcum unassigned taus-semmle Apr 28, 2020


		override predicate calleeAndOffset(Function scope, int paramOffset) { none() }

		override predicate attribute(string name, ObjectInternal value, CfgOrigin origin) { none() }

		@@ -1,3 +1,5 @@
		WARNING: Predicate points_to has been deprecated and may be removed in future (BooleanConstants.ql:8,5-24)

		@@ -1,3 +1,6 @@
		WARNING: Type CustomPointsToAttribute has been deprecated and may be removed in future (Extend.ql:26,35-58)

		@@ -1,2 +1 @@
		\| expressions_test.py:51:4:51:11 \| Compare \| The result of this comparison with 'is' may differ between implementations of Python. \|
		\| expressions_test.py:56:4:56:16 \| Compare \| The result of this comparison with 'is' may differ between implementations of Python. \|

Uh oh!

Conversation

markshannon commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taus-semmle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taus-semmle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

taus-semmle 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!

markshannon May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

taus-semmle left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markshannon May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markshannon commented Apr 24, 2019 •

edited

Loading

markshannon May 28, 2019 •

edited

Loading

markshannon May 29, 2019 •

edited

Loading