From 195a7e59cff10be13bc0473bb43517f340d6de35 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 26 Apr 2024 16:18:27 -0700 Subject: [PATCH 1/5] Don't raise an error if tuple allocation fails when clearing weakrefs It's not safe to raise an exception in `PyObject_ClearWeakRefs()` if one is not already set, since it may be called by `_Py_Dealloc()` and hit https://github.com/python/cpython/blob/5a90de0d4cbc151a6deea36a27eb81b192410e56/Objects/object.c#L2843-L2860. Additionally, make sure we clear the weakrefs even when tuple allocation fails. This bug predates gh-111926. If it's getting tickled now, I suspect it's because we always allocate a tuple when clearing weakrefs that have callbacks. --- Lib/test/test_weakref.py | 27 +++++++++++++++++++++++++++ Objects/weakrefobject.c | 8 +++++++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py index 499ba77fd19542..61cbc935c1e546 100644 --- a/Lib/test/test_weakref.py +++ b/Lib/test/test_weakref.py @@ -10,6 +10,7 @@ import threading import time import random +import textwrap from test import support from test.support import script_helper, ALWAYS_EQ @@ -1009,6 +1010,32 @@ def __del__(self): pass del x support.gc_collect() + @support.cpython_only + def test_no_memory_when_clearing(self): + # gh-118331: Make sure we do not raise an exception from the destructor + # when clearing weakrefs if allocating the intermediate tuple fails. + code = textwrap.dedent(""" + import _testcapi + import weakref + + class Obj: + pass + + def callback(obj): + pass + + + obj = Obj() + # The choice of 50 is arbitrary, but must be large enough to ensure + # the allocation won't be serviced by the free list. + wrs = [weakref.ref(obj, callback) for _ in range(50)] + _testcapi.set_nomemory(0) + del obj + """).strip() + res = script_helper.assert_python_failure("-c", code) + stderr = res.err.decode("ascii", "backslashreplace") + self.assertNotRegex(stderr, "_Py_Dealloc: Deallocator of type") + class SubclassableWeakrefTestCase(TestBase): diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 206107e8505dc7..7ee8a8e5a59fbc 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -1016,7 +1016,13 @@ PyObject_ClearWeakRefs(PyObject *object) PyObject *exc = PyErr_GetRaisedException(); PyObject *tuple = PyTuple_New(num_weakrefs * 2); if (tuple == NULL) { - _PyErr_ChainExceptions1(exc); + _PyWeakref_ClearWeakRefsExceptCallbacks(object); + if (exc == NULL) { + PyErr_WriteUnraisable(object); + } + else { + _PyErr_ChainExceptions1(exc); + } return; } From 99e3f854df936e98665a99f83eb36816f3be4a9c Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 26 Apr 2024 16:37:13 -0700 Subject: [PATCH 2/5] Fix test --- Lib/test/test_weakref.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py index 61cbc935c1e546..f42e63b250d629 100644 --- a/Lib/test/test_weakref.py +++ b/Lib/test/test_weakref.py @@ -1032,7 +1032,7 @@ def callback(obj): _testcapi.set_nomemory(0) del obj """).strip() - res = script_helper.assert_python_failure("-c", code) + res = script_helper.assert_python_ok("-c", code) stderr = res.err.decode("ascii", "backslashreplace") self.assertNotRegex(stderr, "_Py_Dealloc: Deallocator of type") From 632087c085e0f571a435501e03ed294675f36eb6 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 26 Apr 2024 17:46:42 -0700 Subject: [PATCH 3/5] Make test more robust to future changes Depending on finalization, we may encounter another allocation failure that causes the interpreter to exit with a non-zero status. That's OK, we just need to verify that we're not exiting because weakref clearing raised an exception. --- Lib/test/test_weakref.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py index f42e63b250d629..a2f5b9b2902d36 100644 --- a/Lib/test/test_weakref.py +++ b/Lib/test/test_weakref.py @@ -1018,23 +1018,22 @@ def test_no_memory_when_clearing(self): import _testcapi import weakref - class Obj: + class TestObj: pass def callback(obj): pass - - obj = Obj() + obj = TestObj() # The choice of 50 is arbitrary, but must be large enough to ensure # the allocation won't be serviced by the free list. wrs = [weakref.ref(obj, callback) for _ in range(50)] _testcapi.set_nomemory(0) del obj """).strip() - res = script_helper.assert_python_ok("-c", code) + res, _ = script_helper.run_python_until_end("-c", code) stderr = res.err.decode("ascii", "backslashreplace") - self.assertNotRegex(stderr, "_Py_Dealloc: Deallocator of type") + self.assertNotRegex(stderr, "_Py_Dealloc: Deallocator of type 'TestObj'") class SubclassableWeakrefTestCase(TestBase): From 7ba419dca085c04a7de2d8e794b71557959ee4d2 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Fri, 26 Apr 2024 18:11:36 -0700 Subject: [PATCH 4/5] Don't pass object to `PyErr_WriteUnraisable` --- Objects/weakrefobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 7ee8a8e5a59fbc..331b29722c9e3b 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -1018,7 +1018,7 @@ PyObject_ClearWeakRefs(PyObject *object) if (tuple == NULL) { _PyWeakref_ClearWeakRefsExceptCallbacks(object); if (exc == NULL) { - PyErr_WriteUnraisable(object); + PyErr_WriteUnraisable(NULL); } else { _PyErr_ChainExceptions1(exc); From 15c85d453f12cefa64f3142685e01edb992d5b20 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 29 Apr 2024 09:24:35 -0700 Subject: [PATCH 5/5] Don't chain exceptions --- Objects/weakrefobject.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 331b29722c9e3b..93c5fe3aacecfd 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -1017,12 +1017,8 @@ PyObject_ClearWeakRefs(PyObject *object) PyObject *tuple = PyTuple_New(num_weakrefs * 2); if (tuple == NULL) { _PyWeakref_ClearWeakRefsExceptCallbacks(object); - if (exc == NULL) { - PyErr_WriteUnraisable(NULL); - } - else { - _PyErr_ChainExceptions1(exc); - } + PyErr_WriteUnraisable(NULL); + PyErr_SetRaisedException(exc); return; }