Skip to content

Fix parameter pointer aliasing in class member registration - #54

Open
cobriensr wants to merge 1 commit into
godotengine:masterfrom
cobriensr:fix-param-registration-aliasing
Open

Fix parameter pointer aliasing in class member registration#54
cobriensr wants to merge 1 commit into
godotengine:masterfrom
cobriensr:fix-param-registration-aliasing

Conversation

@cobriensr

@cobriensr cobriensr commented Jul 18, 2026

Copy link
Copy Markdown

Problem

When a [BindMethod], virtual method override, or [Signal] is registered, its
managed parameter info is converted to the unmanaged GDExtensionPropertyInfo,
whose name, class_name, and hint_string are pointers. The engine reads
those pointers later only when classdb_register_extension_class_method /
…_signal / …_virtual_method is finally called at the end of the conversion.

The converted values were stored in loop- and block-scoped locals, and
their addresses (&local) were written into the property info:

for (int i = 0; i < parameters.Count; i++)
{
    NativeGodotStringName parameterNameNative = parameter.Name.NativeValue.DangerousSelfRef;
    NativeGodotString hintStringNative = NativeGodotString.Create(parameter.HintString);
    args[i] = new GDExtensionPropertyInfo { name = &parameterNameNative, hint_string = &hintStringNative, ... };
}   // parameterNameNative / hintStringNative slots are reused next iteration

Every iteration reuses the same stack slots, so after the loop all parameters'
args[i].name (and class_name, hint_string, default value) alias the last
parameter's data.

The return value info had the same defect one level down: its
struct was built inside a nested block, so its pointer fields dangled once that
block closed.

A method/signal with more than one parameter registered with
corrupted per-parameter metadata (all parameters reporting the last one's name,
type hint, and default).

This affected all three registration paths:

  1. BindMethod
  2. BindSignal
  3. BindVirtualMethod (identical code)

Plus a companion leak: the NativeGodotString hint strings created during conversion were never disposed.

Fix

Convert parameters into parallel buffers with one slot per parameter, so each
GDExtensionPropertyInfo points at its own stable slot that stays alive until
registration:

  • BindMethod / BindVirtualMethod share a new ConvertParameterInfosToNative
    helper (per-index stackalloc slots for names, class names, hint strings, and
    default values).
  • BindSignal gets ConvertSignalParameterInfosToNative. Its native slot types
    are ref structs, so they can't use a managed array; it stackallocs up to the
    existing span threshold and falls back to NativeMemory.Alloc/Free (in a
    finally) for larger signals.
  • The return value info and the method name are hoisted into the registration
    frame for the same lifetime reason.
  • The hint strings are disposed once, after registration (the engine has copied
    the data by then).

Virtual-method registration doesn't wire up default arguments, so that behavior
is unchanged as it reuses the shared helper and discards the optional-parameter
count.

Verification

  • New unit tests (ClassRegistrationContextConversionTests) exercise the
    extracted helpers with engine-free definitions and assert: (a) each parameter's
    pointers reference its own slot and are pairwise distinct; (b) default
    values dereference to the value of the parameter they belong to which are the exact
    things the aliasing bug corrupted; (c) the default-value array is compacted
    onto the trailing optional parameters when they follow a required one (a naive
    per-parameter-index write would fail this); and (d) the conversion holds up
    above the signal span threshold. Confirmed red/green on both the aliased-slot
    pattern and the naive-compaction pattern.
  • Godot.Bindings.Tests: 205/205 pass (200 baseline + 5).
  • dotnet build src/Godot.Bindings (trim/AOT analyzers on): no new IL or analyzer
    warnings as the only ones present are pre-existing and untouched (IL2075 in
    GodotObject.NativeName.cs, IL2090 in Marshalling.cs).

Scope / known limitations

  • Only empty StringName/String values can be constructed without a running
    Godot host (creating a non-empty one calls into the engine), so the unit tests
    assert on pointer identity/distinctness and on default values built from the
    interop-free bool/int/float Variant types. There is full end-to-end confirmation now
    that the engine reads back the correct per-parameter names, hints, and
    defaults for a registered class. This needs the Godot editor and the integration
    harness, which isn't exercised in CI here.
  • BindProperty has a related but lower-confidence variant (a single property, no
    loop, so the risk is codegen-dependent stack-slot reuse of its block-scoped
    locals) plus the same hint-string leak. Left as a follow-up so every change here
    is a definite, test-backed fix.

Deliberately left as-is (self-review notes)

A self-review surfaced a few lower-severity items that are intentionally not
changed here, to keep every edit a definite fix:

  • BindSignal native-memory (> ParameterSpanThreshold) branch is
    integration-only.
    That branch's NativeMemory.Alloc/Free is inseparable from
    the classdb_register_extension_class_signal call, which needs a running editor;
    the added test covers the conversion helper above the threshold, but the heap
    alloc/free itself can only be exercised by integration.
  • Error-path hint-string disposal is best-effort. On the happy path the native
    strings are disposed after registration; if a conversion or the native register
    call were to throw partway, strings created so far would not be disposed. That
    path aborts extension loading (fatal) and matches the surrounding code's existing
    behavior (e.g. BindProperty doesn't dispose its hint string at all). The one
    reachable trigger, an invalid optional/required parameter ordering is now
    validated up front, before any string is created, so it can't leak.
  • A few micro-allocations are intentional: BindSignal keeps its fixed-size
    stack buffers even when it takes the heap path (the standard small-on-stack /
    large-on-heap idiom needs an unconditional stack buffer), and BindVirtualMethod
    reuses the shared parameter converter, so it passes default-value buffers the
    virtual path never reads. Both are negligible and favor sharing the tested helper
    over duplicating it.

This was found while auditing the registration path for adoption-blocking bugs. An issue was not
previously filed.

When registering a method, virtual method, or signal, the managed parameter
info is converted to the unmanaged GDExtensionPropertyInfo, whose name,
class_name, and hint_string fields are pointers. The engine only reads those
pointers later, when classdb_register_extension_class_method/signal/virtual_method
is called at the end of the conversion. The converted values were stored in
loop- and block-scoped locals whose addresses (&local) were written into the
property info, so their stack slots were reused on the next iteration or after
the block closed. Every parameter ended up aliasing the last parameter's name,
class name, hint string, and default value, and the return value's pointer
fields dangled the same way. Classes with more than one parameter registered
with corrupted metadata (all parameters reporting the last one's name, etc.).

Convert the parameters into parallel buffers with one slot per parameter, so
each property info points at its own stable slot that stays alive until
registration. Hoist the return value info and the method name into the
registration frame for the same reason. The method and signal paths share the
conversion through new internal helpers.

The NativeGodotString hint strings created during conversion were also never
disposed; the engine copies the data at registration, so dispose them once
afterwards.

Validate the optional-parameter ordering in an up-front pass, before any native
string is created, so an invalid definition throws without leaking the strings
converted so far.

Add unit tests for the extracted conversion helpers that assert each parameter
gets its own slot (pointers pairwise distinct), that the default values match
the parameter they belong to (which the aliasing bug collapsed into the last
parameter's value), and that the default-value array is compacted onto the
trailing optional parameters when they follow a required one.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@cobriensr
cobriensr force-pushed the fix-param-registration-aliasing branch from 08ff45d to b41e35c Compare July 18, 2026 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant