Commit aac7360
authored
[mypyc] Fix
## Issue
Mypyc can incorrectly borrow references from property getters (owned
references).
The borrow mechanism is safe for struct field access since they return a
pointer into the object's memory; Property getters on the other hand are
method calls that return new owned references i.e the caller must
`DECREF` them. If mypyc mistakes a property for a struct field, it skips
the `DECREF`, leaking one reference per call.
I think this affects any expression context that enables borrowing
(comparisons, `isinstance`, `is None` checks) when the borrowed
expression is a property access on a cross-module class. We discovered
this through OOM issues in SQLGlot, where `isinstance(self.this, Func)`
(`this` is a `@property`) leaked on every call.
## Repro
```Python3
# base.py:
class Bar:
pass
class Foo:
def __init__(self) -> None:
self.obj: object = Bar()
@Property
def val(self) -> object:
return self.obj
# derived.py:
from base import Foo, Bar
def check(foo: Foo) -> bool:
return isinstance(foo.val, Bar)
# test_leak.py:
import sys
from base import Foo
from derived import check
foo = Foo()
init = sys.getrefcount(foo.obj)
for _ in range(100):
check(foo)
print(f"Leaked refs: {sys.getrefcount(foo.obj) - init}")
```
Compile both with mypyc passing in `PYTHONHASHSEED=3` prints `Leaked
refs: 100`.
## Root cause
1. `is_native_attr_ref()` decides if an attribute access can safely
borrow by checking `has_attr(name) and not get_method(name)` i.e "if
there's an attribute but no method, it's a struct field".
2. Although `has_attr()` is always fully populated, `get_method()`
depends on whether the class's module (`base` in this case) has been
compiled yet.
3. Modules within an SCC are compiled in nondeterministic order; If the
module defining the property hasn't been compiled yet -> `get_method()`
returns `None` -> `is_native_attr_ref` incorrectly treats the property
as a borrowed struct field.
I think there's a broader issue here: Even when `derived.py` imports
from `base.py` (no cycle), mypyc can place them in the same SCC and
compile derived before base. Since `ir.methods` is only populated when a
module's function bodies are compiled, any code that reads `ir.methods`
(i.e `get_methods`) during compilation of a different module in the same
SCC could have similar order-dependent bugs.
## The suggested fix
Check `ir.attributes` directly (struct fields only, always populated)
instead of the unreliable `get_method`. I believe this is the
getter-side counterpart to #21095 which fixed the same class of bug for
property setters.@property getter memory leak (#21230)1 parent fc410cb commit aac7360
2 files changed
Lines changed: 45 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1553 | 1553 | | |
1554 | 1554 | | |
1555 | 1555 | | |
1556 | | - | |
1557 | | - | |
| 1556 | + | |
1558 | 1557 | | |
1559 | 1558 | | |
1560 | 1559 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
5925 | 5925 | | |
5926 | 5926 | | |
5927 | 5927 | | |
| 5928 | + | |
| 5929 | + | |
| 5930 | + | |
| 5931 | + | |
| 5932 | + | |
| 5933 | + | |
| 5934 | + | |
| 5935 | + | |
| 5936 | + | |
| 5937 | + | |
| 5938 | + | |
| 5939 | + | |
| 5940 | + | |
| 5941 | + | |
| 5942 | + | |
| 5943 | + | |
| 5944 | + | |
| 5945 | + | |
| 5946 | + | |
| 5947 | + | |
| 5948 | + | |
| 5949 | + | |
| 5950 | + | |
| 5951 | + | |
| 5952 | + | |
| 5953 | + | |
| 5954 | + | |
| 5955 | + | |
| 5956 | + | |
| 5957 | + | |
| 5958 | + | |
| 5959 | + | |
| 5960 | + | |
| 5961 | + | |
| 5962 | + | |
| 5963 | + | |
| 5964 | + | |
| 5965 | + | |
| 5966 | + | |
| 5967 | + | |
| 5968 | + | |
| 5969 | + | |
| 5970 | + | |
| 5971 | + | |
0 commit comments