Skip to content

Commit 7755993

Browse files
committed
Python: Add jump steps for module attribute reads.
This is the quick-and-dirty solution, as discussed. An even quicker-and-dirtier solution would have used `ModuleValue::attr` and take the `getOrigin` of that as the source of the jump step. However, this turns out to be a bad choice, since `attr` might fail to have a value for the given attribute (for a variety of reasons). Thus, we instead appeal to a helper predicate that keeps track of which names are defined by which right-hand-sides in a given module. (Observe that type tracking works correctly for `x` in `mymodule.py`, even though `x` is never assigned a value in the eyes of the Value API.) This means that points-to is only used to actually figure out if the object we're looking an attribute up on is a module or not. This is the next thing to replace in order to eliminate the dependence on points-to, but this will require some care to ensure that all module lookups are handled correctly. Only two test files needed to be changed for the tests to pass. The first was the fixed false negative in the type tracker, and the other was a bunch of missing flow in the regression test. I have manually removed the `# Flow not found` annotations to make them consistent with the output. Pay particular attention to the annotation on line 117 -- I believe it was misplaced and should have been on line 106 instead (where, indeed, we now have flow where none appeared before).
1 parent 60fcb5e commit 7755993

File tree

4 files changed

+29
-5
lines changed

4 files changed

+29
-5
lines changed

python/ql/src/experimental/dataflow/internal/DataFlowPrivate.qll

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -504,6 +504,26 @@ predicate jumpStep(Node nodeFrom, Node nodeTo) {
504504
or
505505
// Module variable write
506506
nodeFrom = nodeTo.(ModuleVariableNode).getAWrite()
507+
or
508+
// Read of module attribute:
509+
exists(AttrRead r, ModuleValue mv |
510+
r.getObject().asCfgNode().pointsTo(mv) and
511+
module_export(mv.getScope(), r.getAttributeName(), nodeFrom) and
512+
nodeTo = r
513+
)
514+
}
515+
516+
/**
517+
* Holds if the module `m` defines a name `name` that by assigning `defn` to it. This is an
518+
* overapproximation, as `name` may not in fact be exported (e.g. by defining an `__all__` that does
519+
* not include `name`).
520+
*/
521+
private predicate module_export(Module m, string name, CfgNode defn) {
522+
exists(EssaVariable v |
523+
v.getName() = name and
524+
v.getAUse() = m.getANormalExit() and
525+
defn.getNode() = v.getDefinition().(AssignmentDefinition).getValue()
526+
)
507527
}
508528

509529
//--------

python/ql/test/experimental/dataflow/regression/dataflow.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
| module.py:1:13:1:18 | ControlFlowNode for SOURCE | test.py:89:10:89:10 | ControlFlowNode for t |
2+
| module.py:1:13:1:18 | ControlFlowNode for SOURCE | test.py:106:10:106:14 | ControlFlowNode for Attribute |
3+
| module.py:1:13:1:18 | ControlFlowNode for SOURCE | test.py:111:10:111:12 | ControlFlowNode for Attribute |
4+
| module.py:1:13:1:18 | ControlFlowNode for SOURCE | test.py:156:6:156:11 | ControlFlowNode for unsafe |
15
| module.py:6:12:6:17 | ControlFlowNode for SOURCE | test.py:101:10:101:10 | ControlFlowNode for t |
26
| test.py:3:10:3:15 | ControlFlowNode for SOURCE | test.py:3:10:3:15 | ControlFlowNode for SOURCE |
37
| test.py:6:9:6:14 | ControlFlowNode for SOURCE | test.py:7:10:7:10 | ControlFlowNode for s |

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ def test12():
8686

8787
def test13():
8888
t = module.dangerous
89-
SINK(t) # Flow not found
89+
SINK(t)
9090

9191
def test14():
9292
t = module.safe
@@ -108,13 +108,13 @@ def x_sink(arg):
108108
def test17():
109109
t = C()
110110
t.x = module.dangerous
111-
SINK(t.x) # Flow not found
111+
SINK(t.x)
112112

113113
def test18():
114114
t = C()
115115
t.x = module.dangerous
116116
t = hub(t)
117-
x_sink(t) # Flow not found
117+
x_sink(t)
118118

119119
def test19():
120120
t = CUSTOM_SOURCE
@@ -153,7 +153,7 @@ def test22(cond):
153153
SINK(t)
154154

155155
from module import dangerous as unsafe
156-
SINK(unsafe) # Flow not found
156+
SINK(unsafe)
157157

158158
def test23():
159159
with SOURCE as t:

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ def global_var_write_test():
5151

5252
def test_import():
5353
import mymodule
54-
mymodule.x # $f-:tracked
54+
mymodule.x # $tracked
5555
y = mymodule.func() # $tracked
5656
y # $tracked
5757

0 commit comments

Comments
 (0)