fix: Narrow conditions for load_value to give invalid address

[amjames]

Release/ABI note

Although this change uses spare flag bits and leaves sizeof(detail::instance) unchanged, it changes the cross-DSO instance-state protocol and therefore bumps PYBIND11_INTERNALS_VERSION to 13. This PR must be held for pybind11 3.2.0. The intended sequence is to land #5800 in 3.1.1 first, then merge this PR for 3.2.0, so participating extensions can use #5800's general interoperability path for supported conversions across the v12/v13 boundary.

Description

Fixes #6153.

Calling cls.__new__(cls) for a pybind11-bound class creates the Python object and its value/holder slots without constructing the C++ value. Previously, loading that object from bound code could lazily allocate raw storage and treat it as a live C++ object. Reading a member could therefore return uninitialized data, and virtual dispatch could load an invalid vtable pointer and segfault. A malformed pickle can reach the same state; PyTorch has a downstream mitigation in pytorch/pytorch#194647.

The complication is that pybind11's deprecated old-style placement-new __init__ and __setstate__ callbacks legitimately need access to uninitialized storage. This change preserves those callbacks without making that storage available to arbitrary bound-code loads.

Design

Construction is now tracked for each exact value_and_holder slot rather than for the whole Python instance. During an old-style constructor candidate, raw storage is reserved privately by the current loader frame. A one-shot authorization lets only argument zero of that exact candidate load that exact slot; the pointer is not published in the instance until the native callback returns successfully.

All other loads while the slot is under construction raise ValueError, including:

• re-entry while converting a later constructor argument;
• new-style candidates in mixed old-/new-style overload chains;
• nested initialization of the same value;
• access through another base in Python multiple inheritance; and
• concurrent access on free-threaded Python.

Failed candidates clean up their private storage before overload resolution continues. Successful construction publishes and finalizes the value before return-value conversion and post-call policies run. Pure new-style constructors use the same construction-state guard, but never receive the old-style storage authorization.

The construction-state transitions and loads are protected by the instance critical section on free-threaded Python. The change uses spare bits in the existing simple-instance bitfield and nonsimple status byte, so sizeof(instance) and the internals ABI identifier are is unchanged. However, see the Release/ABI note above and this comment below.

Compatibility and tests

The regression coverage includes direct __new__ for bound classes and Python subclasses, ordinary pickle and manual __setstate__, failed old-style initialization and retry, later-argument re-entry, mixed old-/new-style overloads, nested initialization, Python multiple inheritance, and synchronized concurrent access. It also covers legacy callbacks whose self parameter is typed as either the bound C++ class or py::object.

Suggested changelog entry

Fix a potential crash when an uninitialized pybind11 instance created through direct __new__ is loaded by bound code. Preserve deprecated placement-new constructors and pickle __setstate__ callbacks while rejecting reentrant, nested, cross-base, and concurrent access until C++ construction completes.

AI assistance

The original changes were authored with assistance from Claude. The follow-up redesign and regression coverage were developed with Codex GPT-5.6-sol ultra and independently audited by a separate agent.

---

📚 Documentation preview 📚: https://pybind11--6157.org.readthedocs.build/

[henryiii]

I pushed two fixes from Fable. Both started with failing tests, then fixed.

🤖 AI text below 🤖

Review complete: 12 candidates checked, 2 survived (both confirmed), 10 refuted.

1. Incomplete fix — failed old-style __init__ reopens the segfault (type_caster_base.h:1173, empirically reproduced, exit 139). If an old-style placement-new __init__ throws (or a later argument fails to convert) after self was loaded, load_value has already lazily allocated vptr but placement-new never ran. Nothing deallocates it on the error path, so the next method call sees vptr != nullptr, skips the new ValueError, and hands out uninitialized memory. The new construction_in_progress bit is exactly the hook to close this: on constructor-dispatch failure, free a value that was lazily allocated while the holder was never constructed.

2. Guard armed too broadly (pybind11.h:1008). The scope arms for every constructor chain, but new-style py::init never needs lazy allocation (self is injected directly), so reentrant access to a half-built instance during modern construction still gets silent garbage instead of the new diagnostic. Cheap narrowing: arm only when the chain has an old-style constructor (is_constructor && !is_new_style_constructor). Not a regression, but a missed improvement.

Notable refutations: cross-module ABI safe (layout unchanged, bit zero-filled by tp_alloc); the bitfield RMW race matches the accepted pre-existing pattern (has_patients, owned, etc.); throwing instead of overload fallthrough replaces UB, not working behavior; all three cleanup nits stand as written.

[espressolee]

I independently reviewed exact head 66f8f3760f02cb596b28e552fc4f95cd79586b7a against base 5e9611aacc0bdd2054aa36800055014ebcd8e805 on macOS/arm64 with CPython 3.14.6 and 3.14.6t.

The submitted checks work for their covered paths: direct use of a __new__-only instance raises, failed old-style initialization is cleaned up, new-style constructor re-entry raises, pickle round trips survive, and test_class.py is 43/43 in regular Debug, regular NDEBUG, and free-threaded Debug builds.

There is still a blocking re-entry hole in the old-style placement-new path. instance_construction_scope marks the instance for the entire constructor dispatcher, and loading the old-style self argument lazily allocates vptr. If conversion of a later constructor argument executes Python and calls another bound method on the same object, that method sees the now-non-null raw pointer and bypasses the new null-value guard, even though placement-new has not run. A virtual call then reads the uninitialized vtable.

Minimal shape using the PR's own OldStyleInit fixture:

obj = m.OldStyleInit.__new__(m.OldStyleInit)

class Reenter:
    def __index__(self):
        obj.v_data()  # dispatches through unconstructed storage
        raise TypeError

obj.__init__(Reenter())

Exact-head results, fresh processes:

• CPython 3.14.6 Debug: 5/5 SIGSEGV
• CPython 3.14.6 NDEBUG: 5/5 SIGSEGV
• CPython 3.14.6t Debug with the GIL disabled: 5/5 SIGSEGV
• standalone exact-base build: 5/5 SIGSEGV

This is not a regression introduced by the PR, but it remains inside the same claimed invariant: old-style construction should be the only permitted lazy-allocation path without making arbitrary re-entrant native access safe. The current instance-wide flag cannot distinguish the constructor's own self load from a method call (or another thread) while construction is in progress.

Please scope the permission to the old-style constructor's own self conversion, and make every other load before successful construction raise. A regression where conversion of a later argument re-enters the same instance would cover the gap. I am not prescribing a particular implementation because preserving multi-overload old-style dispatch and free-threaded access requires the permission to be call/load-specific rather than merely an instance-wide time window.

Limits: I built the changed test_class module rather than the full local matrix; the exact-head remote matrix is green. The finding is deterministic in all three locally tested configurations above.

[rwgk]

I'll take a stab at this using codex gpt-5.6-sol ultra.

My starting point:

---

Verdict

PR 6157 should not merge at its current head, 66f8f37. The post-Henry review found a genuine blocking hole, and I independently reproduced it on your locally merged branch: the process terminates with SIGSEGV.

What the PR gets right

The original problem is real and security-relevant: direct __new__ creates the Python shell without constructing its C++ value, after which load_value() used to hand out raw, unconstructed storage. This affects PyTorch and malformed pickle handling; PyTorch has already merged a downstream mitigation. Issue #6153, PyTorch mitigation.

The current PR correctly handles several important paths:

• Ordinary access to a __new__-only object raises instead of reading garbage.
• Normal pickle round trips continue working.
• Henry’s cleanup correctly frees lazily allocated storage after a failed old-style constructor.
• Henry correctly prevents pure new-style constructors from enabling lazy allocation.
• I see no conventional ABI-size problem: the added bit still fits in the existing bitfield allocation.

All four submitted focused tests pass, and the current GitHub matrix has 77 successful checks and two expected skips.

Blocking finding

The permission is scoped to the entire instance and entire constructor dispatcher, rather than to the old-style constructor’s own self conversion.

The sequence is:

1. The dispatcher marks the instance as under construction in pybind11.h.
2. Converting the old-style self argument lazily allocates raw storage and stores a non-null pointer in the instance.
3. Converting a later argument can invoke Python—for example, through __index__.
4. That Python code re-enters a bound method on the same object.
5. Because the pointer is now non-null, load_value() skips the new guard in type_caster_base.h and dispatches through unconstructed storage.

A virtual call then segfaults. No thread race is required; ordinary same-thread Python re-entry is sufficient.

This is not a regression introduced by the PR, but it remains squarely inside the safety invariant the PR claims to establish. Green CI simply means this path is not tested.

The same design issue also affects:

• Mixed new-style/old-style overload chains: one old-style overload arms the flag while new-style candidates are tried.
• Python multiple inheritance: constructing base A enables lazy allocation for an uninitialized base B because the flag belongs to the whole Python instance.
• Nested initialization, which can placement-construct over an already-live object.
• Free-threaded concurrent access, where the bit and value pointer are ordinary non-atomic storage.

Recommendations

Before merging:

1. Add espressolee’s later-argument re-entry regression first.
2. Replace the instance-wide permission window with authorization tied to the exact old-style constructor candidate, exact value_and_holder, and exact self load.
3. Ensure every other load rejects reserved-but-unconstructed storage even though its pointer is non-null.
4. Preserve Henry’s failed-construction cleanup and retry behavior.
5. Add coverage for mixed old/new overloads and cross-base multiple inheritance. Nested initialization should preferably be rejected.
6. Correct the stale comment in test_class.py: it says construction_in_progress is set during a pure new-style constructor, but Henry’s commit deliberately leaves it unset.
7. Refresh the PR’s AI summary after redesigning the mechanism and add a suggested changelog entry.

The cleanest state model is conceptually:

uninitialized → reserved exclusively for old-style self → constructed

Ideally, raw storage would not be published through the instance pointer until the constructor callback succeeds. If it must be published, a separate per-value “reserved but unconstructed” state must be checked before every load.

[rwgk]

Conclusion and release sequencing

This fix needs PYBIND11_INTERNALS_VERSION 13. Although the new construction-state flags fit in spare bits and do not change sizeof(detail::instance), they change the protocol that separately compiled extension modules use to interpret a shared instance. Keeping the v12 identifier would allow old and new inline caster code to act on the same object using incompatible rules.

The proposed sequence is therefore:

1. Land [WIP] Interoperability with other Python binding frameworks #5800 in the 3.1.x line, ideally in 3.1.1.
2. Hold this PR for 3.2.0, where the internals version can advance from 12 to 13.

For participating extensions that enable #5800's interoperability mechanism, supported conversions can then cross the v12/v13 boundary through the general foreign-type path. The internals boundary continues to provide isolation, while cross-version interoperability has the documented limitations and modest extra cost of that path rather than relying on unsafe shared state.

Why retaining internals v12 would be unsafe

detail::instance is shared across extension modules that use the same internals identifier, but the code that reads it is compiled inline into each module. Existing v12 load_value() code has only two relevant interpretations of the value pointer:

• A null pointer permits the legacy path to allocate and publish raw storage.
• A non-null pointer is treated as a live C++ object.

It has no representation for "this exact value slot is currently being constructed; do not load or initialize it."

The new implementation keeps old-style placement-new storage private until the callback has successfully returned, leaving the instance's value pointer null in the meantime. An older v12 module can therefore bypass the new protocol, allocate and publish different raw storage, and hand it to bound C++ code before any object lifetime has begun. Substituting a non-null sentinel does not help: old code would treat the sentinel itself as a valid C++ pointer. Either route can reintroduce the undefined behavior this PR is intended to eliminate.

The bump is consequently required by the changed cross-DSO semantics, not merely by the physical size or offsets of detail::instance.

Alternatives considered

Private storage and null rejection without shared construction state

Keeping old-style storage private and rejecting ordinary null loads is sufficient for the original reproducer and many re-entrant cases when all participating code uses the new headers. It is not a complete replacement for the construction-state flag.

In particular, two threads can begin initializing the same value slot, each placement-construct a private object, and discover the collision only when committing. pybind11 has no generic type-erased operation that can correctly destroy the losing object before its holder has been constructed. More importantly, an already-compiled v12 module would still follow its legacy null-pointer allocation path. Narrowing the fix in this way would therefore weaken the guarantee without avoiding the cross-version incompatibility.

Reusing an existing flag or a pointer sentinel

The existing flags describe independent holder, registration, ownership, aliasing, and layout state. Overloading one of them would make normal initialization and cleanup depend on ambiguous flag combinations. A pointer sentinel is unsafe because every older caster, along with other paths that assume a non-null value pointer denotes a live object, can expose or dereference it.

A synchronized side table

A shared registry keyed by the exact value_and_holder slot could technically represent active construction without adding a field to detail::instance. To cover multiple extension modules, interpreters, Python multiple inheritance, nested calls, and free-threaded execution, it would need a carefully synchronized lifetime and locking protocol. A module-local table would not be sufficient.

This would be a substantial one-off shared-state mechanism to represent one per-slot bit, with additional allocation, hashing, locking, and teardown concerns. It also would not eliminate the internals bump: old v12 code would not consult the table and could still expose unconstructed storage.

Decision

Once the v12/v13 boundary is recognized as necessary, storing the construction state directly in the value slot is the smallest and clearest design. It is naturally shared by modules in the same internals domain, is exact for Python multiple-inheritance layouts, and avoids a separate lookup and lifetime-management subsystem.

The chosen approach is therefore to keep the explicit per-value construction state, bump to internals v13 for 3.2.0, and use #5800 as the general bridge for supported conversions between extensions that deliberately live in different internals domains.

[rwgk]

@oremanj for visibility

[rwgk]

@amjames @henryiii @espressolee Could you please make another pass over this PR with your agents?

[espressolee]

I made another pass over exact head 14e32ae23af529df8d82681c2d3064884b259a3c. I do not see a remaining blocker in this PR.

One process note: the auxiliary peer-agent calls did not return a usable review, so none of the conclusions below rely on peer output. I performed the exact-head code pass and the additional probes directly.

The v13 bump is justified by protocol compatibility, not sizeof(detail::instance). I added a small two-extension cross-DSO probe to test that distinction:

• fixed producer 4455e3f and a legacy-v12 consumer shared the registered C++ object successfully: 20/20 fresh regular processes and 20/20 fresh 3.14t processes;
• current-v13 producer and current-v13 consumer retained normal interoperability: 20/20 in both configurations;
• a current-v13 object passed to that legacy-v12 consumer was rejected with TypeError: 20/20 in both configurations. On 3.14t the GIL remained disabled throughout.

That is the behavior the internals split needs to provide: v12 inline caster code no longer interprets a v13 instance using the old null/non-null protocol, while modules in the new domain still interoperate normally.

I also reran the original later-argument re-entry control. The blocked head 66f8f37 segfaults in 5/5 fresh Debug processes; current head rejects the same path safely in 20/20 regular and 20/20 free-threaded processes. The focused test_class.py suite is 49/49 in regular Debug and 49/49 in 3.14t Debug. GitHub currently reports 76 passing checks and two expected skips.

So the proposed sequence looks sound to me: isolate this construction protocol in internals v13 for 3.2.0, and treat #5800 as the explicit interoperability path for supported cross-domain conversions. I did not independently validate #5800's bridge behavior in this pass, so my approval is scoped to #6157's construction-state fix and the v12/v13 isolation at this exact head.