Skip to content

Python: Prevent bad TC and add a bit of caching #5535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 29, 2021
Merged

Python: Prevent bad TC and add a bit of caching #5535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dbef36c
Python: Prevent bad TC and add a bit of caching
tausbn Mar 25, 2021
229250d
Python: Limit size of `TupleElementContent`
tausbn Mar 25, 2021
8734df3
Python: Slight cleanup
tausbn Mar 25, 2021
c2f112c
Python: Filter _before_ the cartesian product
tausbn Mar 25, 2021
f17bbd9
Python: Fix another bad TC.
tausbn Mar 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion python/ql/src/semmle/python/dataflow/new/internal/DataFlowPrivate.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1517,10 +1517,13 @@ predicate forReadStep(CfgNode nodeFrom, Content c, Node nodeTo) {
or
c instanceof SetElementContent
or
c instanceof TupleElementContent
c = small_tuple()
)
}

pragma[noinline]
TupleElementContent small_tuple() { result.getIndex() <= 7 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did any special logic go into picking the value 7? or could it just as well have been 10? (either way, I would really appreciate a comment in the code explaining those details, so it's easy to figure out in 1 year time when we look at it again 😉 )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally said 10, then yoff said that might be a bit high, then I said 5 but then changed my mind and said 7 which we both agreed was a nice round number.

I don't expect this code to be around in a year's time, and ideally not even a month from now. We already have a plan for a more principled approach for this (mimicking the "any index"-ness of list element content), but in the short term restricting this value is the best way to avoid blowup.


/**
* Holds if `nodeTo` is a read of an attribute (corresponding to `c`) of the object in `nodeFrom`.
*
Expand Down
23 changes: 14 additions & 9 deletions python/ql/src/semmle/python/dataflow/new/internal/DataFlowPublic.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,14 +467,22 @@ class BarrierGuard extends GuardNode {
}
}

private predicate comes_from_cfgnode(Node node) {
exists(CfgNode first, Node second |
simpleLocalFlowStep(first, second) and
simpleLocalFlowStep*(second, node)
)
}

/**
* A data flow node that is a source of local flow. This includes things like
* - Expressions
* - Function parameters
*/
class LocalSourceNode extends Node {
cached
LocalSourceNode() {
not simpleLocalFlowStep+(any(CfgNode n), this) and
not comes_from_cfgnode(this) and
not this instanceof ModuleVariableNode
or
this = any(ModuleVariableNode mvn).getARead()
Expand Down Expand Up @@ -522,15 +530,12 @@ private module Cached {
* The slightly backwards parametering ordering is to force correct indexing.
*/
cached
predicate hasLocalSource(Node sink, Node source) {
// Declaring `source` to be a `SourceNode` currently causes a redundant check in the
// recursive case, so instead we check it explicitly here.
source = sink and
source instanceof LocalSourceNode
predicate hasLocalSource(Node sink, LocalSourceNode source) {
source = sink
or
exists(Node mid |
hasLocalSource(mid, source) and
simpleLocalFlowStep(mid, sink)
exists(Node second |
simpleLocalFlowStep(source, second) and
simpleLocalFlowStep*(second, sink)
)
}

Expand Down