Skip to content

Commit c39fc6d

Browse files
authored
[mypyc] Make tagged int attributes memory safe under free threading (#22203)
Previously native tagged `int` attributes used completely unsynchronized access, which could result in use after free or double frees. Fix the memory safety issues, but only synchronize if an int has a long value (tag bit set and not undefined). It can still leak memory if there is a race condition involving long integers (e.g. 2**62 or larger on a 64-bit platform). This is a compromise to avoid slowing down very common (and fast) operations with short tagged integers. The implementation is otherwise similar to attributes with regular reference types. This doesn't have a measurable impact on self check performance on Python 3.14t. Work on mypyc/mypyc#1203.
1 parent defbc8d commit c39fc6d

8 files changed

Lines changed: 438 additions & 7 deletions

File tree

‎mypyc/codegen/emitclass.py‎

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,13 @@
4848
FuncIR,
4949
get_text_signature,
5050
)
51-
from mypyc.ir.rtypes import RTuple, RType, is_simple_refcounted_pointer, object_rprimitive
51+
from mypyc.ir.rtypes import (
52+
RTuple,
53+
RType,
54+
is_int_rprimitive,
55+
is_simple_refcounted_pointer,
56+
object_rprimitive,
57+
)
5258
from mypyc.namegen import NameGenerator
5359
from mypyc.sametype import is_same_type
5460

@@ -1393,6 +1399,20 @@ def generate_getter(cl: ClassIR, attr: str, rtype: RType, emitter: Emitter) -> N
13931399
emitter.emit_line("}")
13941400
return
13951401

1402+
if IS_FREE_THREADED and is_int_rprimitive(rtype) and attr not in cl.final_attributes:
1403+
# In free-threaded builds, CPyTagged_GetAttr takes a new reference to a long
1404+
# value without racing with a concurrent setter (see CPyTagged_SetAttr). It
1405+
# returns CPY_INT_TAG if the attribute is undefined.
1406+
emitter.emit_line(f"CPyTagged tmp = CPyTagged_GetAttr((PyObject *)self, &{attr_expr});")
1407+
emitter.emit_line("if (unlikely(tmp == CPY_INT_TAG)) {")
1408+
emitter.emit_line("PyErr_SetString(PyExc_AttributeError,")
1409+
emitter.emit_line(f' "attribute {repr(attr)} of {repr(cl.name)} undefined");')
1410+
emitter.emit_line("return NULL;")
1411+
emitter.emit_line("}")
1412+
emitter.emit_line("return CPyTagged_StealAsObject(tmp);")
1413+
emitter.emit_line("}")
1414+
return
1415+
13961416
# HACK: Don't consider refcounted values as always defined, since it's possible to
13971417
# access uninitialized values via 'gc.get_objects()'. Accessing non-refcounted
13981418
# values is benign.
@@ -1459,6 +1479,28 @@ def generate_setter(cl: ClassIR, attr: str, rtype: RType, emitter: Emitter) -> N
14591479
emitter.emit_line("}")
14601480
return
14611481

1482+
if IS_FREE_THREADED and is_int_rprimitive(rtype):
1483+
# In free-threaded builds, CPyTagged_SetAttr steals the new value and safely
1484+
# reclaims the old one (see CPyTagged_GetAttr). Unbox (borrowed) and take a
1485+
# new reference to hand over; a failed unbox leaves the attribute intact.
1486+
# CPY_INT_TAG deletes the attribute.
1487+
if deletable:
1488+
emitter.emit_line("if (value != NULL) {")
1489+
emitter.emit_unbox(
1490+
"value", "tmp", rtype, error=ReturnHandler("-1"), declare_dest=True, borrow=True
1491+
)
1492+
emitter.emit_inc_ref("tmp", rtype)
1493+
emitter.emit_line(f"CPyTagged_SetAttr((PyObject *)self, &self->{attr_field}, tmp);")
1494+
if deletable:
1495+
emitter.emit_line("} else {")
1496+
emitter.emit_line(
1497+
f"CPyTagged_SetAttr((PyObject *)self, &self->{attr_field}, CPY_INT_TAG);"
1498+
)
1499+
emitter.emit_line("}")
1500+
emitter.emit_line("return 0;")
1501+
emitter.emit_line("}")
1502+
return
1503+
14621504
# HACK: Don't consider refcounted values as always defined, since it's possible to
14631505
# access uninitialized values via 'gc.get_objects()'. Accessing non-refcounted
14641506
# values is benign.

‎mypyc/codegen/emitfunc.py‎

Lines changed: 34 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -462,9 +462,22 @@ def emit_load_attr_take_ref(
462462
safe by borrow_generator_attrs. The default (GIL) build always takes the plain-load
463463
path and increfs separately.
464464
465+
Non-Final 'int' attributes use CPyTagged_GetAttr on free-threaded builds, which
466+
also returns a new reference (or CPY_INT_TAG if undefined). Final 'int'
467+
attributes keep the plain load + incref, like CPy_GetAttrRefFinal.
468+
465469
Thread-confined attributes also use plain loads; see
466470
ClassIR.attrs_are_thread_confined.
467471
"""
472+
if (
473+
IS_FREE_THREADED
474+
and is_int_rprimitive(attr_rtype)
475+
and not op.is_borrowed
476+
and not cl.attrs_are_thread_confined()
477+
and not cl.is_final_attr(op.attr)
478+
):
479+
self.emitter.emit_line(f"{dest} = CPyTagged_GetAttr((PyObject *){obj}, &{attr_expr});")
480+
return True
468481
use_get_attr_ref = (
469482
self.emitter.target_python.free_threaded
470483
and is_simple_refcounted_pointer(attr_rtype)
@@ -570,11 +583,12 @@ def get_attr_with_allow_error_value(self, op: GetAttr) -> None:
570583

571584
attr_expr = self.get_attr_expr(obj, op, decl_cl)
572585
# On free-threaded builds this takes a new reference atomically (see
573-
# emit_load_attr_take_ref). CPy_GetAttrRef returns NULL when the field is
574-
# undefined, which is precisely the error value here, so the "NULL without
575-
# AttributeError" behavior is preserved (this op has error_kind ERR_NEVER).
576-
# is_simple_refcounted_pointer excludes tuples/vecs, so error_value_check's
577-
# special cases only matter on the plain-load path (where no ref was taken).
586+
# emit_load_attr_take_ref). CPy_GetAttrRef returns NULL and CPyTagged_GetAttr
587+
# returns CPY_INT_TAG when the field is undefined, which is precisely the error
588+
# value here, so the "error value without AttributeError" behavior is preserved
589+
# (this op has error_kind ERR_NEVER). The take-ref paths exclude tuples/vecs, so
590+
# error_value_check's special cases only matter on the plain-load path (where no
591+
# ref was taken).
578592
if not self.emit_load_attr_take_ref(dest, obj, op, cl, attr_rtype, attr_expr):
579593
# Only emit inc_ref if not NULL
580594
if attr_rtype.is_refcounted and not op.is_borrowed:
@@ -644,6 +658,21 @@ def visit_set_attr(self, op: SetAttr) -> None:
644658
)
645659
if op.error_kind == ERR_FALSE:
646660
self.emitter.emit_line(f"{dest} = 1;")
661+
elif (
662+
IS_FREE_THREADED
663+
and is_int_rprimitive(attr_rtype)
664+
and not cl.attrs_are_thread_confined()
665+
):
666+
# These also steal src, and reclaim the old value if it's a long.
667+
attr_expr = self.get_attr_expr(obj, op, decl_cl)
668+
if op.is_init:
669+
self.emitter.emit_line(f"CPyTagged_InitAttr(&{attr_expr}, {src});")
670+
else:
671+
self.emitter.emit_line(
672+
f"CPyTagged_SetAttr((PyObject *){obj}, &{attr_expr}, {src});"
673+
)
674+
if op.error_kind == ERR_FALSE:
675+
self.emitter.emit_line(f"{dest} = 1;")
647676
else:
648677
# ...and struct access for normal attributes.
649678
attr_expr = self.get_attr_expr(obj, op, decl_cl)

‎mypyc/lib-rt/pythonsupport.c‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,53 @@ PyObject *CPy_GetAttrRefSlow(PyObject *v, PyObject *owner, PyObject **field) {
3232
Py_END_CRITICAL_SECTION();
3333
return result;
3434
}
35+
36+
// Cold slow path of CPyTagged_GetAttr, also used for undefined (CPY_INT_TAG) values.
37+
// Reload the field with acquire (rather than using an acquire fence after the relaxed
38+
// load, which TSan doesn't model), so that a long stored with release by
39+
// CPyTagged_SetAttrSlow is fully visible. Then secure a reference to the PyLong using
40+
// the CPy_GetAttrRef protocol.
41+
CPy_NOINLINE
42+
CPy_COLD
43+
CPyTagged CPyTagged_GetAttrSlow(PyObject *owner, CPyTagged *field) {
44+
CPyTagged v = _Py_atomic_load_uintptr_acquire((uintptr_t *)field);
45+
if (CPyTagged_IsPointerFree(v)) {
46+
return v;
47+
}
48+
PyObject *p = (PyObject *)(v & ~CPY_INT_TAG);
49+
if (_Py_TryIncrefFast(p)) {
50+
return v;
51+
}
52+
if (_Py_TryIncRefShared(p)) {
53+
if (v == _Py_atomic_load_uintptr((uintptr_t *)field)) {
54+
return v;
55+
}
56+
Py_DECREF(p);
57+
}
58+
Py_BEGIN_CRITICAL_SECTION(owner);
59+
v = _Py_atomic_load_uintptr_relaxed((uintptr_t *)field);
60+
if (!CPyTagged_IsPointerFree(v)) {
61+
_Py_NewRefWithLock((PyObject *)(v & ~CPY_INT_TAG));
62+
}
63+
Py_END_CRITICAL_SECTION();
64+
return v;
65+
}
66+
67+
// Cold slow path of CPyTagged_SetAttr, used when the new or the old value may be a
68+
// long. Like CPy_SetAttrRef: store the new value under the owner's critical section
69+
// and release the old value (if it's a long) after leaving it.
70+
CPy_NOINLINE
71+
CPy_COLD
72+
void CPyTagged_SetAttrSlow(PyObject *owner, CPyTagged *field, CPyTagged value) {
73+
CPyTagged old;
74+
Py_BEGIN_CRITICAL_SECTION(owner);
75+
old = _Py_atomic_load_uintptr_relaxed((uintptr_t *)field);
76+
_Py_atomic_store_uintptr_release((uintptr_t *)field, value);
77+
Py_END_CRITICAL_SECTION();
78+
if (!CPyTagged_IsPointerFree(old)) {
79+
Py_DECREF((PyObject *)(old & ~CPY_INT_TAG));
80+
}
81+
}
3582
#endif
3683

3784
/////////////////////////////////////////

‎mypyc/lib-rt/pythonsupport.h‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,69 @@ static inline void CPy_InitAttrRef(PyObject **field, PyObject *value) {
162162
_Py_atomic_store_ptr_relaxed(field, value);
163163
}
164164

165+
// Native attributes of type 'int' (CPyTagged fields).
166+
//
167+
// A tagged value is pointer-free if it carries no reference: a short int (tag bit
168+
// clear) or the undefined value CPY_INT_TAG (a tagged NULL). Only a long int
169+
// (tag bit set, not CPY_INT_TAG) owns a reference to a PyLong object.
170+
//
171+
// Reads and writes of pointer-free values (the common case) use only relaxed loads
172+
// and stores, with no lock and no refcount operation. A long value goes through
173+
// cold slow paths that follow the CPy_GetAttrRef/CPy_SetAttrRef protocol on the
174+
// untagged pointer. The fast path writer never releases a reference, and only
175+
// stores pointer-free values. Acting on a stale pointer-free view it may overwrite
176+
// a long concurrently stored by another writer, which then leaks (stays alive
177+
// forever) -- this needs unsynchronized concurrent writers to the same attribute.
178+
// Only slow path writers release references, each releasing only the value it
179+
// loaded under the owner's lock after storing its replacement under that lock, so
180+
// there are no double frees, and the reader guarantees of CPy_GetAttrRef still
181+
// hold. Longs are stored with release and dereferenced only after an acquire load.
182+
//
183+
// The relaxed fast path gives the same (lack of) ordering guarantees as other
184+
// unboxed attribute types such as 'bool', 'float' and 'i64'.
185+
static inline int CPyTagged_IsPointerFree(CPyTagged x) {
186+
// Short-int test first: the 'x == CPY_INT_TAG' compare only runs for odd values.
187+
return !(x & CPY_INT_TAG) || x == CPY_INT_TAG;
188+
}
189+
190+
CPy_COLD CPyTagged CPyTagged_GetAttrSlow(PyObject *owner, CPyTagged *field);
191+
CPy_COLD void CPyTagged_SetAttrSlow(PyObject *owner, CPyTagged *field, CPyTagged value);
192+
193+
// Read an 'int' attribute, returning a new reference (or CPY_INT_TAG if undefined).
194+
// The hot path costs the same as the GIL build's load + CPyTagged_INCREF. Reading an
195+
// undefined attribute is an error, so it's left to the slow path, which keeps the
196+
// hot path to a single tag bit test.
197+
static inline CPyTagged CPyTagged_GetAttr(PyObject *owner, CPyTagged *field) {
198+
CPyTagged v = _Py_atomic_load_uintptr_relaxed((uintptr_t *)field);
199+
if (likely(!(v & CPY_INT_TAG))) {
200+
return v;
201+
}
202+
return CPyTagged_GetAttrSlow(owner, field);
203+
}
204+
205+
// Set an 'int' attribute, stealing the reference to 'value' (CPY_INT_TAG deletes the
206+
// attribute) and reclaiming the previous value. A long 'value' always takes the
207+
// locked slow path, even if the old value looks pointer-free: without the lock, a
208+
// writer can't know that the old value it read isn't a pointer, and releases must
209+
// happen under the lock (see above).
210+
static inline void CPyTagged_SetAttr(PyObject *owner, CPyTagged *field, CPyTagged value) {
211+
if (likely(CPyTagged_IsPointerFree(value))) {
212+
CPyTagged old = _Py_atomic_load_uintptr_relaxed((uintptr_t *)field);
213+
if (likely(CPyTagged_IsPointerFree(old))) {
214+
_Py_atomic_store_uintptr_relaxed((uintptr_t *)field, value);
215+
return;
216+
}
217+
}
218+
CPyTagged_SetAttrSlow(owner, field, value);
219+
}
220+
221+
// Initialize a previously undefined 'int' attribute, stealing the reference to
222+
// 'value'. This happens while 'self' is still thread-local, so a relaxed store
223+
// suffices (see CPy_InitAttrRef).
224+
static inline void CPyTagged_InitAttr(CPyTagged *field, CPyTagged value) {
225+
_Py_atomic_store_uintptr_relaxed((uintptr_t *)field, value);
226+
}
227+
165228
#endif
166229

167230
// Generated generator and coroutine helpers claim this flag while executing,

‎mypyc/test-data/run-classes.test‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9421,3 +9421,96 @@ def test_interpreted_subclass_shadowing() -> None:
94219421
assert (o.read_literal(), o.LITERAL) == (5, 97)
94229422
assert o.read_items() is C.ITEMS
94239423
assert o.ITEMS == [1]
9424+
9425+
[case testIntAttributeShortAndLongValues]
9426+
from typing import Any, Final
9427+
import sys
9428+
from mypy_extensions import trait
9429+
from testutil import assertRaises
9430+
9431+
BIG: Final = 2**80
9432+
MEDIUM: Final = 2**40 # Long on 32-bit platforms, short on 64-bit platforms
9433+
9434+
def make_big(i: int) -> int:
9435+
# Not a literal, since literals may be immortal
9436+
return BIG * (i + 1)
9437+
9438+
class C:
9439+
__deletable__ = ["n"]
9440+
9441+
def __init__(self, n: int) -> None:
9442+
self.n = n
9443+
9444+
class Fin:
9445+
def __init__(self, n: int) -> None:
9446+
self.n: Final = n
9447+
9448+
@trait
9449+
class T:
9450+
k: int
9451+
9452+
class WithTrait(T):
9453+
def __init__(self, k: int) -> None:
9454+
self.k = k
9455+
9456+
def get_k(t: T) -> int:
9457+
return t.k
9458+
9459+
def set_k(t: T, k: int) -> None:
9460+
t.k = k
9461+
9462+
def test_transitions() -> None:
9463+
c = C(1)
9464+
d: Any = c
9465+
for v in [BIG, 5, BIG + 1, BIG + 2, -BIG, 0, MEDIUM, -MEDIUM, -1, BIG * 3]:
9466+
c.n = v
9467+
assert c.n == v
9468+
assert d.n == v
9469+
d.n = v + 1
9470+
assert c.n == v + 1
9471+
assert d.n == v + 1
9472+
9473+
def test_old_long_value_released() -> None:
9474+
c = C(make_big(1))
9475+
dynamic_sys: Any = sys
9476+
# Any, so that boxing doesn't steal the reference on the last use
9477+
old: Any = c.n
9478+
before = dynamic_sys.getrefcount(old)
9479+
c.n = 1
9480+
assert dynamic_sys.getrefcount(old) == before - 1
9481+
c.n = make_big(2)
9482+
old = c.n
9483+
before = dynamic_sys.getrefcount(old)
9484+
del c.n
9485+
assert dynamic_sys.getrefcount(old) == before - 1
9486+
9487+
def test_delete() -> None:
9488+
c = C(BIG + 1)
9489+
del c.n
9490+
with assertRaises(AttributeError):
9491+
c.n
9492+
d: Any = c
9493+
with assertRaises(AttributeError):
9494+
d.n
9495+
d.n = BIG + 2
9496+
assert c.n == BIG + 2
9497+
del c.n
9498+
c.n = BIG + 3
9499+
assert c.n == BIG + 3
9500+
9501+
def test_final() -> None:
9502+
for v in [1, BIG + 1]:
9503+
f = Fin(v)
9504+
assert f.n == v
9505+
d: Any = f
9506+
assert d.n == v
9507+
9508+
def test_trait() -> None:
9509+
# Access through the trait type uses a field offset looked up at runtime
9510+
w = WithTrait(BIG)
9511+
assert get_k(w) == BIG
9512+
set_k(w, 2)
9513+
assert get_k(w) == 2
9514+
set_k(w, BIG + 1)
9515+
assert get_k(w) == BIG + 1
9516+
assert w.k == BIG + 1

0 commit comments

Comments
 (0)