Skip to content

Commit 08d7bd2

Browse files
eendebakptblurb-it[bot]efimov-mikhailvstinner
authored
gh-140232: Do not track frozenset objects with immutables (#140234)
Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Mikhail Efimov <efimov.mikhail@gmail.com> Co-authored-by: Victor Stinner <vstinner@python.org>
1 parent 1a637b2 commit 08d7bd2

File tree

5 files changed

+114
-3
lines changed

5 files changed

+114
-3
lines changed

Lib/test/test_capi/test_set.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,12 @@ def test_add(self):
166166
# CRASHES: add(instance, NULL)
167167
# CRASHES: add(NULL, NULL)
168168

169+
def test_add_frozenset(self):
170+
add = _testlimitedcapi.set_add
171+
frozen_set = frozenset()
172+
# test adding an element to a non-uniquely referenced frozenset throws an exception
173+
self.assertRaises(SystemError, add, frozen_set, 1)
174+
169175
def test_discard(self):
170176
discard = _testlimitedcapi.set_discard
171177
for cls in (set, set_subclass):

Lib/test/test_sys.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1880,7 +1880,10 @@ class S(set):
18801880
check(S(), set(), '3P')
18811881
class FS(frozenset):
18821882
__slots__ = 'a', 'b', 'c'
1883-
check(FS(), frozenset(), '3P')
1883+
1884+
class mytuple(tuple):
1885+
pass
1886+
check(FS([mytuple()]), frozenset([mytuple()]), '3P')
18841887
from collections import OrderedDict
18851888
class OD(OrderedDict):
18861889
__slots__ = 'a', 'b', 'c'
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Frozenset objects with immutable elements are no longer tracked by the garbage collector.

Modules/_testlimitedcapi/set.c

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,72 @@ test_frozenset_add_in_capi(PyObject *self, PyObject *Py_UNUSED(obj))
155155
return NULL;
156156
}
157157

158+
static PyObject *
159+
raiseTestError(const char* test_name, const char* msg)
160+
{
161+
PyErr_Format(PyExc_AssertionError, "%s: %s", test_name, msg);
162+
return NULL;
163+
}
164+
165+
static PyObject *
166+
test_frozenset_add_in_capi_tracking_immutable(PyObject *self, PyObject *Py_UNUSED(ignored))
167+
{
168+
// Test: GC tracking - frozenset with only immutable items should not be tracked
169+
PyObject *frozenset = PyFrozenSet_New(NULL);
170+
if (frozenset == NULL) {
171+
return NULL;
172+
}
173+
if (PySet_Add(frozenset, Py_True) < 0) {
174+
Py_DECREF(frozenset);
175+
return NULL;
176+
}
177+
if (PyObject_GC_IsTracked(frozenset)) {
178+
Py_DECREF(frozenset);
179+
return raiseTestError("test_frozenset_add_in_capi_tracking_immutable",
180+
"frozenset with only bool should not be GC tracked");
181+
}
182+
Py_DECREF(frozenset);
183+
Py_RETURN_NONE;
184+
}
185+
186+
static PyObject *
187+
test_frozenset_add_in_capi_tracking(PyObject *self, PyObject *Py_UNUSED(ignored))
188+
{
189+
// Test: GC tracking - frozenset with tracked object should be tracked
190+
PyObject *frozenset = PyFrozenSet_New(NULL);
191+
if (frozenset == NULL) {
192+
return NULL;
193+
}
194+
195+
PyObject *tracked_obj = PyErr_NewException("_testlimitedcapi.py_set_add", NULL, NULL);
196+
if (tracked_obj == NULL) {
197+
goto error;
198+
}
199+
if (!PyObject_GC_IsTracked(tracked_obj)) {
200+
Py_DECREF(frozenset);
201+
Py_DECREF(tracked_obj);
202+
return raiseTestError("test_frozenset_add_in_capi_tracking",
203+
"test object should be tracked");
204+
}
205+
if (PySet_Add(frozenset, tracked_obj) < 0) {
206+
goto error;
207+
}
208+
Py_DECREF(tracked_obj);
209+
if (!PyObject_GC_IsTracked(frozenset)) {
210+
Py_DECREF(frozenset);
211+
return raiseTestError("test_frozenset_add_in_capi_tracking",
212+
"frozenset with with GC tracked object should be tracked");
213+
}
214+
Py_DECREF(frozenset);
215+
Py_RETURN_NONE;
216+
217+
error:
218+
Py_DECREF(frozenset);
219+
Py_XDECREF(tracked_obj);
220+
return NULL;
221+
}
222+
223+
158224
static PyObject *
159225
test_set_contains_does_not_convert_unhashable_key(PyObject *self, PyObject *Py_UNUSED(obj))
160226
{
@@ -219,6 +285,8 @@ static PyMethodDef test_methods[] = {
219285
{"set_clear", set_clear, METH_O},
220286

221287
{"test_frozenset_add_in_capi", test_frozenset_add_in_capi, METH_NOARGS},
288+
{"test_frozenset_add_in_capi_tracking", test_frozenset_add_in_capi_tracking, METH_NOARGS},
289+
{"test_frozenset_add_in_capi_tracking_immutable", test_frozenset_add_in_capi_tracking_immutable, METH_NOARGS},
222290
{"test_set_contains_does_not_convert_unhashable_key",
223291
test_set_contains_does_not_convert_unhashable_key, METH_NOARGS},
224292

Objects/setobject.c

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1368,6 +1368,26 @@ make_new_set_basetype(PyTypeObject *type, PyObject *iterable)
13681368
return make_new_set(type, iterable);
13691369
}
13701370

1371+
// gh-140232: check whether a frozenset can be untracked from the GC
1372+
static void
1373+
_PyFrozenSet_MaybeUntrack(PyObject *op)
1374+
{
1375+
assert(op != NULL);
1376+
// subclasses of a frozenset can generate reference cycles, so do not untrack
1377+
if (!PyFrozenSet_CheckExact(op)) {
1378+
return;
1379+
}
1380+
// if no elements of a frozenset are tracked by the GC, we untrack the object
1381+
Py_ssize_t pos = 0;
1382+
setentry *entry;
1383+
while (set_next((PySetObject *)op, &pos, &entry)) {
1384+
if (_PyObject_GC_MAY_BE_TRACKED(entry->key)) {
1385+
return;
1386+
}
1387+
}
1388+
_PyObject_GC_UNTRACK(op);
1389+
}
1390+
13711391
static PyObject *
13721392
make_new_frozenset(PyTypeObject *type, PyObject *iterable)
13731393
{
@@ -1379,7 +1399,11 @@ make_new_frozenset(PyTypeObject *type, PyObject *iterable)
13791399
/* frozenset(f) is idempotent */
13801400
return Py_NewRef(iterable);
13811401
}
1382-
return make_new_set(type, iterable);
1402+
PyObject *obj = make_new_set(type, iterable);
1403+
if (obj != NULL) {
1404+
_PyFrozenSet_MaybeUntrack(obj);
1405+
}
1406+
return obj;
13831407
}
13841408

13851409
static PyObject *
@@ -2932,7 +2956,11 @@ PySet_New(PyObject *iterable)
29322956
PyObject *
29332957
PyFrozenSet_New(PyObject *iterable)
29342958
{
2935-
return make_new_set(&PyFrozenSet_Type, iterable);
2959+
PyObject *result = make_new_set(&PyFrozenSet_Type, iterable);
2960+
if (result != NULL) {
2961+
_PyFrozenSet_MaybeUntrack(result);
2962+
}
2963+
return result;
29362964
}
29372965

29382966
Py_ssize_t
@@ -3010,6 +3038,11 @@ PySet_Add(PyObject *anyset, PyObject *key)
30103038
// API limits the usage of `PySet_Add` to "fill in the values of brand
30113039
// new frozensets before they are exposed to other code". In this case,
30123040
// this can be done without a lock.
3041+
// Since another key is added to the set, we must track the frozenset
3042+
// if needed.
3043+
if (PyFrozenSet_CheckExact(anyset) && !PyObject_GC_IsTracked(anyset) && PyObject_GC_IsTracked(key)) {
3044+
_PyObject_GC_TRACK(anyset);
3045+
}
30133046
return set_add_key((PySetObject *)anyset, key);
30143047
}
30153048

0 commit comments

Comments
 (0)