Skip to content

Commit 97fefd2

Browse files
committed
Python: Attempt to fix import flow
It's nice that it fixes the `InsecureProtocol` test-case (which maybe should have been a test-case for the import resolution library in the first place?) But it's not quite right: 1. it adds spurious flow for `clashing_attr` 2. it runs into huge problems for typetracking_imports/tracked.expected 3. it runs into the problem for github#10176 with an `from <pkg> import *` blocking flow from previously defined variable, that is NOT overridden. (simplistic_reexport.bar_attr)
1 parent bea0acb commit 97fefd2

File tree

5 files changed

+192
-89
lines changed

5 files changed

+192
-89
lines changed

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

Lines changed: 44 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -78,34 +78,63 @@ module ImportResolution {
7878
}
7979

8080
/**
81-
* Holds if the module `m` defines a name `name` by assigning `defn` to it. This is an
82-
* overapproximation, as `name` may not in fact be exported (e.g. by defining an `__all__` that does
83-
* not include `name`).
81+
* Holds if the module `m` defines a name `name` with the value `val`. The value
82+
* represents the value `name` will have at the end of the module (the last place we
83+
* have def-use flow to).
84+
*
85+
* Note: The handling of re-exporting imports is a bit simplistic. We assume that if
86+
* an import is made, it will be re-exported (which will not be the case if a new
87+
* value is assigned to the name, or it is deleted).
8488
*/
8589
pragma[nomagic]
86-
predicate module_export(Module m, string name, DataFlow::CfgNode defn) {
87-
exists(EssaVariable v, EssaDefinition essaDef |
88-
v.getName() = name and
89-
v.getAUse() = m.getANormalExit() and
90-
allowedEssaImportStep*(essaDef, v.getDefinition())
90+
predicate module_export(Module m, string name, DataFlow::Node val) {
91+
// Definitions made inside `m` itself
92+
//
93+
// for code such as `foo = ...; foo.bar = ...` there will be TWO
94+
// EssaDefinition/EssaVariable. One for `foo = ...` (AssignmentDefinition) and one
95+
// for `foo.bar = ...`. The one for `foo.bar = ...` (EssaNodeRefinement). The
96+
// EssaNodeRefinement is the one that will reach the end of the module (normal
97+
// exit).
98+
//
99+
// However, we cannot just use the EssaNodeRefinement as the `val`, because the
100+
// normal data-flow depends on use-use flow, and use-use flow targets CFG nodes not
101+
// EssaNodes. So we need to go back from the EssaDefinition/EssaVariable that
102+
// reaches the end of the module, to the first definition of the variable, and then
103+
// track forwards using use-use flow to find a suitable CFG node that has flow into
104+
// it from use-use flow.
105+
exists(EssaVariable lastUseVar, EssaVariable firstDef |
106+
lastUseVar.getName() = name and
107+
// we ignore special variable $ introduced by our analysis (not used for anything)
108+
// we ignore special variable * introduced by `from <pkg> import *` -- TODO: understand why we even have this?
109+
not name in ["$", "*"] and
110+
lastUseVar.getAUse() = m.getANormalExit() and
111+
allowedEssaImportStep*(firstDef, lastUseVar) and
112+
not allowedEssaImportStep(_, firstDef)
91113
|
92-
defn.getNode() = essaDef.(AssignmentDefinition).getValue()
114+
not EssaFlow::defToFirstUse(firstDef, _) and
115+
val.asVar() = firstDef
93116
or
94-
defn.getNode() = essaDef.(ArgumentRefinement).getArgument()
117+
exists(ControlFlowNode mid, ControlFlowNode end |
118+
EssaFlow::defToFirstUse(firstDef, mid) and
119+
EssaFlow::useToNextUse*(mid, end) and
120+
not EssaFlow::useToNextUse(end, _) and
121+
lastUseVar.getAUse() = end and
122+
val.asCfgNode() = end
123+
)
95124
)
96125
or
97-
// `from <pkg> import *`
126+
// re-exports from `from <pkg> import *`
98127
exists(Module importedFrom |
99128
importedFrom = ImportStar::getStarImported(m) and
100-
module_export(importedFrom, name, defn) and
129+
module_export(importedFrom, name, val) and
101130
potential_module_export(importedFrom, name)
102131
)
103132
or
104-
// `import <pkg>` or `from <pkg> import <stuff>`
133+
// re-exports from `import <pkg>` or `from <pkg> import <stuff>`
105134
exists(Alias a |
106-
defn.asExpr() = a.getValue() and
135+
val.asExpr() = a.getValue() and
107136
a.getAsname().(Name).getId() = name and
108-
defn.getScope() = m
137+
val.getScope() = m
109138
)
110139
}
111140

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
| pkg/alias_only_direct.py:1:26:1:36 | Comment # $ tracked | Missing result:tracked= |
2+
| pkg/alias_problem.py:1:26:1:36 | Comment # $ tracked | Missing result:tracked= |
3+
| pkg/alias_problem_fixed.py:3:26:3:36 | Comment # $ tracked | Missing result:tracked= |
4+
| pkg/problem_absolute_import.py:1:29:1:39 | Comment # $ tracked | Missing result:tracked= |
5+
| pkg/use.py:2:30:2:40 | Comment # $ tracked | Missing result:tracked= |
6+
| pkg/use.py:3:16:3:26 | Comment # $ tracked | Missing result:tracked= |
7+
| pkg/use.py:9:36:9:46 | Comment # $ tracked | Missing result:tracked= |
8+
| pkg/use.py:10:16:10:26 | Comment # $ tracked | Missing result:tracked= |
9+
| pkg/use.py:16:42:16:52 | Comment # $ tracked | Missing result:tracked= |
10+
| pkg/use.py:17:16:17:26 | Comment # $ tracked | Missing result:tracked= |
11+
| pkg/use.py:23:33:23:43 | Comment # $ tracked | Missing result:tracked= |
12+
| pkg/use.py:24:16:24:26 | Comment # $ tracked | Missing result:tracked= |
13+
| pkg/use.py:30:40:30:50 | Comment # $ tracked | Missing result:tracked= |
14+
| pkg/use.py:31:16:31:26 | Comment # $ tracked | Missing result:tracked= |
15+
| pkg/use.py:37:49:37:59 | Comment # $ tracked | Missing result:tracked= |
16+
| pkg/use.py:38:16:38:26 | Comment # $ tracked | Missing result:tracked= |
17+
| pkg/use.py:43:47:43:57 | Comment # $ tracked | Missing result:tracked= |
18+
| pkg/use.py:44:16:44:26 | Comment # $ tracked | Missing result:tracked= |
19+
| pkg/works_absolute_import.py:2:29:2:39 | Comment # $ tracked | Missing result:tracked= |

0 commit comments

Comments
 (0)