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

Returning borrowed references is fundamentally unsafe #21

Open
markshannon opened this issue May 10, 2023 · 5 comments
Open

Returning borrowed references is fundamentally unsafe #21

markshannon opened this issue May 10, 2023 · 5 comments

Comments

@markshannon
Copy link

See also #5 and #11

Returning a borrowed reference is fundamentally unsafe.

There are conditions where it can be done safely, but each case requires careful analysis, and is often, if not usually, the case that the analysis is done incorrectly.

For example, #5 (comment) suggests that it is fine to return a borrowed reference to a tuple element.
However, it is only safe if a reference to the tuple is held on the stack. If the only reference to the tuple is on the heap, then borrowing a reference to an element of the tuple is unsafe, as mutation of a heap object could result in the reference to the tuple, and its element vanishing.

I think it is better to say that returning a borrowed reference is always unsafe, than rely on flawed assumptions about performance and faulty reasoning about lifetimes.

@vstinner
Copy link
Contributor

For me, returning a borrowed reference is the same class of bug than issue #57: implicit resource management, it's unclear until when a resource is valid or not.

For the general case of "returning a pointer", I proposed a generic PyResource API which requires calling PyResource_Release() function when the caller is done with the resource: python/cpython#106592

For the specific case of PyObject* borrowed reference, I think that the fix is easier: just return a PyObject* strong reference instead. The caller is expected to call Py_DECREF() to "release the resource".

@vstinner
Copy link
Contributor

My notes on borrowed references: https://pythoncapi.readthedocs.io/bad_api.html#borrowed-references

I added PyModule_AddObjectRef() and PyWeakref_GetRef(), variants of PyModule_AddObject() and PyWeakref_GetObject(), which return a strong reference, instead of a borrowed reference.

In PR python/cpython#106005, I propose adding PyDict_GetItemRef() function, variant of PyDict_GetItemWithError() returning a strong reference.

@iritkatriel iritkatriel added the v label Jul 20, 2023
@gvanrossum
Copy link

Cross-reference python/steering-council#201 where the SC decided to delegate the naming convention to the C-API WG. @vstinner If you have a preference for a consistent naming scheme (perhaps based on APIs you've already added), now would be the time to propose it here.

@vstinner
Copy link
Contributor

For function returning a borrowed reference, when a variant returning a strong reference is added, so far the Ref suffix was used/added:

  • Python 3.10: PyModule_AddObjectRef() VS PyModule_AddObject()
  • Python 3.13: PyImport_AddModuleRef() VS PyImport_AddModule()
  • Python 3.13: PyWeakref_GetRef() VS PyWeakref_GetObject()
  • Python 3.13: PyDictGetItemRef() VS PyDictGetItem()
  • Python 3.13: PyDictGetItemStringRef() VS PyDictGetItemString()

@iritkatriel iritkatriel removed the v label Oct 23, 2023
@encukou
Copy link
Contributor

encukou commented Oct 23, 2023

Issue for proposed guidelines: capi-workgroup/api-evolution#16

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

5 participants