Skip to content

Fix over-release of caller-owned interface pointer in CreateObject - #2511

Open
Sergio Pedri (Sergio0694) wants to merge 3 commits into
staging/3.0from
user/sergiopedri/comwrappers-reentrant-marshalling
Open

Fix over-release of caller-owned interface pointer in CreateObject#2511
Sergio Pedri (Sergio0694) wants to merge 3 commits into
staging/3.0from
user/sergiopedri/comwrappers-reentrant-marshalling

Conversation

@Sergio0694

Copy link
Copy Markdown
Member

Summary

Fixes a reference count over-release in WindowsRuntimeComWrappers.CreateObject, which released an interface pointer owned by the caller whenever a marshalling operation re-entered the marshalling infrastructure on the same thread.

Motivation

CreateObject receives the IInspectable interface pointer through the CreateObjectTargetInterfacePointer thread-static. That pointer is normally supplied (and owned) by the caller, but in the WeakReference<T> rehydration path there is no caller context, so CreateObject has to QueryInterface for it itself and release it afterwards.

To tell those two cases apart, the finally block re-read CreateObjectTargetInterfacePointer and treated null as "I acquired this myself". That inference is only valid as long as nothing resets the field while CreateObject is running, and GetOrCreateObjectForComInstanceUnsafe resets it to null on exit. Any nested marshalling operation therefore left the field null, so the outer finally released a pointer it did not own, dropping the reference count one time too many.

This is reachable today. Marshalling a NotifyCollectionChangedEventArgs resolves through the most-derived-type lookup to its ComWrappers marshaller, whose CreateObject calls NotifyCollectionChangedEventArgsMarshaller.ConvertToManaged. That in turn marshals the NewItems/OldItems collections through IListMarshaller.ConvertToManaged, which re-enters GetOrCreateObjectForComInstanceUnsafe.

While fixing that, the same shared state turned out to have a second, independent problem: because GetOrCreateObjectForComInstanceUnsafe unconditionally cleared the callbacks rather than restoring them, a nested marshalling operation also destroyed the outer operation's static type information. Once control returned to the outer CreateObject, the fallback that would have produced a wrapper specialized to the statically known type saw a null callback and silently degraded to an opaque IInspectable wrapper.

Changes

  • src/WinRT.Runtime2/InteropServices/WindowsRuntimeComWrappers.cs: track whether CreateObject acquired the interface pointer itself in a local, instead of inferring it in the finally block from shared thread-static state. This is the actual fix for the over-release.

  • src/WinRT.Runtime2/InteropServices/WindowsRuntimeComWrappers.cs: save and restore the three marshalling thread-statics in GetOrCreateObjectForComInstanceUnsafe instead of always writing null, so a nested marshalling operation no longer destroys the outer operation's state. For a top-level call the saved values are null, so the existing invariant that these fields are null outside a marshalling operation is preserved, and the WeakReference<T> rehydration path keeps behaving as before.

  • src/Tests/UnitTest/ComWrappersTests.cs: new regression test covering re-entrant marshalling. The shipping trigger (NotifyCollectionChangedEventArgs) needs the WinUI runtime to activate, which is not available in the unit test host, so the test instead uses a minimal native IInspectable with a hand-written vtable whose GetRuntimeClassName marshals a second native object. That callback runs inside the outer CreateObject, reproducing the same re-entrancy without any activation dependency. Rather than hard-coding how many references the marshalling infrastructure takes internally, the test marshals twice (once re-entrant, once not) and asserts that both have the same net effect on the reference count of the caller-owned pointer, which keeps it robust to unrelated internal changes.

Validation

The new test was verified against each state of the fix:

Runtime state Result
Before the fix Fails with Expected:<1>. Actual:<0>
First commit only Passes
Both commits Passes

Actual:<0> is exactly the expected symptom: the spurious Release cancels out the reference that the created wrapper takes.

Running the full UnitTest suite before and after the change goes from 432 failed / 81 passed to 431 failed / 82 passed, so exactly one test flips and there are no regressions. The remaining failures are pre-existing and environmental: the C++ test component is not registered in the local test host, so those tests fail with REGDB_E_CLASSNOTREG while constructing the test class.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

'CreateObject' captured the caller-supplied 'IInspectable' pointer from the
'CreateObjectTargetInterfacePointer' thread-static, but then decided in its
'finally' block whether it owned that pointer by re-reading the same field and
checking it for 'null'. That inference is only valid if nothing resets the
field in between.

Re-entrant marshalling on the same thread does exactly that:
'GetOrCreateObjectForComInstanceUnsafe' resets the field to 'null' when it
returns, so any nested marshalling operation performed while the outer
'CreateObject' is still running leaves the field 'null'. The outer 'finally'
then calls 'Release' on an interface pointer that was supplied by the caller
and that it does not own, dropping the reference count too many times.

This is reachable today: marshalling a 'NotifyCollectionChangedEventArgs'
resolves through the most-derived-type lookup to its ComWrappers marshaller,
whose 'CreateObject' calls
'NotifyCollectionChangedEventArgsMarshaller.ConvertToManaged'. That in turn
marshals the 'NewItems'/'OldItems' collections through
'IListMarshaller.ConvertToManaged', which re-enters
'GetOrCreateObjectForComInstanceUnsafe'.

Track the acquisition in a local instead, so the release decision no longer
depends on shared thread-static state.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 15b56f71-7292-4049-bf75-17f6198d7446
'GetOrCreateObjectForComInstanceUnsafe' communicates the static type callbacks
and the caller-supplied interface pointer to 'CreateObject' through
thread-statics, and unconditionally cleared them once the call returned.

Clearing is only correct for a top-level marshalling operation. When a
marshaller also marshals nested objects, this method is re-entered while an
outer marshalling operation is still in flight, and clearing the fields
destroys the outer operation's state. Once control returns to the outer
'CreateObject', it observes callbacks that are no longer there: the fallback
that would have produced a wrapper specialized to the statically known type
sees a 'null' callback, and silently degrades to an opaque 'IInspectable'
wrapper instead.

Save the previous values and restore them, instead of always writing 'null'.
For a top-level call the saved values are 'null', so the existing invariant
that these fields are 'null' outside a marshalling operation is preserved, and
the 'WeakReference<T>' rehydration path keeps behaving as before.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 15b56f71-7292-4049-bf75-17f6198d7446
Adds a test covering the case that was broken: a native object whose
marshalling re-enters the marshalling infrastructure on the same thread while
the outer 'CreateObject' call is still running.

The shipping example of this is 'NotifyCollectionChangedEventArgs', but that
type requires the WinUI runtime to activate, which is not available in this
test host. The test instead uses a minimal native 'IInspectable' with a
hand-written vtable, whose 'GetRuntimeClassName' marshals a second native
object. That callback runs inside the outer 'CreateObject', reproducing the
same re-entrancy.

Rather than hard-coding how many references the marshalling infrastructure
takes internally, the test marshals twice (once re-entrant, once not) and
asserts that both have the same net effect on the reference count of the
caller-owned interface pointer.

Verified against the parent commits: the test fails before the fix with
'Expected:<1>. Actual:<0>' (the caller-owned pointer is released one extra
time, cancelling out the reference the wrapper takes) and passes after it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 15b56f71-7292-4049-bf75-17f6198d7446
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working CsWinRT 3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant