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

Fix: implement trivial destructors for static PyObject unique pointers to avoid invalid memory access on exit #28

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

LinZhihao-723
Copy link
Member

Description

The static destructors of the global/static PyObject unique pointers (such as Py types and exceptions) will attempt to decrease the reference count when the process exits. However, its behavior is undefined due to the following reasons, which was first exposed in Python 3.10.12 on Linux with a segfault on exit:

  1. the Python interpreter can exit before the global/static destructors execute. Accessing the reference count is illegal if the interpreter is already shut down. There is no way to guarantee the ordering of the execution for the interpreter cleanup and the global/static destructors.
  2. C++ RAII guarantees that some code will execute when the scope exits, but it doesn't guarantee that all of the code finishes executing; the process may exit before the RAII destructor is complete.

This PR fixes the problem by making all the global/static Pyobject pointers trivially destructible. This means it will not attempt to release any resources when reset or destruction happens, which can lead to potential memory leaks. However, according to the current use case, this is acceptable since the only expected execution of the trivial destructor is on process exit.

We keep using smart ptr for two purposes:

  1. protect the underlying raw pointers from being exposed and accidentally modified.
  2. indicate the underlying object will never be freed by C++ static destructor.

src/clp_ffi_py/Py_utils.cpp Outdated Show resolved Hide resolved
src/clp_ffi_py/PyObjectUtils.hpp Outdated Show resolved Hide resolved
@LinZhihao-723 LinZhihao-723 merged commit 5e02fbd into y-scope:main Oct 31, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants