Skip to content

gh-80384: Check that callback is callable at weak reference creation #151145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
serhiy-storchaka merged 2 commits into from
Jun 10, 2026
Merged

gh-80384: Check that callback is callable at weak reference creation #151145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06145a6
gh-80384: Check that callback is callable at weak reference creation
serhiy-storchaka Jun 9, 2026
88afc02
Fix test_recursion_error_during_traceback in test_traceback.
serhiy-storchaka Jun 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions Doc/c-api/weakref.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,11 @@ as much as it can.
should accept a single parameter, which will be the weak reference object
itself. *callback* may also be ``None`` or ``NULL``. If *ob* is not a
weakly referenceable object, or if *callback* is not callable, ``None``, or
``NULL``, this will return ``NULL`` and raise :exc:`TypeError`.
``NULL``, this will raise :exc:`TypeError` and return ``NULL``.

.. versionchanged:: next
Raise :exc:`!TypeError` if *callback* is not callable, ``None``, or
``NULL``.

.. seealso::
:c:func:`PyType_SUPPORTS_WEAKREFS` for checking if *ob* is weakly
Expand All @@ -59,7 +63,11 @@ as much as it can.
collected; it should accept a single parameter, which will be the weak
reference object itself. *callback* may also be ``None`` or ``NULL``. If *ob*
is not a weakly referenceable object, or if *callback* is not callable,
``None``, or ``NULL``, this will return ``NULL`` and raise :exc:`TypeError`.
``NULL``, this will raise :exc:`TypeError` and return ``NULL``.

.. versionchanged:: next
Raise :exc:`!TypeError` if *callback* is not callable, ``None``, or
``NULL``.

.. seealso::
:c:func:`PyType_SUPPORTS_WEAKREFS` for checking if *ob* is weakly
Expand Down
6 changes: 6 additions & 0 deletions Doc/library/weakref.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@ See :ref:`__slots__ documentation <slots>` for details.
.. versionchanged:: 3.4
Added the :attr:`__callback__` attribute.

.. versionchanged:: next
Raise :exc:`!TypeError` if *callback* is not callable or ``None``.


.. function:: proxy(object[, callback])

Expand All @@ -151,6 +154,9 @@ See :ref:`__slots__ documentation <slots>` for details.
Extended the operator support on proxy objects to include the matrix
multiplication operators ``@`` and ``@=``.

.. versionchanged:: next
Raise :exc:`!TypeError` if *callback* is not callable or ``None``.


.. function:: getweakrefcount(object)

Expand Down
2 changes: 2 additions & 0 deletions Lib/test/test_capi/test_weakref.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ def test_pyweakref_newref(self):
# PyWeakref_NewRef() handles None callback as NULL callback
wr = newref(obj, None)
self.assertIs(type(wr), weakref.ReferenceType)
self.assertRaises(TypeError, newref, obj, 42)
log = []
wr = newref(obj, log.append)
self.assertIs(type(wr), weakref.ReferenceType)
Expand All @@ -116,6 +117,7 @@ def test_pyweakref_newproxy(self):
# PyWeakref_NewProxy() handles None callback as NULL callback
wp = newproxy(obj, None)
self.assertIs(type(wp), weakref.ProxyType)
self.assertRaises(TypeError, newproxy, obj, 42)
log = []
wp = newproxy(obj, log.append)
self.assertIs(type(wp), weakref.ProxyType)
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/test_traceback.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ def test_recursion_error_during_traceback(self):
sys.setrecursionlimit(15)
def f():
ref(lambda: 0, [])
ref(lambda: 0, ord)
f()
Comment on lines 208 to 210

@vstinner vstinner Jun 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test expects that calling the callback raises an exception. Currently, it raises a TypeError since [] (list) is not callable. I suggest making the exception more explicit:

                def callback(wr):
                    raise Exception("error")

                def f():
                    ref(lambda: 0, callback)
                    f()

Passing ord() is more surprising to me.

@serhiy-storchaka serhiy-storchaka Jun 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I correctly understand the purpose of this test, we cannot do this. This would insert a Python frame, this would give a non-NULL source_line. This is why the test used such indirect way of raising an exception.

I am not completely sure in my reconstruction, this is why I have not added an explaining comment, which would be helpful here.

@vstinner vstinner Jun 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test was added by commit c836231.

At first ref(lambda: 0, []) calls, display_source_line() returns rc=0, no exception set, source_line is not NULL. But when we are getting closer to the recursion limit, display_source_line() can fail with RecursionError on the call:

    fob = _PyObject_CallMethod(io, &_Py_ID(TextIOWrapper),
                               "Os", binary, encoding);

In this case, display_source_line() clears the exception, returns 0 and source_line is NULL. Calling ignore_source_errors() here just sets err=0 (no exception is set), but err is already equal to 0. So I don't understand well what we are testing here.

@serhiy-storchaka serhiy-storchaka Jun 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not confident in my explanation, and adding incorrect comment may be worse than missing comment.

try:
Expand Down
5 changes: 5 additions & 0 deletions Lib/test/test_weakref.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,11 @@ def test_basic_callback(self):
self.check_basic_callback(create_function)
self.check_basic_callback(create_bound_method)

def test_non_callable_callback(self):
c = C()
self.assertRaises(TypeError, weakref.ref, c, 42)
self.assertRaises(TypeError, weakref.proxy, c, 42)

@support.cpython_only
def test_cfunction(self):
_testcapi = import_helper.import_module("_testcapi")
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/C_API/2026-06-09-14-50-35.gh-issue-80384.UEGyvB.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
:c:func:`PyWeakref_NewRef` and :c:func:`PyWeakref_NewProxy` now raise
:exc:`TypeError` if the *callback* argument is not callable, ``None``, or
``NULL``.
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2026-06-09-14-49-17.gh-issue-80384.ttRAja.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
:func:`weakref.ref` and :func:`weakref.proxy` now raise :exc:`TypeError` if
the *callback* argument is not callable or ``None``.
9 changes: 8 additions & 1 deletion Objects/weakrefobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,8 +416,15 @@ get_or_create_weakref(PyTypeObject *type, PyObject *obj, PyObject *callback)
Py_TYPE(obj)->tp_name);
return NULL;
}
if (callback == Py_None)
if (callback == Py_None) {
callback = NULL;
}
if (callback != NULL && !PyCallable_Check(callback)) {
PyErr_Format(PyExc_TypeError,
"callback must be callable or None, not '%T'",
callback);
return NULL;
}

PyWeakReference **list = GET_WEAKREFS_LISTPTR(obj);
if ((type == &_PyWeakref_RefType) ||
Expand Down
Loading