capi-workgroup / problems

Discussions about problems with the current C Api
19 stars 6 forks source link

Returning borrowed references is fundamentally unsafe #21

Open markshannon opened 1 year ago

markshannon commented 1 year ago

See also https://github.com/capi-workgroup/problems/issues/5 and https://github.com/capi-workgroup/problems/issues/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, https://github.com/capi-workgroup/problems/issues/5#issuecomment-1541599164 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 commented 1 year ago

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: https://github.com/python/cpython/issues/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 commented 1 year ago

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 https://github.com/python/cpython/pull/106005, I propose adding PyDict_GetItemRef() function, variant of PyDict_GetItemWithError() returning a strong reference.

gvanrossum commented 1 year ago

Cross-reference https://github.com/python/steering-council/issues/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 commented 1 year ago

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

encukou commented 1 year ago

Issue for proposed guidelines: https://github.com/capi-workgroup/api-evolution/issues/16