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.

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

fix: Improve PyCapsule exception handling #4232

Merged
merged 3 commits into from
Oct 11, 2022
Merged

fix: Improve PyCapsule exception handling #4232

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b42b6aa
Improve pycapsule error handling corner cases
Skylion007 Oct 10, 2022
1a12aea
Handle another corner case
Skylion007 Oct 10, 2022
94d47a1
Simplify err handling code
Skylion007 Oct 10, 2022
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
4 changes: 2 additions & 2 deletions include/pybind11/pytypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1829,7 +1829,7 @@ class capsule : public object {
// guard if destructor called while err indicator is set
error_scope error_guard;
auto destructor = reinterpret_cast<void (*)(void *)>(PyCapsule_GetContext(o));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line was introduced with PR #752:

https://github.com/pybind/pybind11/pull/752/files

@wjakob Why is this PyCapsule_GetContext() and not PyCapsule_GetDestructor()?

I see it's self-consistent within pybind11, PyCapsule_SetContext() is used here:

pybind11/include/pybind11/pytypes.h

Line 1846 in da104a9

if (!m_ptr || PyCapsule_SetContext(m_ptr, (void *) destructor) != 0) {

But I'd expect that the rest of the world handles destructors via PyCapsule_SetDestructor() & PyCapsule_GetDestructor(). Could this lead to leaks, because we're never calling PyCapsule_GetDestructor() anywhere in pybind11?

I just looked, even Python 2.7 had PyCapsule_SetDestructor() & PyCapsule_GetDestructor() already.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It turns out my worries here were unfounded. Based on feedback from @wjakob, I came to realize that my core misunderstanding was that py::capsule is controlling the lifetime (in a C++ dtor) but there is no C++ dtor here and the lifetime it is (of course) tied to the Python refcount of the capsule. This is connected to having misunderstood the nullptr for the 2nd argument of PyCapsule_New, that is the name, not the destructor.

I.e. everything is fine here. But having looked at this pretty closely now, I decided this minor cleanup will be useful: #4238

if (PyErr_Occurred()) {
if (destructor == nullptr && PyErr_Occurred()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Main thing here is PyErr_Occurred() is a much more expensive call, especially on the common path so might as well avoid doing so if possible

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm ... but what if the error indicator is set (for whatever reason)? We're leaving the process in an invalid state, follow-on Python C API calls may behave in confusing ways (painful debugging memories come back).

If the Python documentation guarantees that the error indicator is not set unless destructor == nullptr, this change is fine.

Looking: https://docs.python.org/3/c-api/capsule.html#c.PyCapsule_GetContext

Return the current context stored in the capsule. On failure, set an exception and return NULL.

It is legal for a capsule to have a NULL context. This makes a NULL return code somewhat ambiguous; use PyCapsule_IsValid() or PyErr_Occurred() to disambiguate.

To me it sounds like:

  • If the error indicator is set the return value is certainly nullptr.
  • But knowing that the return value is nullptr does not tell us anything about the state of the error indicator.

So checking the error indicator is mandatory, unless we want to risk leaving the process in an invalid state, which I don't think is a responsible thing to do, even it this was more than a minor optimization.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rwgk The documentation and the reference implementation seem to guarantee that the error indicator is not set unless it returns a nullptr.

A better way to state this is the following. Is it ever valid to set the ErrorIndicator and return a non-nullptr from this method? That is there any case where non-null and the error indicator is set (for the return to this function. Ignoring it could be set to an err due leakage from some other part of the code.

The answer is no, because if it returned any non-null ptr, then an error did not occur. The error only occurs if the capsule is PyCapsule_IsValid() returns false internally, it therefore unable to access the members of the pycapsule, and then return null. Therefore, this function only sets the error indicator IFF it returns a nullptr. The error guard guarantees that we the error indicator is not set.

Therefore, destructor being non-null strictly implies that no error has occurred.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm ... but what if the error indicator is set (for whatever reason)?

I mean that logic could be implied at any point in the code. At some point, you have to decide when ErrorIndicator is worth checking. This is a place where the flow is clearly defined and the ErrorIndicator cannot be set unless it's nullptr since we have an error guard currently in place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, sorry for getting this wrong. I didn't look at the implementation yesterday, from that it's totally clear.

throw error_already_set();
}
const char *name = get_name_in_error_scope(o);
Expand All @@ -1843,7 +1843,7 @@ class capsule : public object {
}
});

if (!m_ptr || PyCapsule_SetContext(m_ptr, (void *) destructor) != 0) {
if (!m_ptr || PyCapsule_SetContext(m_ptr, reinterpret_cast<void *>(destructor)) != 0) {
throw error_already_set();
}
}
Expand Down