Fix data race in registering instances

include/pybind11/pybind11.h

@@ -1714,6 +1714,13 @@ class class_ : public detail::generic_type {
         }
     }
 
+    static void register_instance_once(detail::instance *inst, detail::value_and_holder &v_h) {
+        if (!v_h.instance_registered()) {
+            v_h.set_instance_registered();
+            register_instance(inst, v_h.value_ptr(), v_h.type);
+        }
+    }
+
     /// Performs instance initialization including constructing a holder and registering the known
     /// instance.  Should be called as soon as the `type` value_ptr is set for an instance.  Takes
     /// an optional pointer to an existing holder to use; if not specified and the instance is
@@ -1732,11 +1739,19 @@ class class_ : public detail::generic_type {
         // by the critical section inside `get_type_info`.
         gil_scoped_acquire gil;
         auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(type)));
-        if (!v_h.instance_registered()) {
-            register_instance(inst, v_h.value_ptr(), v_h.type);
-            v_h.set_instance_registered();
+        // Registering publishes `inst` to other threads, so it should come last. Holders that
+        // may run user code still register first, since that code can look `inst` up on this
+        // thread (e.g. via PYBIND11_OVERRIDE). Other threads can then find such an instance before
+        // its holder is constructed. A custom unique_ptr deleter counts as user code.
+        constexpr bool register_first = !std::is_same<holder_type, std::unique_ptr<type>>::value
+                                        && !detail::is_shared_ptr<holder_type>::value;
+        if (register_first) {
+            register_instance_once(inst, v_h);
         }
         init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr<type>());
+        if (!register_first) {
+            register_instance_once(inst, v_h);
+        }
     }
 
     template <typename WrappedType>
@@ -1775,10 +1790,6 @@ class class_ : public detail::generic_type {
         gil_scoped_acquire gil;
 
         auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(type)));
-        if (!v_h.instance_registered()) {
-            register_instance(inst, v_h.value_ptr(), v_h.type);
-            v_h.set_instance_registered();
-        }
         auto *uninitialized_location = std::addressof(v_h.holder<holder_type>());
         auto *value_ptr_w_t = v_h.value_ptr<type>();
         // Try downcast from `type` to `type_alias`:
@@ -1799,6 +1810,8 @@ class class_ : public detail::generic_type {
             }
         }
         v_h.set_holder_constructed();
+        // Nothing above runs user code, so registering last is always safe here.
+        register_instance_once(inst, v_h);
     }
 
     // Deallocates an instance; via holder, if constructed; otherwise via operator delete.

tests/test_smart_ptr.cpp

@@ -352,6 +352,45 @@ struct ElementList {
     std::vector<std::shared_ptr<ElementBase>> l;
 };
 
+// test_holder_ctor_calls_python_override
+class RetainNotifier {
+public:
+    virtual ~RetainNotifier() = default;
+    virtual void on_retain() {}
+    void add_ref() {
+        ++ref_count_;
+        on_retain();
+    }
+    void release() {
+        if (--ref_count_ == 0) {
+            delete this;
+        }
+    }
+
+private:
+    int ref_count_ = 0;
+};
+
+class PyRetainNotifier : public RetainNotifier {
+public:
+    void on_retain() override { PYBIND11_OVERRIDE(void, RetainNotifier, on_retain, ); }
+};
+
+// Holder whose constructor calls back into the object, so constructing it runs user code
+// (including Python overrides of on_retain()).
+template <typename T>
+class notifying_intrusive_ptr {
+    T *ptr_;
+
+public:
+    explicit notifying_intrusive_ptr(T *p) : ptr_(p) { ptr_->add_ref(); }
+    notifying_intrusive_ptr(const notifying_intrusive_ptr &other)
+        : notifying_intrusive_ptr(other.ptr_) {}
+    notifying_intrusive_ptr &operator=(const notifying_intrusive_ptr &) = delete;
+    ~notifying_intrusive_ptr() { ptr_->release(); }
+    T *get() const { return ptr_; }
+};
+
 } // namespace
 
 // ref<T> is a wrapper for 'Object' which uses intrusive reference counting
@@ -377,6 +416,7 @@ PYBIND11_DECLARE_HOLDER_TYPE(T, huge_unique_ptr<T>, )
 PYBIND11_DECLARE_HOLDER_TYPE(T, custom_unique_ptr<T>, )
 PYBIND11_DECLARE_HOLDER_TYPE(T, shared_ptr_with_addressof_operator<T>, )
 PYBIND11_DECLARE_HOLDER_TYPE(T, unique_ptr_with_addressof_operator<T>, )
+PYBIND11_DECLARE_HOLDER_TYPE(T, notifying_intrusive_ptr<T>, true)
 
 namespace holder_caster_traits_test {
 struct example_base {};
@@ -658,5 +698,10 @@ TEST_SUBMODULE(smart_ptr, m) {
         .def_readwrite("ptr",
                        &ContainerUsingPrivateESFT::ptr); // <- access ESFT through shared_ptr
 
+    // test_holder_ctor_calls_python_override
+    py::class_<RetainNotifier, PyRetainNotifier, notifying_intrusive_ptr<RetainNotifier>>(
+        m, "RetainNotifier")
+        .def(py::init<>());
+
     holder_caster_traits_test::wrap(m);
 }

tests/test_smart_ptr.py

@@ -392,3 +392,14 @@ def test_shared_ptr_cast_for_custom_holder_throws(cast_shared_ptr):
         ),
     ):
         cast_shared_ptr()
+
+
+def test_holder_ctor_calls_python_override():
+    calls = []
+
+    class PyNotifier(m.RetainNotifier):
+        def on_retain(self):
+            calls.append("retain")
+
+    PyNotifier()
+    assert calls == ["retain"]

tests/test_thread.cpp

@@ -15,6 +15,7 @@
 
 #include <chrono>
 #include <condition_variable>
+#include <memory>
 #include <mutex>
 #include <stdexcept>
 #include <string>
@@ -40,6 +41,18 @@ struct IntStruct {
 struct EmptyStruct {};
 EmptyStruct SharedInstance;
 
+// Templated so each holder kind gets its own C++ type, since a type can only be bound once.
+template <int>
+struct SharedObject {
+    int value() const { return 42; }
+};
+
+template <typename T>
+std::shared_ptr<T> get_shared() {
+    static const auto instance = std::make_shared<T>();
+    return instance;
+}
+
 #ifdef Py_GIL_DISABLED
 
 // Holds detail::internals::mutex on a native thread that never touches the Python C API, so that
@@ -213,6 +226,14 @@ TEST_SUBMODULE(thread, m) {
     py::class_<EmptyStruct>(m, "EmptyStruct")
         .def_readonly_static("SharedInstance", &SharedInstance);
 
+    using SharedObjectClassic = SharedObject<0>;
+    using SharedObjectSh = SharedObject<1>;
+    py::class_<SharedObjectClassic, std::shared_ptr<SharedObjectClassic>>(m, "SharedObjectClassic")
+        .def("value", &SharedObjectClassic::value);
+    m.def("get_shared_object_classic", &get_shared<SharedObjectClassic>);
+    py::classh<SharedObjectSh>(m, "SharedObjectSh").def("value", &SharedObjectSh::value);
+    m.def("get_shared_object_sh", &get_shared<SharedObjectSh>);
+
 #if defined(PYBIND11_HAS_STD_BARRIER)
     // In the free-threaded build, during PyThreadState_Clear, removing the thread from the biased
     // reference counting table may call destructors. Make sure that it doesn't crash.

tests/test_thread.py

@@ -73,6 +73,28 @@ def access_shared_instance():
         thread.join()
 
 
+@pytest.mark.skipif(sys.platform.startswith("emscripten"), reason="Requires threads")
+@pytest.mark.parametrize(
+    "get_shared", [m.get_shared_object_classic, m.get_shared_object_sh]
+)
+def test_concurrent_cast_of_shared_cpp_object(get_shared):
+    # Only fails under ThreadSanitizer on free-threaded builds; otherwise the race is silent.
+    nb_threads = 4
+    b = threading.Barrier(nb_threads)
+
+    def cast_shared_object():
+        b.wait()
+        for _ in range(1000):
+            # get_shared() alone never reads the wrapper's flags. Calling value() does.
+            get_shared().value()
+
+    threads = [threading.Thread(target=cast_shared_object) for _ in range(nb_threads)]
+    for thread in threads:
+        thread.start()
+    for thread in threads:
+        thread.join()
+
+
 @pytest.mark.skipif(sys.platform.startswith("emscripten"), reason="Requires threads")
 @pytest.mark.skipif(not m.defined_PYBIND11_HAS_STD_BARRIER, reason="no <barrier>")
 @pytest.mark.skipif(env.sys_is_gil_enabled(), reason="Deadlock with the GIL")