Python/support match #7635

yoff · 2022-01-18T20:13:18Z

Cleaned up version of #7356

QL part of https://github.com/github/semmle-code/pull/41454

- new syntactic category `Pattern` (in `Patterns.qll`) - subpatterns available on statments - new statements `MatchStmt` and `Case` (`Match` would conflict with the shared ReDoS library) - new expression `Guard` - support for pattern lists

- new SSA definition `PatternCaptureDefinition` - new SSA definition `PatternAliasDefinition` - implement `hasDefiningNode`

- also update `validTest.py`, but commented out for now otherwise CI will fail until we force it to run with Python 3.10 - added debug utility for dataflow (`dataflowTestPaths.ql`)

adapted from [the ruby instructions](https://github.com/github/codeql/blob/main/ruby/doc/prepare-db-upgrade.md)

- should still update stats for tables

yoff · 2022-01-20T12:22:04Z

The language tests pass in the Semmle-code PR.
Here, I see errors of the form

[VALUE_NOT_IN_TYPE] predicate py_bools(@py_bool_parent parent, int idx): Value 4333 of field parent is not in type @py_bool_parent. The value is however in the following types: @py_Try. Appears in tuple (4333,4)

Does that indicate a problem with the upgrade script?

yoff · 2022-01-24T05:46:11Z

The language tests pass in the Semmle-code PR. Here, I see errors of the form
[VALUE_NOT_IN_TYPE] predicate py_bools(@py_bool_parent parent, int idx): Value 4333 of field parent is not in type @py_bool_parent. The value is however in the following types: @py_Try. Appears in tuple (4333,4)
Does that indicate a problem with the upgrade script?

Ah, I guess that is expected since this repository contains the new dbscheme, but not the updated extractor.

…atch

tausbn

A bunch of comments, but overall this is a wonderful piece of work!
I especially like the thorough comments and the clear division regarding setting/removing Content in flow steps. Made it very easy to review. 🙂

tausbn · 2022-01-24T13:46:21Z

python/ql/lib/semmle/python/AstGenerated.qll

+
+/** INTERNAL: See the class `MatchAsPattern` for further information. */
+library class MatchAsPattern_ extends @py_MatchAsPattern, Pattern {
+  /** Gets the pattern of this matchaspattern pattern. */


matchaspattern is not the most elegant thing, though I guess it'll be a pain to get it to autogenerate it better.

I guess we can just ignore it for now. We can always override the QLDoc on the wrapper class, if need be.

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

tausbn · 2022-01-24T13:52:37Z

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

+ * `command` is the subject of the as-pattern, while the second component of `command` is the subject
+ * of the first capture pattern. As such, 'subject' refers to the pattern under evaluation.


I'm slightly confused by this description. I see two as-patterns, and a second component only appears in the second of these.

Yes, that description is wrong, I think it has not been updated. Thanks for catching that.

tausbn · 2022-01-26T12:08:41Z

python/ql/lib/semmle/python/AstExtended.qll

 library class ExprParent extends ExprParent_ { }

+/** Internal implementation class */
+library class PatternListParent extends PatternListParent_ { }


You can leave off the library bit. We don't need that any more.

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

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

tausbn · 2022-01-26T12:31:32Z

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

+def test_value_pattern():
+    match SOURCE:
+        case Unsafe.VALUE as x:
+            SINK(x) #$ flow="SOURCE, l:-2 -> x" MISSING: flow="SOURCE, l:-5 -> x"


This missing flow is curious. Do you know what causes it?

Yes. I thought we would need the guard aspect to be implemented, but now that you mention it, there is already flow from the value pattern Unsafe.VALUE to the alias x. So all we should need is flow from the actual value to the value pattern.

Actually, I think we are also missing flow from the SSA variable on line 58 to the attribute at line 62.

tausbn · 2022-01-26T12:33:33Z

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

+def test_sequence_pattern_list():
+    match [NONSOURCE, SOURCE]:
+        case [x, y]:
+            SINK_F(x) #$ SPURIOUS: flow="SOURCE, l:-2 -> x"


I note the spurious flow here, which I assume is caused by the list content mentioned elsewhere. I feel like we should be able to tighten this up a bit.

In this case, the incoming subject [NONSOURCE, SOURCE] has list content, so we really do not know where in the list the taint resides. In more realistic code, it would be written as

match l: case [x, y]: ... use x ...

If l has list content, we have to read it into x.

Co-authored-by: Taus <[email protected]>

The comment about `py_scopes` was simply removed

Particularly in value and literal patterns. This is getting a little bit into the guards aspect of matching. We could similarly add reverse flow in terms of sub-patterns storing to a sequence pattern, a flow step from alternatives to an-or-pattern, etc.. It does not seem too likely that sources are embedded in patterns to begin with, but for secrets perhaps? It is illustrated by the literal test. The value test still fails. I believe we miss flow in general from the static attribute.

…atch

tausbn

Looks good! Ship it!

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

…ate.qll Co-authored-by: Taus <[email protected]>

yoff requested a review from a team as a code owner January 18, 2022 20:13

github-actions bot added the Python label Jan 18, 2022

This was referenced Jan 18, 2022

Python: support match yoff/codeql#11

Closed

Python: Basic support for match statement #7356

Closed

yoff marked this pull request as draft January 19, 2022 12:30

python: New generated files

b17f844

yoff force-pushed the python/support-match branch from c98f6af to cb7d358 Compare January 19, 2022 12:36

yoff added 4 commits January 19, 2022 14:29

python: Wrappers for database classes

de8ecb2

- new syntactic category `Pattern` (in `Patterns.qll`) - subpatterns available on statments - new statements `MatchStmt` and `Case` (`Match` would conflict with the shared ReDoS library) - new expression `Guard` - support for pattern lists

pythos: SSA for match

bb210f4

- new SSA definition `PatternCaptureDefinition` - new SSA definition `PatternAliasDefinition` - implement `hasDefiningNode`

python: dataflow for match

36e18d5

- also update `validTest.py`, but commented out for now otherwise CI will fail until we force it to run with Python 3.10 - added debug utility for dataflow (`dataflowTestPaths.ql`)

python: tools for writing upgrades and downgrade

ef9fb08

adapted from [the ruby instructions](https://github.com/github/codeql/blob/main/ruby/doc/prepare-db-upgrade.md)

yoff force-pushed the python/support-match branch from cb7d358 to 74108e9 Compare January 19, 2022 13:30

python: upgrade and downgrade scripts

db253e8

yoff force-pushed the python/support-match branch from 74108e9 to db253e8 Compare January 19, 2022 14:23

yoff added 4 commits January 19, 2022 16:38

update stats for types

a0e79c1

- should still update stats for tables

python: remove compiler warnings

7e9a9e3

python: update stats for tables

d10ad3b

python: missing start tag for relation

32cbeae

yoff marked this pull request as ready for review January 20, 2022 12:22

Merge branch 'main' of github.com:github/codeql into python/support-m…

47af3a6

…atch

tausbn requested changes Jan 26, 2022

View reviewed changes

yoff and others added 4 commits January 27, 2022 10:31

Apply suggestions from code review

e28669e

Co-authored-by: Taus <[email protected]>

python: address review comments

cb52ab6

The comment about `py_scopes` was simply removed

Merge branch 'main' of github.com:github/codeql into python/support-m…

c60df7d

…atch

yoff requested a review from tausbn January 27, 2022 16:39

tausbn previously approved these changes Jan 28, 2022

View reviewed changes

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll Outdated Show resolved Hide resolved

Update python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPriv…

74d57bb

…ate.qll Co-authored-by: Taus <[email protected]>

yoff dismissed tausbn’s stale review via 74d57bb January 28, 2022 10:38

tausbn approved these changes Jan 28, 2022

View reviewed changes

tausbn merged commit 47a57e0 into main Jan 28, 2022

tausbn deleted the python/support-match branch January 28, 2022 10:55

		* `command` is the subject of the as-pattern, while the second component of `command` is the subject
		* of the first capture pattern. As such, 'subject' refers to the pattern under evaluation.

Python/support match #7635

Python/support match #7635

Uh oh!

Conversation

yoff commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yoff commented Jan 20, 2022

Uh oh!

yoff commented Jan 24, 2022

Uh oh!

tausbn left a comment

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!

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!

tausbn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yoff commented Jan 18, 2022 •

edited

Loading