pybind / pybind11

Seamless operability between C++11 and Python
https://pybind11.readthedocs.io/
Other
15.08k stars 2.05k forks source link

fix: refcount bug involving trampoline functions with `PyObject *` return type. #5156

Closed rwgk closed 2 weeks ago

rwgk commented 3 weeks ago

Description

The heap-use-after-free bug was discovered through PyCLIF-pybind11 testing with ASAN (see https://github.com/pybind/pybind11/pull/5156#issuecomment-2156823687). The fix has two simple parts:

  1. Introduce a cast_safe() specialization for PyObject *.
  2. Preempt use of cast_ref in PYBIND11_OVERRIDE_IMPL (so that cast_safe() is used instead).

These changes ensure that

                return hfunc.f(std::forward<Args>(args)...).template cast<Return>();

in pybind11/functional.h does not incorrectly drop a Python reference count.

See also: PR #4601, which introduced type_caster<PyObject> but missed the corner case fixed by this PR.

For easy reference: Google-internal global testing ID: OCL:641800583:BASE:641923381:1718036131555:271433f2

Suggested changelog entry:

A refcount bug (leading to heap-use-after-free) involving trampoline functions with ``PyObject *`` return type was fixed.
rwgk commented 2 weeks ago

For completeness, the original failure was (using the Google-internal toolchain):

blaze test --config=asan //third_party/clif/testing/python:virtual_funcs_test --//devtools/clif/python/codegen_mode=pybind11
...
==8010==ERROR: AddressSanitizer: heap-use-after-free on address 0x502000465750 at pc 0x560a2e7e0e08 bp 0x7ffff62b38b0 sp 0x7ffff62b38a8
READ of size 8 at 0x502000465750 thread T0
    #0 0x560a2e7e0e07 in Py_DECREF third_party/python_runtime/v3_11/Include/object.h:537:9
    #1 0x560a2e7e0e07 in Py_XDECREF third_party/python_runtime/v3_11/Include/object.h:602:9
    #2 0x560a2e7e0e07 in pybind11::handle::dec_ref() const & third_party/pybind11/include/pybind11/detail/../detail/../pytypes.h:283:9
    #3 0x560a2e818f3a in ~object third_party/pybind11/include/pybind11/detail/../detail/../pytypes.h:379:17
    #4 0x560a2e818f3a in pybind11::detail::type_caster<_object, void>::~type_caster() third_party/pybind11/include/pybind11/type_caster_pyobject_ptr.h:18:7
    #5 0x7fc5b7d0edf5 in __run_exit_handlers (/usr/grte/v5/lib64/libc.so.6+0x78df5) (BuildId: 9a996398ce14a94560b0c642eb4f6e94)
    #6 0x7fc5b7d0eef9 in exit (/usr/grte/v5/lib64/libc.so.6+0x78ef9) (BuildId: 9a996398ce14a94560b0c642eb4f6e94)
    #7 0x560a2f83591d in Py_Exit third_party/python_runtime/v3_11/Python/pylifecycle.c:2966:5
...
rwgk commented 2 weeks ago

To ensure that the added test reproduces the original failure:

Failure of the test added in this PR (without the production code changes; using the Google-internal toolchain):

blaze test --config=asan third_party/pybind11/tests:test_type_caster_pyobject_ptr
...
==2670==ERROR: AddressSanitizer: heap-use-after-free on address 0x50600260e4d0 at pc 0x7f38b2cc9bb8 bp 0x7ffd5eb55410 sp 0x7ffd5eb55408
READ of size 8 at 0x50600260e4d0 thread T0
    #0 0x7f38b2cc9bb7 in Py_DECREF third_party/python_runtime/v3_11/Include/object.h:537:9
    #1 0x7f38b2cc9bb7 in Py_XDECREF third_party/python_runtime/v3_11/Include/object.h:602:9
    #2 0x7f38b2cc9bb7 in pybind11::handle::dec_ref() const & third_party/pybind11/include/pybind11/detail/../detail/../pytypes.h:283:9
    #3 0x7f38b386b1aa in ~object third_party/pybind11/include/pybind11/detail/../detail/../pytypes.h:379:17
    #4 0x7f38b386b1aa in pybind11::detail::type_caster<_object, void>::~type_caster() third_party/pybind11/include/pybind11/type_caster_pyobject_ptr.h:18:7
...
rwgk commented 2 weeks ago

Ignoring the 3 failing Python 3.13 builds: The same failures appear under https://github.com/pybind/pybind11/pull/3939.

rwgk commented 2 weeks ago

Thanks @henryiii!