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

sigsegv on exit for static PyObject Py_XDECREF #126508

Closed
ovesh opened this issue Nov 6, 2024 · 5 comments
Closed

sigsegv on exit for static PyObject Py_XDECREF #126508

ovesh opened this issue Nov 6, 2024 · 5 comments
Labels
topic-C-API type-bug An unexpected behavior, bug, or error

Comments

@ovesh
Copy link

ovesh commented Nov 6, 2024

Bug report

Bug description:

Noticed that on py312 a C extension I maintain is segfaulting when python exits. Managed to create a minimal C extension that reproduces the issue:

#include <Python.h>
#include <memory>

struct PyObjectDecrementer {
	void operator()(PyObject* obj) const {
		Py_XDECREF(obj);
	};
};

static std::unique_ptr<PyObject, PyObjectDecrementer> stored_obj = nullptr;

static PyObject* set_stored_int(PyObject* self, PyObject* args) {
    const int* input_int;

    if (!PyArg_ParseTuple(args, "i", &input_int)) {
        printf("failed to parse input int\n");
        return NULL;
    }

    PyObject* new_int = PyLong_FromLong((long)input_int);
    if (!new_int) {
        printf("failed to create new int object\n");
        return NULL;
    }

    stored_obj.reset(new_int);

    Py_RETURN_NONE;
}

static PyMethodDef HelloMethods[] = {
    {"set_stored_int", set_stored_int, METH_VARARGS, "Store a Python int in a static variable."},
    {NULL, NULL, 0, NULL}
};

static struct PyModuleDef hellomodule = {
    PyModuleDef_HEAD_INIT, "hello", NULL, -1, HelloMethods
};

PyMODINIT_FUNC PyInit_hello(void) {
    return PyModule_Create(&hellomodule);
}

Minimal setup.py:

from setuptools import setup, Extension
hello_module = Extension('hello', sources=['library.cpp'])
setup(name='hello', version='1.0', description='xxxyyy', ext_modules=[hello_module])
% python
Python 3.12.7 | packaged by conda-forge | (main, Oct  4 2024, 15:55:29) [Clang 17.0.6 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import hello
>>> hello.set_stored_int(23)
>>> exit()
zsh: segmentation fault  python

Occurs at least on linux amd64 and MacOS. Narrowed it down with git bisect to this commit: df3173d

CPython versions tested on:

3.12

Operating systems tested on:

Linux, macOS

@ovesh ovesh added the type-bug An unexpected behavior, bug, or error label Nov 6, 2024
@s22chan
Copy link

s22chan commented Nov 6, 2024

(release) backtrace implies it wasn't able to get a non null OMState/interpreter (after skipping past the assert) at

assert(interp->obmalloc != NULL); // otherwise not initialized or freed
if I read it right.

Process 15740 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = instruction step into
    frame #0: 0x00007ff814b4e592 libdyld.dylib`tlv_get_addr + 13
libdyld.dylib`tlv_get_addr:
->  0x7ff814b4e592 <+13>: testq  %rax, %rax
    0x7ff814b4e595 <+16>: je     0x7ff814b4e59c ; <+23>
    0x7ff814b4e597 <+18>: addq   0x10(%rdi), %rax
    0x7ff814b4e59b <+22>: retq
Target 0: (python) stopped.
(lldb)
Process 15740 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = instruction step into
    frame #0: 0x00007ff814b4e595 libdyld.dylib`tlv_get_addr + 16
libdyld.dylib`tlv_get_addr:
->  0x7ff814b4e595 <+16>: je     0x7ff814b4e59c ; <+23>
    0x7ff814b4e597 <+18>: addq   0x10(%rdi), %rax

that ends up crashing it here:

* thread #1, queue = 'com.apple.main-thread', stop reason = instruction step into
    frame #0: 0x00000001000f6ffc python`_PyObject_Free + 28
python`_PyObject_Free:
->  0x1000f6ffc <+28>: movq   0x10(%rax), %rbx

@asvetlov
Copy link
Contributor

asvetlov commented Nov 6, 2024

If I understand correctly, segfault is raised on calling stored_obj destructor which is C++ class instance.

At this moment Python interpreter is already shut down and destroyed.

Py_XDECREF cannot get the current interpreter which leads to segmentation fault.
Destructors for static C++ variables are not good place to call Python code.

While segfault is not desired behavior, I'm curious what should we do here.
No-op on object destruction if the interpreter is gone doesn't sound like a good thing, it hides the actual error.

Py_XDECREF cannot signal the error; in C we have no exceptions.

I can propose a workaround that fixes the reproducer:

struct PyObjectDecrementer {
	void operator()(PyObject* obj) const {
	        if(PyInterpreterState_Get() != NULL)  {
		    Py_XDECREF(obj);
		}
	};
};

It can leave resources leaked, e.g. a file is not closed. Probably unclosed resource is not a problem, it will be released on the process exit. Anyway, decref could call __del__ which is dangerous is the interpreter is destroyed already.

Another (and maybe better) option is registering m_clear callback of PyModuleDef to explicitly clean-up all static PyObject* instances. The callback will be called on the module reloading, when the interpreter is still alive.

@s22chan
Copy link

s22chan commented Nov 6, 2024

Another (and maybe better) option is registering m_clear callback of PyModuleDef to explicitly clean-up all static PyObject* instances. The callback will be called on the module reloading, when the interpreter is still alive.

For the multiple interpreter case, will that necessitate some state management on the extension side?

@asvetlov
Copy link
Contributor

asvetlov commented Nov 6, 2024

Yes probably. For multi-interpreter mode there is a recommendation to avoid static variables but put the shared data into the module state. See PyModuleDef.m_size and PyModule_GetState(...). Say, Modules/_abc.c could provide a relatively small usage example.

@ZeroIntensity ZeroIntensity added topic-C-API pending The issue will be closed if no feedback is provided labels Nov 6, 2024
@ZeroIntensity
Copy link
Member

ZeroIntensity commented Nov 6, 2024

Yeah, I don't think there's anything that's a bug here. I highly suggest looking into multi-phase initialization and module state for calling things upon finalization.

I can propose a workaround that fixes the reproducer:

Hmm, I'm not sure that will always work either. Acquiring the interpreter state is reliant on the thread state, which is probably cleared by the time the destructor gets called. You probably want PyThreadState_Get() != NULL instead.

@ZeroIntensity ZeroIntensity closed this as not planned Won't fix, can't repro, duplicate, stale Nov 6, 2024
@ZeroIntensity ZeroIntensity removed the pending The issue will be closed if no feedback is provided label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-C-API type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants