Skip to content

Repository navigation

Commit 10ac6e6

Browse files
naschemenascheme
committed
pythongh-127271: Replace use of PyCell_GET/SET
These macros are not safe to use in the free-threaded build. Use `PyCell_GetRef()` and `PyCell_SetTakeRef()` instead. Add `PyCell_GET` to the free-threading howto table of APIs that return borrowed refs. Add critical sections to `PyCell_GET` and `PyCell_SET`.
1 parent 6da9d25 commit 10ac6e6

6 files changed

Lines changed: 70 additions & 34 deletions

File tree

‎Doc/howto/free-threading-extensions.rst‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,8 @@ that return :term:`strong references <strong reference>`.
167167
+-----------------------------------+-----------------------------------+
168168
| :c:func:`PyImport_AddModule` | :c:func:`PyImport_AddModuleRef` |
169169
+-----------------------------------+-----------------------------------+
170+
| :c:func:`PyCell_GET` | :c:func:`PyCell_Get` |
171+
+-----------------------------------+-----------------------------------+
170172

171173
Not all APIs that return borrowed references are problematic. For
172174
example, :c:func:`PyTuple_GetItem` is safe because tuples are immutable.

‎Include/Python.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@
6969
#include "pystats.h"
7070
#include "pyatomic.h"
7171
#include "lock.h"
72+
#include "critical_section.h"
7273
#include "object.h"
7374
#include "refcount.h"
7475
#include "objimpl.h"
@@ -130,7 +131,6 @@
130131
#include "import.h"
131132
#include "abstract.h"
132133
#include "bltinmodule.h"
133-
#include "critical_section.h"
134134
#include "cpython/pyctype.h"
135135
#include "pystrtod.h"
136136
#include "pystrcmp.h"

‎Include/cpython/cellobject.h‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,18 +22,24 @@ PyAPI_FUNC(PyObject *) PyCell_Get(PyObject *);
2222
PyAPI_FUNC(int) PyCell_Set(PyObject *, PyObject *);
2323

2424
static inline PyObject* PyCell_GET(PyObject *op) {
25+
PyObject *res;
2526
PyCellObject *cell;
2627
assert(PyCell_Check(op));
2728
cell = _Py_CAST(PyCellObject*, op);
28-
return cell->ob_ref;
29+
Py_BEGIN_CRITICAL_SECTION(cell);
30+
res = cell->ob_ref;
31+
Py_END_CRITICAL_SECTION();
32+
return res;
2933
}
3034
#define PyCell_GET(op) PyCell_GET(_PyObject_CAST(op))
3135

3236
static inline void PyCell_SET(PyObject *op, PyObject *value) {
3337
PyCellObject *cell;
3438
assert(PyCell_Check(op));
3539
cell = _Py_CAST(PyCellObject*, op);
40+
Py_BEGIN_CRITICAL_SECTION(cell);
3641
cell->ob_ref = value;
42+
Py_END_CRITICAL_SECTION();
3743
}
3844
#define PyCell_SET(op, value) PyCell_SET(_PyObject_CAST(op), (value))
3945

‎Objects/frameobject.c‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "pycore_moduleobject.h" // _PyModule_GetDict()
99
#include "pycore_modsupport.h" // _PyArg_CheckPositional()
1010
#include "pycore_object.h" // _PyObject_GC_UNTRACK()
11+
#include "pycore_cell.h" // PyCell_GetRef() PyCell_SetTakeRef()
1112
#include "pycore_opcode_metadata.h" // _PyOpcode_Deopt, _PyOpcode_Caches
1213

1314

@@ -187,11 +188,8 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value)
187188
}
188189
}
189190
if (cell != NULL) {
190-
PyObject *oldvalue_o = PyCell_GET(cell);
191-
if (value != oldvalue_o) {
192-
PyCell_SET(cell, Py_XNewRef(value));
193-
Py_XDECREF(oldvalue_o);
194-
}
191+
Py_XINCREF(value);
192+
PyCell_SetTakeRef((PyCellObject *)cell, value);
195193
} else if (value != PyStackRef_AsPyObjectBorrow(oldvalue)) {
196194
PyStackRef_XCLOSE(fast[i]);
197195
fast[i] = PyStackRef_FromPyObjectNew(value);
@@ -1987,19 +1985,25 @@ frame_get_var(_PyInterpreterFrame *frame, PyCodeObject *co, int i,
19871985
if (kind & CO_FAST_FREE) {
19881986
// The cell was set by COPY_FREE_VARS.
19891987
assert(value != NULL && PyCell_Check(value));
1990-
value = PyCell_GET(value);
1988+
value = PyCell_GetRef((PyCellObject *)value);
19911989
}
19921990
else if (kind & CO_FAST_CELL) {
19931991
if (value != NULL) {
19941992
if (PyCell_Check(value)) {
19951993
assert(!_PyFrame_IsIncomplete(frame));
1996-
value = PyCell_GET(value);
1994+
value = PyCell_GetRef((PyCellObject *)value);
1995+
}
1996+
else {
1997+
// (likely) Otherwise it is an arg (kind & CO_FAST_LOCAL),
1998+
// with the initial value set when the frame was created...
1999+
// (unlikely) ...or it was set via the f_locals proxy.
2000+
Py_INCREF(value);
19972001
}
1998-
// (likely) Otherwise it is an arg (kind & CO_FAST_LOCAL),
1999-
// with the initial value set when the frame was created...
2000-
// (unlikely) ...or it was set via the f_locals proxy.
20012002
}
20022003
}
2004+
else {
2005+
Py_XINCREF(value);
2006+
}
20032007
}
20042008
*pvalue = value;
20052009
return 1;
@@ -2076,14 +2080,14 @@ PyFrame_GetVar(PyFrameObject *frame_obj, PyObject *name)
20762080
continue;
20772081
}
20782082

2079-
PyObject *value; // borrowed reference
2083+
PyObject *value;
20802084
if (!frame_get_var(frame, co, i, &value)) {
20812085
break;
20822086
}
20832087
if (value == NULL) {
20842088
break;
20852089
}
2086-
return Py_NewRef(value);
2090+
return value;
20872091
}
20882092

20892093
PyErr_Format(PyExc_NameError, "variable %R does not exist", name);

‎Objects/typeobject.c‎

Lines changed: 38 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
#include "pycore_typeobject.h" // struct type_cache
2020
#include "pycore_unionobject.h" // _Py_union_type_or
2121
#include "pycore_weakref.h" // _PyWeakref_GET_REF()
22+
#include "pycore_cell.h" // PyCell_GetRef()
2223
#include "opcode.h" // MAKE_CELL
2324

2425
#include <stddef.h> // ptrdiff_t
@@ -11676,23 +11677,28 @@ super_init_without_args(_PyInterpreterFrame *cframe, PyTypeObject **type_p,
1167611677

1167711678
assert(_PyFrame_GetCode(cframe)->co_nlocalsplus > 0);
1167811679
PyObject *firstarg = PyStackRef_AsPyObjectBorrow(_PyFrame_GetLocalsArray(cframe)[0]);
11680+
if (firstarg == NULL) {
11681+
PyErr_SetString(PyExc_RuntimeError, "super(): arg[0] deleted");
11682+
return -1;
11683+
}
1167911684
// The first argument might be a cell.
11680-
if (firstarg != NULL && (_PyLocals_GetKind(co->co_localspluskinds, 0) & CO_FAST_CELL)) {
11681-
// "firstarg" is a cell here unless (very unlikely) super()
11682-
// was called from the C-API before the first MAKE_CELL op.
11683-
if (_PyInterpreterFrame_LASTI(cframe) >= 0) {
11684-
// MAKE_CELL and COPY_FREE_VARS have no quickened forms, so no need
11685-
// to use _PyOpcode_Deopt here:
11686-
assert(_PyCode_CODE(co)[0].op.code == MAKE_CELL ||
11687-
_PyCode_CODE(co)[0].op.code == COPY_FREE_VARS);
11688-
assert(PyCell_Check(firstarg));
11689-
firstarg = PyCell_GET(firstarg);
11685+
// "firstarg" is a cell here unless (very unlikely) super()
11686+
// was called from the C-API before the first MAKE_CELL op.
11687+
if ((_PyLocals_GetKind(co->co_localspluskinds, 0) & CO_FAST_CELL) &&
11688+
(_PyInterpreterFrame_LASTI(cframe) >= 0)) {
11689+
// MAKE_CELL and COPY_FREE_VARS have no quickened forms, so no need
11690+
// to use _PyOpcode_Deopt here:
11691+
assert(_PyCode_CODE(co)[0].op.code == MAKE_CELL ||
11692+
_PyCode_CODE(co)[0].op.code == COPY_FREE_VARS);
11693+
assert(PyCell_Check(firstarg));
11694+
firstarg = PyCell_GetRef((PyCellObject *)firstarg);
11695+
if (firstarg == NULL) {
11696+
PyErr_SetString(PyExc_RuntimeError, "super(): arg[0] deleted");
11697+
return -1;
1169011698
}
1169111699
}
11692-
if (firstarg == NULL) {
11693-
PyErr_SetString(PyExc_RuntimeError,
11694-
"super(): arg[0] deleted");
11695-
return -1;
11700+
else {
11701+
Py_INCREF(firstarg);
1169611702
}
1169711703

1169811704
// Look for __class__ in the free vars.
@@ -11707,18 +11713,22 @@ super_init_without_args(_PyInterpreterFrame *cframe, PyTypeObject **type_p,
1170711713
if (cell == NULL || !PyCell_Check(cell)) {
1170811714
PyErr_SetString(PyExc_RuntimeError,
1170911715
"super(): bad __class__ cell");
11716+
Py_DECREF(firstarg);
1171011717
return -1;
1171111718
}
11712-
type = (PyTypeObject *) PyCell_GET(cell);
11719+
type = (PyTypeObject *) PyCell_GetRef((PyCellObject *)cell);
1171311720
if (type == NULL) {
1171411721
PyErr_SetString(PyExc_RuntimeError,
1171511722
"super(): empty __class__ cell");
11723+
Py_DECREF(firstarg);
1171611724
return -1;
1171711725
}
1171811726
if (!PyType_Check(type)) {
1171911727
PyErr_Format(PyExc_RuntimeError,
1172011728
"super(): __class__ is not a type (%s)",
1172111729
Py_TYPE(type)->tp_name);
11730+
Py_DECREF(type);
11731+
Py_DECREF(firstarg);
1172211732
return -1;
1172311733
}
1172411734
break;
@@ -11727,6 +11737,7 @@ super_init_without_args(_PyInterpreterFrame *cframe, PyTypeObject **type_p,
1172711737
if (type == NULL) {
1172811738
PyErr_SetString(PyExc_RuntimeError,
1172911739
"super(): __class__ cell not found");
11740+
Py_DECREF(firstarg);
1173011741
return -1;
1173111742
}
1173211743

@@ -11773,16 +11784,24 @@ super_init_impl(PyObject *self, PyTypeObject *type, PyObject *obj) {
1177311784
return -1;
1177411785
}
1177511786
}
11787+
else {
11788+
Py_INCREF(type);
11789+
Py_XINCREF(obj);
11790+
}
1177611791

11777-
if (obj == Py_None)
11792+
if (obj == Py_None) {
11793+
Py_DECREF(obj);
1177811794
obj = NULL;
11795+
}
1177911796
if (obj != NULL) {
1178011797
obj_type = supercheck(type, obj);
11781-
if (obj_type == NULL)
11798+
if (obj_type == NULL) {
11799+
Py_DECREF(type);
11800+
Py_DECREF(obj);
1178211801
return -1;
11783-
Py_INCREF(obj);
11802+
}
1178411803
}
11785-
Py_XSETREF(su->type, (PyTypeObject*)Py_NewRef(type));
11804+
Py_XSETREF(su->type, (PyTypeObject*)type);
1178611805
Py_XSETREF(su->obj, obj);
1178711806
Py_XSETREF(su->obj_type, obj_type);
1178811807
return 0;

‎Python/bltinmodule.c‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include "pycore_pythonrun.h" // _Py_SourceAsString()
1414
#include "pycore_sysmodule.h" // _PySys_GetAttr()
1515
#include "pycore_tuple.h" // _PyTuple_FromArray()
16+
#include "pycore_cell.h" // PyCell_GetRef()
1617

1718
#include "clinic/bltinmodule.c.h"
1819

@@ -209,7 +210,7 @@ builtin___build_class__(PyObject *self, PyObject *const *args, Py_ssize_t nargs,
209210
PyObject *margs[3] = {name, bases, ns};
210211
cls = PyObject_VectorcallDict(meta, margs, 3, mkw);
211212
if (cls != NULL && PyType_Check(cls) && PyCell_Check(cell)) {
212-
PyObject *cell_cls = PyCell_GET(cell);
213+
PyObject *cell_cls = PyCell_GetRef((PyCellObject *)cell);
213214
if (cell_cls != cls) {
214215
if (cell_cls == NULL) {
215216
const char *msg =
@@ -221,9 +222,13 @@ builtin___build_class__(PyObject *self, PyObject *const *args, Py_ssize_t nargs,
221222
"__class__ set to %.200R defining %.200R as %.200R";
222223
PyErr_Format(PyExc_TypeError, msg, cell_cls, name, cls);
223224
}
225+
Py_XDECREF(cell_cls);
224226
Py_SETREF(cls, NULL);
225227
goto error;
226228
}
229+
else {
230+
Py_DECREF(cell_cls);
231+
}
227232
}
228233
}
229234
error:

0 commit comments

Comments
 (0)