Change implementation of the `__init__() must be called when overriding __init__` safety feature to work for any metaclass. by rwgk · Pull Request #30095 · google/pybind11clif

rwgk · 2024-01-26T21:29:14Z

Description

The safety feature introduced with pybind/pybind11#2152 (__init__() must be called when overriding __init__ ) only works for the default py::metaclass(). This PR changes the implementation of the safety feature so that it works for any metaclass (except when using PyPy; see below). (This has already uncovered missing super.__init__() calls in jax.)

The main driving force for this PR is that PyCLIF-pybind11 needs to use py::metaclass((PyObject *) &PyType_Type). Without this PR, the equivalent safety feature in PyCLIF would be lost. Note that it was a significant effort to clean up the Google codebase before the PyCLIF safety feature could be enabled, with this PyCLIF commit; i.e. it would be a very concerning loss.

This PR is based on a mechanism originally introduced with that PyCLIF commit. However, the original PyCLIF mechanism lacks the weakref-based cleanup of the internal registry backing the mechanism (derived_tp_init_registry). While this has not been a problem in practice, it is easy enough to add the cleanup feature here. The corresponding code is very similar to existing code here, which is a critical part of the core pybind11 functionality; in other words, it is a heavily tested and time-tested approach.

Details, based on the original PyCLIF commit message linked above:

Situation:

CppBase is a py::class_-wrapped C++ object.

What happens when the Python interpreter processes the following code (usually at import time)?

class PC(CppBase):
  pass

When the native Python PC class is built:

PC tp_new is set to use CppBase tp_new, but
PC tp_init does NOT in any way involve CppBase tp_init.

It is the responsibility of PC.__init__ to call CppBase.__init__, but this is not checked.

The approach used in this PR is:

PC tp_init is replaced with an "intercept" function.
The intercept function calls the original PC tp_init.
After that call finishes (and if it was successful), the intercept function checks if the CppBase wrapped C++ object was initialized.

Note that the derived_tp_init_registry()->count(type) == 0 condition in pybind11_object_new() enables daisy-chaining similar intercept functions, e.g. in other pybind11 extensions built with hidden visibility, or potentially other binding systems.

The intercept mechanism turns out to not be compatible with PyPy, therefore the safety feature cannot be enabled for metaclasses other than the default py::metaclass().

Suggested changelog entry:

…t.py

* Call from new `tp_init_intercepted()` (adopting mechanism first added in PyCLIF: google/clif@7cba87d). * Remove `pybind11_meta_call()` (which was added with pybind/pybind11#2152).

…nit != tp_init_intercepted`

…in). ``` ==6380==WARNING: MemorySanitizer: use-of-uninitialized-value #0 0x5611589c9a58 in Py_DECREF third_party/python_runtime/v3_11/Include/object.h:537:9 ... Uninitialized value was created by a heap deallocation #0 0x5611552757b0 in free third_party/llvm/llvm-project/compiler-rt/lib/msan/msan_interceptors.cpp:218:3 google#1 0x56115898e06b in _PyMem_RawFree third_party/python_runtime/v3_11/Objects/obmalloc.c:154:5 google#2 0x56115898f6ad in PyObject_Free third_party/python_runtime/v3_11/Objects/obmalloc.c:769:5 google#3 0x561158271bcc in PyObject_GC_Del third_party/python_runtime/v3_11/Modules/gcmodule.c:2407:5 google#4 0x7f21224b070c in pybind11_object_dealloc third_party/pybind11/include/pybind11/detail/class.h:483:5 google#5 0x5611589c2ed0 in subtype_dealloc third_party/python_runtime/v3_11/Objects/typeobject.c:1463:5 ... ```

…ith PyPy and Python `type` as metaclass")

…n/sharding.cc This change is to unblock google/pybind11clif#30095. Leaving wrapped C++ types uninitialized creates a potential for triggering undefined behavior from Python. PiperOrigin-RevId: 602787434

…n/sharding.cc This change is to unblock google/pybind11clif#30095. Leaving wrapped C++ types uninitialized creates a potential for triggering undefined behavior from Python. PiperOrigin-RevId: 602884828

…pe' is not used` diagnostics (many CI jobs; seems to be a clang issue).

rainwoodman · 2024-01-31T19:29:33Z

    type->tp_base = type_incref(&PyType_Type);
    type->tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HEAPTYPE;

+#if defined(PYPY_VERSION)


The new mechanism does not need to override tp_call?

I think what is missing in the context is that pybind11_meta_call only conducts protection for base class init, and can thus be skipped on CPYTHON.

Maybe defining a few secondary compilation conditions,

#if defined(PYPY_VERSION) #define PYBIND11_ENABLE_TP_META_CALL_INIT_PROTECTION #else #define PYBIND11_ENABLE_SAFE_TP_INIT_INIT_PROTECTION #endif

and wrap the two protection mechanisms in their corresponding conditions??

No: Basically, the starting point for the original change in PyCLIF was exactly to find a trick that doesn't involve manipulating the metaclass.

I also thought a little bit about "intercepting" PyType_Type.tp_call, but that would be very intrusive and have a runtime impact for pretty much all calls creating a Python object.

Maybe defining a few secondary compilation conditions,

Done, thanks for the suggestion! I chose different names for the macros and made it so that the choice of implementation can be overridden externally.

rainwoodman · 2024-01-31T19:31:01Z

 }

+using derived_tp_init_registry_type
+    = std::unordered_map<PyTypeObject *, int (*)(PyObject *, PyObject *, PyObject *)>;


what about initproc as the value type?

Much nicer, thanks! (I totally didn't think of that)

rainwoodman · 2024-01-31T19:39:02Z

 from pybind11_tests import python_multiple_inheritance as m

+#
+# Using default py::metaclass():


# CppBase0 Uses default py::metaclass()

rainwoodman · 2024-01-31T19:39:30Z

+
+
+#
+# Using py::metaclass((PyObject *) &PyType_Type):


CppBase1 uses ....

rainwoodman · 2024-01-31T19:45:52Z

+// This mechanism was originally developed here:
+// https://github.com/google/clif/commit/7cba87dd8385ab707c98e814ce742eeca877eb9e
+extern "C" inline int tp_init_intercepted(PyObject *self, PyObject *args, PyObject *kw) {
+    assert(PyType_Check(self) == 0);


How is this condition guaranteed? Looks like we don't ensure it when intercepting tp_init, based on my initial read of the pybind11object_new method.

PS: if it was not for consisitency with pyclif naming, I would recommend calling this safe_tp_init.

How is this condition guaranteed?

It's not. I put the assert() here only to be sure my code isn't doing something unexpected. This PR is already TGP tested, which makes me believe it is very unlikely that the assert() will ever fire, but if it does, I'll have a concrete situation to fix (as opposed to anticipating/guessing).

PS: if it was not for consisitency with pyclif naming,

Name changed to tp_init_with_safety_checks.

(After this PR is merged I'll go back and change the PyCLIF code accordingly. I also want to backport the weakref-based cleanup.)

…@rainwoodman

…@rainwoodman

rainwoodman · 2024-02-01T18:19:22Z

+/// Instance creation function for all pybind11 types. It only allocates space for the
+/// C++ object, but doesn't call the constructor -- an `__init__` function must do that.
+extern "C" inline PyObject *pybind11_object_new(PyTypeObject *type, PyObject *, PyObject *) {
+#if defined(PYBIND11_INIT_SAFETY_CHECKS_VIA_INTERCEPTING_TP_INIT)


maybe the ifdef for pybind11_meta_call should also be in the body like here?

Decided on chat: leaving as is.

…ges. In order of importance: * Bug fix (moderately important): Missing `Py_DECREF(type_self)` in `tp_dealloc_impl`. * This bug was triggered with the upgrade to Python to 3.9. It was discovered coincidentally when adding the new `testDerivedTpInitRegistryWeakrefBasedCleanup` (see below). When running the test in a `while True` loop, the Resident Memory Size (`RES` in `top`) exceeded 1 GB after just a few seconds. * Root cause: python/cpython#79991 * Critical clue leading to the fix: https://github.com/pybind/pybind11/blob/768cebe17e65c2a0a64ed067510729efc3c7ff6c/include/pybind11/detail/class.h#L467-L469 * Bug fix (very minor): Incorrect `Py_DECREF(self)` in `tp_init_with_safety_checks`. * This bug was introduced with cl/559501787. The `Py_DECREF(self)` was accidentally adopted along with the corresponding pybind11 error message (see link in description of cl/559501787). It was discovered coincidentally by MSAN (heap use after free) while testing [google/pybind11clif#30095](google/pybind11clif#30095). * After inspecting https://github.com/python/cpython/blob/b833b00482234cb0f949e6c277ee1bce1a2cbb85/Objects/typeobject.c#L1103-L1106 it became clear that the `Py_DECREF(self)` is indeed incorrect in this situation (it is correct in the original pybind11 sources). * The weakref-based cleanup added in [google/pybind11clif#30095](google/pybind11clif#30095) is ported back to PyCLIF. — This was the original purpose of this CL. * `tp_init_intercepted` is renamed to `tp_init_with_safety_checks` for for compatibility with google/pybind11clif#30095. GitHub testing: #90 PiperOrigin-RevId: 604337502

rwgk added 4 commits January 26, 2024 12:06

Also wrap with py::metaclass((PyObject *) &PyType_Type)

a911a18

Transfer additional tests from PyCLIF python_multiple_inheritance_tes…

9c3799c

…t.py

Expand tests to fully cover wrapping with alternative metaclasses.

b9aeb34

* Factor out ensure_base_init_functions_were_called().

7b66ebe

* Call from new `tp_init_intercepted()` (adopting mechanism first added in PyCLIF: google/clif@7cba87d). * Remove `pybind11_meta_call()` (which was added with pybind/pybind11#2152).

rwgk force-pushed the tp_init_intercepted_pywrapcc branch from 7084009 to 7b66ebe Compare January 28, 2024 19:10

rwgk added 7 commits January 28, 2024 11:40

Bug fix (maybe actually two bugs?): simplify condition to `type->tp_i…

a87e484

…nit != tp_init_intercepted`

IncludeCleaner fixes (Google toolchain).

1f155c3

Restore type->tp_call = pybind11_meta_call; for PyPy only.

d1ab242

pytest.skip("ensure_base_init_functions_were_called() does not work w…

38ac8d9

…ith PyPy and Python `type` as metaclass")

Merge branch 'google_pywrapcc_main' into tp_init_intercepted_pywrapcc

bf9d647

Do not intercept our own tp_init function (pybind11_object_init).

9cc3de1

copybara-service Bot mentioned this pull request Jan 30, 2024

Add missing super().__init__() involving types wrapped in xla/python/sharding.cc jax-ml/jax#19582

Merged

copybara-service Bot mentioned this pull request Jan 30, 2024

Add missing super().__init__() involving types wrapped in xla/python/sharding.cc openxla/xla#8978

Merged

rwgk added 3 commits January 30, 2024 17:46

Add derived_tp_init_registry weakref-based cleanup.

a9922b4

Replace assert() with if to resolve erroneous `lambda capture 'ty…

5c558e6

…pe' is not used` diagnostics (many CI jobs; seems to be a clang issue).

Add derived_tp_init_registry()->count(type) == 0 condition.

2f917e5

rwgk changed the title ~~WIP: tp_init_intercepted_pywrapcc~~ Change the implementation of the safety feature introduced with pybind/pybind11#2152 to work for any metaclass. Jan 31, 2024

rwgk marked this pull request as ready for review January 31, 2024 18:23

rwgk changed the title ~~Change the implementation of the safety feature introduced with pybind/pybind11#2152 to work for any metaclass.~~ Change implementation of the __init__() must be called when overriding __init__ safety feature to work for any metaclass. Jan 31, 2024

rainwoodman reviewed Jan 31, 2024

View reviewed changes

rwgk added 2 commits January 31, 2024 14:32

Changes based on feedback from @rainwoodman

766b97f

Use PYBIND11_INIT_SAFETY_CHECKS_VIA_* macros, based on suggestion from …

fd47cc6

…@rainwoodman

rainwoodman approved these changes Feb 1, 2024

View reviewed changes

rwgk merged commit 54f8341 into google:main Feb 1, 2024

rwgk deleted the tp_init_intercepted_pywrapcc branch February 1, 2024 19:02

rwgk added the could be backported Could be backported to pybind11 master label Feb 1, 2024

Conversation

rwgk commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Suggested changelog entry:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rwgk commented Jan 26, 2024 •

edited

Loading