From 85026c540060a8ce917eefce21902477d323a946 Mon Sep 17 00:00:00 2001 From: dgelessus Date: Thu, 16 May 2019 17:47:51 +0200 Subject: [PATCH 1/4] bpo-36880: Add failing test, fix to follow shortly --- Lib/ctypes/test/test_refcounts.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/Lib/ctypes/test/test_refcounts.py b/Lib/ctypes/test/test_refcounts.py index f2edfa6400ef24..1ebceebd184740 100644 --- a/Lib/ctypes/test/test_refcounts.py +++ b/Lib/ctypes/test/test_refcounts.py @@ -97,5 +97,31 @@ def func(a, b): f(1, 2) self.assertEqual(sys.getrefcount(ctypes.c_int), a) + @support.refcount_test + def test_callback_py_object_none_return(self): + """Test that returning ``None`` from a ``py_object`` callback + does not affect ``None``'s refcount (bpo-36880).""" + + import sys + + for FUNCTYPE in (ctypes.CFUNCTYPE, ctypes.PYFUNCTYPE): + with self.subTest(FUNCTYPE=FUNCTYPE): + proto = FUNCTYPE(ctypes.py_object) + @proto + def func(): + return None + + # Check that calling func does not affect None's refcount. + none_refcount = sys.getrefcount(None) + # Because None's refcount can also change for other reasons, + # we call func in a loop to ensure that any effects on None's + # refcount are clearly visible. + for _ in range(10000): + func() + # Allow for small variations in None's refcount from other + # sources. + self.assertAlmostEqual( + sys.getrefcount(None), none_refcount, delta=50) + if __name__ == '__main__': unittest.main() From 2a5aeba96ee19acb81989a8e07e009ec7ab0cfb6 Mon Sep 17 00:00:00 2001 From: dgelessus Date: Thu, 16 May 2019 19:36:46 +0200 Subject: [PATCH 2/4] bpo-36880: Fix refcount issue with ctypes callbacks returning None Thank you to Eryk Sun for finding the cause of the bug and providing the code fix. --- .../2019-05-13-11-37-30.bpo-36880.ZgBgH0.rst | 2 ++ Modules/_ctypes/callbacks.c | 15 ++++++++------- 2 files changed, 10 insertions(+), 7 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2019-05-13-11-37-30.bpo-36880.ZgBgH0.rst diff --git a/Misc/NEWS.d/next/Library/2019-05-13-11-37-30.bpo-36880.ZgBgH0.rst b/Misc/NEWS.d/next/Library/2019-05-13-11-37-30.bpo-36880.ZgBgH0.rst new file mode 100644 index 00000000000000..f65323813d0543 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-05-13-11-37-30.bpo-36880.ZgBgH0.rst @@ -0,0 +1,2 @@ +Fix a reference counting issue when a :mod:`ctypes` callback with return +type :class:`~ctypes.py_object` returns ``None``, which could cause crashes. diff --git a/Modules/_ctypes/callbacks.c b/Modules/_ctypes/callbacks.c index 9f793c2771bfb9..8250b382a0e251 100644 --- a/Modules/_ctypes/callbacks.c +++ b/Modules/_ctypes/callbacks.c @@ -270,15 +270,16 @@ if (x == NULL) _PyTraceback_Add(what, "_ctypes/callbacks.c", __LINE__ - 1), PyEr be the result. EXCEPT when restype is py_object - Python itself knows how to manage the refcount of these objects. */ - if (keep == NULL) /* Could not convert callback result. */ + if (keep == NULL) { /* Could not convert callback result. */ PyErr_WriteUnraisable(callable); - else if (keep == Py_None) /* Nothing to keep */ - Py_DECREF(keep); - else if (setfunc != _ctypes_get_fielddesc("O")->setfunc) { - if (-1 == PyErr_WarnEx(PyExc_RuntimeWarning, - "memory leak in callback function.", - 1)) + } else if (setfunc != _ctypes_get_fielddesc("O")->setfunc) { + if (keep == Py_None) { /* Nothing to keep */ + Py_DECREF(keep); + } else if (PyErr_WarnEx(PyExc_RuntimeWarning, + "memory leak in callback function.", + 1) == -1) { PyErr_WriteUnraisable(callable); + } } } Py_XDECREF(result); From fb17c68cd049f718b75fca287a6c058dcd4c04f6 Mon Sep 17 00:00:00 2001 From: dgelessus Date: Tue, 3 Jan 2023 13:15:26 +0100 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Eryk Sun --- Lib/test/test_ctypes/test_refcounts.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_ctypes/test_refcounts.py b/Lib/test/test_ctypes/test_refcounts.py index 1ebceebd184740..c7f501c0b1c0c4 100644 --- a/Lib/test/test_ctypes/test_refcounts.py +++ b/Lib/test/test_ctypes/test_refcounts.py @@ -99,15 +99,14 @@ def func(a, b): @support.refcount_test def test_callback_py_object_none_return(self): - """Test that returning ``None`` from a ``py_object`` callback - does not affect ``None``'s refcount (bpo-36880).""" + # bpo-36880: test that returning None from a py_object callback + # does not decrement the refcount of None. import sys for FUNCTYPE in (ctypes.CFUNCTYPE, ctypes.PYFUNCTYPE): with self.subTest(FUNCTYPE=FUNCTYPE): - proto = FUNCTYPE(ctypes.py_object) - @proto + @FUNCTYPE(ctypes.py_object) def func(): return None From 66ea175d778b6de5f37c998162d9cf7181c58635 Mon Sep 17 00:00:00 2001 From: dgelessus Date: Tue, 3 Jan 2023 13:19:01 +0100 Subject: [PATCH 4/4] Remove manual sys.getrefcount check in favor of regrtest --huntrleaks --- Lib/test/test_ctypes/test_refcounts.py | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/Lib/test/test_ctypes/test_refcounts.py b/Lib/test/test_ctypes/test_refcounts.py index c7f501c0b1c0c4..48958cd2a60196 100644 --- a/Lib/test/test_ctypes/test_refcounts.py +++ b/Lib/test/test_ctypes/test_refcounts.py @@ -102,8 +102,6 @@ def test_callback_py_object_none_return(self): # bpo-36880: test that returning None from a py_object callback # does not decrement the refcount of None. - import sys - for FUNCTYPE in (ctypes.CFUNCTYPE, ctypes.PYFUNCTYPE): with self.subTest(FUNCTYPE=FUNCTYPE): @FUNCTYPE(ctypes.py_object) @@ -111,16 +109,8 @@ def func(): return None # Check that calling func does not affect None's refcount. - none_refcount = sys.getrefcount(None) - # Because None's refcount can also change for other reasons, - # we call func in a loop to ensure that any effects on None's - # refcount are clearly visible. for _ in range(10000): func() - # Allow for small variations in None's refcount from other - # sources. - self.assertAlmostEqual( - sys.getrefcount(None), none_refcount, delta=50) if __name__ == '__main__': unittest.main()