Skip to content

gh-135075: Make PyObject_SetAttr() fail with NULL value and exception #136180

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.

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jul 1, 2025

Make PyObject_SetAttr() and PyObject_SetAttrString() fail if called with NULL value and an exception set.


📚 Documentation preview 📚: https://cpython-previews--136180.org.readthedocs.build/

…eption

Make PyObject_SetAttr() and PyObject_SetAttrString() fail if called
with NULL value and an exception set.
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Generally looks good to me.

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

I think that Modules/_testcapi/abstract.c is better place than Modules/_testcapi/object.c.

@@ -197,6 +197,10 @@ Object Protocol
in favour of using :c:func:`PyObject_DelAttr`, but there are currently no
plans to remove it.
The function must not be called with ``NULL`` *v* and an an exception set.
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph and the previous one are about the same case -- when v is NULL. You can pass NULL, but this is soft-deprecated, and no error should be set. Why not merge them?

And maybe add versionchanged?

obj.attr = 123
with self.assertRaises(SystemError):
_testcapi.object_setattr_null_exc(obj, 'attr')
self.assertTrue(hasattr(obj, 'attr'))
Copy link
Member

Choose a reason for hiding this comment

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

assertHasAttr

Also check __context__ or __cause__ of the exception.

return NULL;
}

PyErr_SetString(PyExc_ValueError, "error");
Copy link
Member

Choose a reason for hiding this comment

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

You can pass an exception as argument. Then you can check __context__ or __cause__ of SystemError by identity.

{
PyObject *obj;
const char *name;
if (!PyArg_ParseTuple(args, "Os", &obj, &name)) {
Copy link
Member

Choose a reason for hiding this comment

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

Use z#. So you can test with undecodable name -- this will allow to test that the value is tested for NULL in PyObject_SetAttrString, not only in PyObject_SetAttr, and that it is tested before attempting to decode the name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants