Skip to content

Commit a9f62a3

Browse files
committed
[mypyc] Make attribute access memory safe on free-threaded builds (#21705)
This fixes memory unsafety when there is a race condition related to attribute get/set operations on a free-threaded build, for simple reference-based attributes only (`PyObject *`). This includes attributes with primitive types like `list`, `set` and `str` and `dict`, and native class types. The main idea is to use delayed decref for the old attribute value when assigning to an attribute. There are detailed comments explaining the approach in detail. This causes a ~5% slowdown in self check when using 3.14t. Currently it looks like a significant perf cost is unavoidable if we want to fix memory unsafety, but we can further optimize mypy to reduce the impact by introducing final attributes, for example. Remaining work includes fixing attributes with fixed-length tuple types and tagged integer types (in follow-up PRs). I used coding agent assist heavily. Work on mypyc/mypyc#1203.
1 parent 0faa413 commit a9f62a3

9 files changed

Lines changed: 404 additions & 20 deletions

File tree

mypyc/codegen/emitclass.py

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
BITMAP_BITS,
3030
BITMAP_TYPE,
3131
CPYFUNCTION_NAME,
32+
IS_FREE_THREADED,
3233
MYPYC_DEFAULTS_SETUP,
3334
NATIVE_PREFIX,
3435
PREFIX,
@@ -43,7 +44,7 @@
4344
FuncIR,
4445
get_text_signature,
4546
)
46-
from mypyc.ir.rtypes import RTuple, RType, object_rprimitive
47+
from mypyc.ir.rtypes import RTuple, RType, is_simple_refcounted_pointer, object_rprimitive
4748
from mypyc.namegen import NameGenerator
4849
from mypyc.sametype import is_same_type
4950

@@ -1165,6 +1166,32 @@ def generate_getter(cl: ClassIR, attr: str, rtype: RType, emitter: Emitter) -> N
11651166
emitter.emit_line("{")
11661167
attr_expr = f"self->{attr_field}"
11671168

1169+
if IS_FREE_THREADED and is_simple_refcounted_pointer(rtype):
1170+
# In free-threaded builds, load the attribute and take a new reference
1171+
# atomically to avoid a use-after-free race with a concurrent setter.
1172+
# CPy_GetAttrRef returns NULL if the attribute is undefined (NULL field),
1173+
# which is exactly the error/undefined value for a 'PyObject *' field.
1174+
#
1175+
# Final attributes are never rebound (no setter), so there is no concurrent
1176+
# writer to race with: a plain load + incref is safe. Use the cheaper
1177+
# CPy_GetAttrRefFinal, which skips the try-incref and _Py_NewRefWithLock
1178+
# slow path entirely (an unconditional Py_INCREF needs no maybe-weakref).
1179+
# This getter is generated per defining class, so a direct membership test
1180+
# matches the read-only getset table above (no need to walk the MRO).
1181+
if attr in cl.final_attributes:
1182+
getattr_ref = f"CPy_GetAttrRefFinal((PyObject **)&{attr_expr})"
1183+
else:
1184+
getattr_ref = f"CPy_GetAttrRef((PyObject **)&{attr_expr})"
1185+
emitter.emit_line(f"PyObject *retval = {getattr_ref};")
1186+
emitter.emit_line("if (unlikely(retval == NULL)) {")
1187+
emitter.emit_line("PyErr_SetString(PyExc_AttributeError,")
1188+
emitter.emit_line(f' "attribute {repr(attr)} of {repr(cl.name)} undefined");')
1189+
emitter.emit_line("return NULL;")
1190+
emitter.emit_line("}")
1191+
emitter.emit_line("return retval;")
1192+
emitter.emit_line("}")
1193+
return
1194+
11681195
# HACK: Don't consider refcounted values as always defined, since it's possible to
11691196
# access uninitialized values via 'gc.get_objects()'. Accessing non-refcounted
11701197
# values is benign.
@@ -1202,6 +1229,30 @@ def generate_setter(cl: ClassIR, attr: str, rtype: RType, emitter: Emitter) -> N
12021229
emitter.emit_line("return -1;")
12031230
emitter.emit_line("}")
12041231

1232+
if IS_FREE_THREADED and is_simple_refcounted_pointer(rtype):
1233+
# In free-threaded builds, publish the new value atomically via
1234+
# CPy_SetAttrRef so a concurrent reader (see CPy_GetAttrRef) never sees a
1235+
# torn pointer or a freed old value. CPy_SetAttrRef steals its value and
1236+
# reclaims the old one, so we cast/type-check the incoming value, take a
1237+
# new reference (the setter only borrows 'value'), then hand it over.
1238+
# A NULL value deletes the attribute (reclaims the old value, stores NULL).
1239+
if deletable:
1240+
emitter.emit_line("if (value != NULL) {")
1241+
if is_same_type(rtype, object_rprimitive):
1242+
emitter.emit_line("PyObject *tmp = value;")
1243+
else:
1244+
emitter.emit_cast("value", "tmp", rtype, declare_dest=True)
1245+
emitter.emit_lines("if (!tmp)", " return -1;")
1246+
emitter.emit_inc_ref("tmp", rtype)
1247+
emitter.emit_line(f"CPy_SetAttrRef((PyObject **)&self->{attr_field}, tmp);")
1248+
if deletable:
1249+
emitter.emit_line("} else {")
1250+
emitter.emit_line(f"CPy_SetAttrRef((PyObject **)&self->{attr_field}, NULL);")
1251+
emitter.emit_line("}")
1252+
emitter.emit_line("return 0;")
1253+
emitter.emit_line("}")
1254+
return
1255+
12051256
# HACK: Don't consider refcounted values as always defined, since it's possible to
12061257
# access uninitialized values via 'gc.get_objects()'. Accessing non-refcounted
12071258
# values is benign.

mypyc/codegen/emitfunc.py

Lines changed: 71 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,13 @@
1212
TracebackAndGotoHandler,
1313
c_array_initializer,
1414
)
15-
from mypyc.common import GENERATOR_ATTRIBUTE_PREFIX, HAVE_IMMORTAL, NATIVE_PREFIX, REG_PREFIX
15+
from mypyc.common import (
16+
GENERATOR_ATTRIBUTE_PREFIX,
17+
HAVE_IMMORTAL,
18+
IS_FREE_THREADED,
19+
NATIVE_PREFIX,
20+
REG_PREFIX,
21+
)
1622
from mypyc.ir.class_ir import ClassIR
1723
from mypyc.ir.func_ir import FUNC_CLASSMETHOD, FUNC_STATICMETHOD, FuncDecl, FuncIR, all_values
1824
from mypyc.ir.ops import (
@@ -83,6 +89,7 @@
8389
is_int_rprimitive,
8490
is_none_rprimitive,
8591
is_pointer_rprimitive,
92+
is_simple_refcounted_pointer,
8693
is_tagged,
8794
)
8895

@@ -394,6 +401,35 @@ def get_attr_expr(self, obj: str, op: GetAttr | SetAttr, decl_cl: ClassIR) -> st
394401
cast = f"({decl_cl.struct_name(self.emitter.names)} *)"
395402
return f"({cast}{obj})->{self.emitter.attr(op.attr)}"
396403

404+
def emit_load_attr_take_ref(
405+
self, dest: str, obj: str, op: GetAttr, cl: ClassIR, attr_rtype: RType, attr_expr: str
406+
) -> bool:
407+
"""Emit the load of a native attribute into 'dest', taking a new reference.
408+
409+
On free-threaded builds, reading a single reference-counted 'PyObject *' field
410+
and taking a new reference must be done atomically to avoid a use-after-free
411+
race with a concurrent setter. CPy_GetAttrRef performs the load and incref
412+
atomically and returns a new reference (or NULL if undefined), so callers must
413+
NOT emit a separate inc_ref. Return True in that case so the caller can skip it.
414+
415+
Final attributes are never rebound (no setter), so there is no concurrent writer
416+
and no use-after-free window; an owned read uses the cheaper CPy_GetAttrRefFinal
417+
(a plain load + incref). Borrowed reads keep the plain load: they are only emitted
418+
for attributes safe to borrow on free-threaded builds (Final and vec attrs -- see
419+
transform_member_expr in irbuild), whose values live as long as their container.
420+
The default (GIL) build always takes the plain-load path and increfs separately.
421+
"""
422+
use_get_attr_ref = (
423+
IS_FREE_THREADED and is_simple_refcounted_pointer(attr_rtype) and not op.is_borrowed
424+
)
425+
if use_get_attr_ref and cl.is_final_attr(op.attr):
426+
self.emitter.emit_line(f"{dest} = CPy_GetAttrRefFinal((PyObject **)&{attr_expr});")
427+
elif use_get_attr_ref:
428+
self.emitter.emit_line(f"{dest} = CPy_GetAttrRef((PyObject **)&{attr_expr});")
429+
else:
430+
self.emitter.emit_line(f"{dest} = {attr_expr};")
431+
return use_get_attr_ref
432+
397433
def visit_get_attr(self, op: GetAttr) -> None:
398434
if op.allow_error_value:
399435
self.get_attr_with_allow_error_value(op)
@@ -427,7 +463,9 @@ def visit_get_attr(self, op: GetAttr) -> None:
427463
else:
428464
# Otherwise, use direct or offset struct access.
429465
attr_expr = self.get_attr_expr(obj, op, decl_cl)
430-
self.emitter.emit_line(f"{dest} = {attr_expr};")
466+
use_get_attr_ref = self.emit_load_attr_take_ref(
467+
dest, obj, op, cl, attr_rtype, attr_expr
468+
)
431469
always_defined = cl.is_always_defined(op.attr)
432470
merged_branch = None
433471
if not always_defined:
@@ -458,7 +496,7 @@ def visit_get_attr(self, op: GetAttr) -> None:
458496
)
459497
)
460498

461-
if attr_rtype.is_refcounted and not op.is_borrowed:
499+
if attr_rtype.is_refcounted and not op.is_borrowed and not use_get_attr_ref:
462500
if not merged_branch and not always_defined:
463501
self.emitter.emit_line("} else {")
464502
self.emitter.emit_inc_ref(dest, attr_rtype)
@@ -480,16 +518,20 @@ def get_attr_with_allow_error_value(self, op: GetAttr) -> None:
480518
cl = rtype.class_ir
481519
attr_rtype, decl_cl = cl.attr_details(op.attr)
482520

483-
# Direct struct access without NULL check
484521
attr_expr = self.get_attr_expr(obj, op, decl_cl)
485-
self.emitter.emit_line(f"{dest} = {attr_expr};")
486-
487-
# Only emit inc_ref if not NULL
488-
if attr_rtype.is_refcounted and not op.is_borrowed:
489-
check = self.error_value_check(op, "!=")
490-
self.emitter.emit_line(f"if ({check}) {{")
491-
self.emitter.emit_inc_ref(dest, attr_rtype)
492-
self.emitter.emit_line("}")
522+
# On free-threaded builds this takes a new reference atomically (see
523+
# emit_load_attr_take_ref). CPy_GetAttrRef returns NULL when the field is
524+
# undefined, which is precisely the error value here, so the "NULL without
525+
# AttributeError" behavior is preserved (this op has error_kind ERR_NEVER).
526+
# is_simple_refcounted_pointer excludes tuples/vecs, so error_value_check's
527+
# special cases only matter on the plain-load path (where no ref was taken).
528+
if not self.emit_load_attr_take_ref(dest, obj, op, cl, attr_rtype, attr_expr):
529+
# Only emit inc_ref if not NULL
530+
if attr_rtype.is_refcounted and not op.is_borrowed:
531+
check = self.error_value_check(op, "!=")
532+
self.emitter.emit_line(f"if ({check}) {{")
533+
self.emitter.emit_inc_ref(dest, attr_rtype)
534+
self.emitter.emit_line("}")
493535

494536
def next_branch(self) -> Branch | None:
495537
if self.op_index + 1 < len(self.ops):
@@ -529,6 +571,23 @@ def visit_set_attr(self, op: SetAttr) -> None:
529571
op.attr,
530572
)
531573
)
574+
elif IS_FREE_THREADED and is_simple_refcounted_pointer(attr_rtype):
575+
# In free-threaded builds, publishing a single reference-counted
576+
# 'PyObject *' field must be atomic so a concurrent reader (see
577+
# CPy_GetAttrRef) never observes a torn pointer or a freed value.
578+
# Both helpers steal the reference to src.
579+
attr_expr = self.get_attr_expr(obj, op, decl_cl)
580+
if op.is_init:
581+
# The attribute is known to be previously undefined (NULL), so
582+
# there is no old value to reclaim; a relaxed store suffices
583+
# (self's later publication provides the release barrier -- see
584+
# CPy_InitAttrRef).
585+
self.emitter.emit_line(f"CPy_InitAttrRef((PyObject **)&{attr_expr}, {src});")
586+
else:
587+
# Atomically swap in the new value and reclaim the old one.
588+
self.emitter.emit_line(f"CPy_SetAttrRef((PyObject **)&{attr_expr}, {src});")
589+
if op.error_kind == ERR_FALSE:
590+
self.emitter.emit_line(f"{dest} = 1;")
532591
else:
533592
# ...and struct access for normal attributes.
534593
attr_expr = self.get_attr_expr(obj, op, decl_cl)

mypyc/ir/class_ir.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,22 @@ def attr_details(self, name: str) -> tuple[RType, ClassIR]:
269269
def attr_type(self, name: str) -> RType:
270270
return self.attr_details(name)[0]
271271

272+
def is_final_attr(self, name: str) -> bool:
273+
"""Is the (possibly inherited) attribute Final, i.e. never rebound?
274+
275+
A Final attribute is read-only at runtime (it has no setter) and is assigned
276+
exactly once during construction, so it can never be reassigned afterwards.
277+
This makes it safe to borrow on free-threaded builds (no concurrent store can
278+
invalidate a borrowed reference) and lets reads skip the concurrent-writer
279+
guard. Returns False for properties and for attributes this class doesn't have.
280+
"""
281+
for ir in self.mro:
282+
if name in ir.attributes:
283+
return name in ir.final_attributes
284+
if name in ir.property_types:
285+
return False
286+
return False
287+
272288
def method_decl(self, name: str) -> FuncDecl:
273289
for ir in self.mro:
274290
if name in ir.method_decls:

mypyc/ir/rtypes.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -569,6 +569,19 @@ def is_native_rprimitive(rtype: RType) -> bool:
569569
return isinstance(rtype, RPrimitive) and rtype.name in KNOWN_NATIVE_TYPES
570570

571571

572+
def is_simple_refcounted_pointer(rtype: RType) -> bool:
573+
"""Is rtype represented at runtime as a single, reference-counted 'PyObject *'?
574+
575+
This covers 'object', 'str', containers, instances of native classes and
576+
optional/union types -- everything whose C representation is exactly one
577+
'PyObject *' field that owns a reference. It excludes unboxed types (tagged
578+
'int', fixed-width ints, floats, bools), inline tuples ('RTuple'), vectors
579+
('RVec') and C structs ('RStruct'), which need different treatment for
580+
free-threaded memory safety.
581+
"""
582+
return rtype.is_refcounted and not rtype.is_unboxed and not isinstance(rtype, RStruct)
583+
584+
572585
def is_tagged(rtype: RType) -> TypeGuard[RPrimitive]:
573586
return rtype is int_rprimitive or rtype is short_int_rprimitive
574587

mypyc/irbuild/builder.py

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1642,13 +1642,11 @@ def is_final_native_attr_ref(self, expr: MemberExpr) -> bool:
16421642
builds, since no concurrent store can invalidate the borrowed reference.
16431643
"""
16441644
obj_rtype = self.node_type(expr.expr)
1645-
if not (isinstance(obj_rtype, RInstance) and obj_rtype.class_ir.is_ext_class):
1646-
return False
1647-
# Find the class that defines the attribute and check whether it's Final there.
1648-
for ir in obj_rtype.class_ir.mro:
1649-
if expr.name in ir.attributes:
1650-
return expr.name in ir.final_attributes
1651-
return False
1645+
return (
1646+
isinstance(obj_rtype, RInstance)
1647+
and obj_rtype.class_ir.is_ext_class
1648+
and obj_rtype.class_ir.is_final_attr(expr.name)
1649+
)
16521650

16531651
def root_is_reassigned(self, v: Value) -> bool:
16541652
"""Is the root local variable a borrow chain 'v' reads from reassigned this expression?

mypyc/lib-rt/pythonsupport.c

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,27 @@
55

66
#include "pythonsupport.h"
77

8+
#ifdef Py_GIL_DISABLED
9+
// Cold slow path of CPy_GetAttrRef (declared in pythonsupport.h). Reached only
10+
// when the inline fast-path try-incref fails: the value is owned by another
11+
// thread, so taking a reference requires an atomic shared-refcount operation.
12+
// Kept out-of-line so the inline fast path stays small.
13+
//
14+
// First try the lock-free shared-refcount CAS. If the value has not had
15+
// maybe-weakref set yet (for example, it was published by CPy_InitAttrRef), force
16+
// a cross-thread reference via _Py_NewRefWithLock, which cannot fail and sets
17+
// maybe-weakref so subsequent reads take the fast path. The value was already
18+
// observed in the field by CPy_GetAttrRef; CPy_SetAttrRef's QSBR-delayed decref
19+
// keeps any replaced value alive long enough for this reader.
20+
CPy_NOINLINE
21+
PyObject *CPy_GetAttrRefSlow(PyObject *v) {
22+
if (_Py_TryIncRefShared(v)) {
23+
return v;
24+
}
25+
return _Py_NewRefWithLock(v); // sets maybe-weakref; cannot fail
26+
}
27+
#endif
28+
829
/////////////////////////////////////////
930
// Adapted from bltinmodule.c in Python 3.7.0
1031
PyObject*

0 commit comments

Comments
 (0)