Skip to content
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

Argument accepting a Python object or NULL: error prone API #47

Open
vstinner opened this issue Jun 9, 2023 · 2 comments
Open

Argument accepting a Python object or NULL: error prone API #47

vstinner opened this issue Jun 9, 2023 · 2 comments

Comments

@vstinner
Copy link
Contributor

vstinner commented Jun 9, 2023

Python has multiple C API functions which accept a Python or NULL, both are valid:

  • PyObject_SetAttr(obj, attr_name, NULL)
  • PyObject_SetAttrString(obj, attr_name, NULL)
  • PySequence_SetItem(obj, i, NULL)
  • PySequence_SetSlice(obj, i1, i2, NULL)

The problem is: what if creating a value fails and the code pass NULL instead of a Python object, without checking for failure?

The following code sets the number attribute to 123... or deletes the number attribute if PyLong_FromLong() fails (eg. MemoryError).

PyObject *num = PyLong_FromLong(123);
PyObject_SetAttrString(obj, "number", num);
Py_XDECREF(num);
// surprise surprise, was an exception raised or not?

// (... more code ...)

// let's make it even more funny, make the exception silent!
// There are many functions calling PyErr_Clear() for various reasons.
PyErr_Clear();
@vstinner
Copy link
Contributor Author

vstinner commented Jun 9, 2023

See also python/cpython#105375 "Incorrect error handling for APIs that can raise exceptions".

@encukou
Copy link
Contributor

encukou commented Aug 3, 2023

Let's not design new API like this, but IMO this is not a good reason to remove the existing functions.

what if creating a value fails and the code pass NULL instead of a Python object, without checking for failure?

You get surprising but well-defined behaviour -- deletion of an item/attribute. Or the function could do assert(!PyErr_Occurred()) – fail in debug mode.
These is much better than most cases of accidentally sticking NULL somewhere it shouldn't go. It doesn't need to be singled out. The problem is with the user not checking for failure.

A variant of this issue that actually is problematic is tp_setattr, where a user-defined function needs to expect NULL.

@iritkatriel iritkatriel removed the v label Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants