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: valgrind-detected after-freeing access of PyMethodDef (macOS Python 3.9.0 segfaults) #2576

Merged
merged 4 commits into from
Oct 14, 2020

Conversation

YannickJadoul
Copy link
Collaborator

@YannickJadoul YannickJadoul commented Oct 11, 2020

Python 3.9 added this line to meth_dealloc, unfortunately for us, 2 lines too low.

Short summary of what goes wrong:

  • pybind11 stores the function_record in a capsule, and passes this as the self argument to PyCFunction_NewEx
  • The PyCFunctionObject object nicely keeps track of that, but decrefs m->m_self 2 lines before decrefing PyCFunction_GET_CLASS(m). The capsule with the function_record with the PyModuleDef gets freed.
  • PyCFunction_GET_CLASS accesses m_ml -> ml_flags, where m_ml now points to something that was destructed.
  • BOOM

I guess this being so close explains why we don't often get a crash here. Maybe macOS protects its memory better?

  • 61e40f6 demonstrates that leaking the PyModuleDef resolves the Python 3.9 crash on macOS.
  • d10f154 would be a (the?) solution, if it weren't calling cpp_function during the construction of a cpp_function during the construction of a cpp_function during the construction of a cpp_function during ... and thus blowing up the stack.

Closes #2558

Related issue on bugs.python.org: https://bugs.python.org/issue41237
PR on CPython adding the line to meth_dealloc that makes things crash: python/cpython#19936

@henryiii henryiii changed the title Check if valgrind-detected after-freeing access of PyMethodDef causes macOS Python 3.9 segfaults fix: valgrind-detected after-freeing access of PyMethodDef (macOS Python 3.9.0 segfaults) Oct 13, 2020
Copy link
Collaborator Author

@YannickJadoul YannickJadoul left a comment

Choose a reason for hiding this comment

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

Replacing delete rec->def; with

                static int counter = 0;
                std::cout << ++counter << std::endl;

and running pybind11's tests, gives me a final count of 1808, of which 49 before cleaning up (so things that would be cleaned up during, by pybind11, that are the true leaks, here):

======================================== test session starts ========================================
platform linux -- Python 3.8.6, pytest-5.3.2, py-1.8.0, pluggy-0.13.1
rootdir: /home/yannick/pybind11/tests, inifile: pytest.ini
plugins: lazy-fixture-0.6.2
collected 430 items                                                                                 

../../tests/test_async.py ..                                                                  [  0%]
../../tests/test_buffers.py .........                                                         [  2%]
../../tests/test_builtin_casters.py ....s...........                                          [  6%]
../../tests/test_call_policies.py ....1
.2
...                                                    [  8%]
../../tests/test_callbacks.py 3
4
...5
.6
.....                                                       [ 10%]
../../tests/test_chrono.py ...........................................                        [ 20%]
../../tests/test_class.py .............7
8
......9
10
.........11
12
13
.                                       [ 26%]
../../tests/test_constants_and_functions.py ....                                              [ 27%]
../../tests/test_copy_move.py ..14
15
16
17
..s..                                                         [ 29%]
../../tests/test_custom_type_casters.py ..                                                    [ 30%]
../../tests/test_docstring_options.py .                                                       [ 30%]
../../tests/test_eigen.py .........................                                           [ 36%]
../../tests/test_enum.py ......                                                               [ 37%]
../../tests/test_eval.py 18
19
.20
.                                                                   [ 37%]
../../tests/test_exceptions.py .........                                                      [ 40%]
../../tests/test_factory_constructors.py ....21
.22
..23
........                                      [ 43%]
../../tests/test_gil_scoped.py 24
25
26
27
.....                                                          [ 44%]
../../tests/test_iostream.py ............                                                     [ 47%]
../../tests/test_kwargs_and_defaults.py ........                                              [ 49%]
../../tests/test_local_bindings.py ..........                                                 [ 51%]
../../tests/test_methods_and_attributes.py .....28
........29
.......                               [ 56%]
../../tests/test_modules.py .....                                                             [ 57%]
../../tests/test_multiple_inheritance.py ........30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
...                                          [ 60%]
../../tests/test_numpy_array.py ................................................              [ 71%]
../../tests/test_numpy_dtypes.py ...............                                              [ 74%]
../../tests/test_numpy_vectorize.py ........                                                  [ 76%]
../../tests/test_opaque_types.py ...                                                          [ 77%]
../../tests/test_operator_overloading.py ....                                                 [ 78%]
../../tests/test_pickling.py .....                                                            [ 79%]
../../tests/test_pytypes.py ...............................                                   [ 86%]
../../tests/test_sequences_and_iterators.py ........                                          [ 88%]
../../tests/test_smart_ptr.py .............                                                   [ 91%]
../../tests/test_stl.py ........ss........                                                    [ 95%]
../../tests/test_stl_binders.py .........                                                     [ 97%]
../../tests/test_tagbased_polymorphic.py .                                                    [ 97%]
../../tests/test_union.py .                                                                   [ 98%]
../../tests/test_virtual_functions.py .46
47
.48
.49
.....                                                [100%]

====================================== short test summary info ======================================
SKIPPED [1] test_builtin_casters.py:135: no <string_view>
SKIPPED [1] test_copy_move.py:69: no <optional>
SKIPPED [1] test_stl.py:109: no <optional>
SKIPPED [1] test_stl.py:137: no <experimental/optional>
================================== 426 passed, 4 skipped in 5.81s ===================================

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci related to the CI system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Python 3.9 macOS segfault
4 participants