Draft: use-after-free guards#1928
Draft
SteveSandersonMS wants to merge 6 commits into
Draft
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
SteveSandersonMS
force-pushed
the
stevesa/csharp-dispose-graceful
branch
from
35847dc to
62012e2
Compare
This comment has been minimized.
This comment has been minimized.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| self.FeedInbound(bytesPtr, bytesLen); | ||
| } | ||
| RouteOutbound(userData, bytesPtr, (UIntPtr)bytesLen); |
| _cliEntrypoint = cliEntrypoint; | ||
| _environment = environment; | ||
| _logger = logger; | ||
| s_diagnosticLogger = logger; |
|
|
||
| private int RegisterOutboundToken() | ||
| { | ||
| _outboundToken = Interlocked.Increment(ref s_nextOutboundToken); |
Comment on lines
+378
to
+381
| catch (Exception ex) | ||
| { | ||
| _logger.LogDebug(ex, "FfiRuntimeHost: connection_close failed"); | ||
| } |
… CI leg Removes the redundant per-transport StopClientForCleanupAsync helper so E2E cleanup uses graceful StopAsync uniformly, and re-enables the windows+inprocess .NET SDK CI leg for the in-process FFI investigation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…after-free The native runtime can invoke the outbound callback from a tokio task that outlives Dispose (connection_close/host_shutdown don't join it). The old code passed his as a GCHandle in user_data and dereferenced it in the callback, so a late callback dereferenced a freed handle and crashed the process with an AccessViolationException, cascading into unrelated E2E tests. Instead, mint an integer token per host, keep a ConcurrentDictionary registry, and pass the token as user_data. Dispose unregisters the token first so any callback fired during/after teardown is logged (visible in CI) and dropped rather than faulting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The inbound write path (.NET->Rust connection_write) had no mutual exclusion against a host's native teardown (connection_close + host_shutdown). Because all in-process FFI hosts share process-global native state (the single runtime.node, its shared tokio runtime, and the napi-oop provider bridge), one host tearing that state down while another host's connection_write P/Invoke was executing in native code dereferenced half-freed shared state and crashed the whole test-host process with an AccessViolationException, cascading into unrelated tests. Add a process-global ReaderWriterLockSlim: connection_write takes the read lock (independent connections still write concurrently); a host's native teardown takes the exclusive write lock. This guarantees no write is executing in native code while any host tears the shared state down. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n a booting child host_start blocks until the embedded child signals readiness, but a Dispose that raced an in-flight StartAsync previously saw the native ids as still-zero and did nothing, leaving the fully-started host orphaned (never torn down). Worse, if the provider were closed while a child was still loading its package index, the child aborts the shared in-process test host with "peer is closed". Serialize StartAsync's publish of the native ids against Dispose with a lock so whichever runs second performs the teardown of the fully-started host. StartAsync now stages the server/connection in locals and, if it observes _disposed after host_start returns, cleanly tears the just-started child down instead of publishing it. Extract the gated native teardown into a shared TeardownNative helper used by both paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The net472 target adds no in-process transport coverage; restrict the windows inprocess leg to net8.0 (net472 stays covered by the windows default leg). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SteveSandersonMS
force-pushed
the
stevesa/csharp-dispose-graceful
branch
from
e699b25 to
b5dd9e5
Compare
SteveSandersonMS
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Status
There's a shutdown race that affects .NET+Windows+inproc specifically. This does not seem likely to affect any real application, since real applications typically start up one runtime instance and then only shut it down when the application itself is shutting down. Our E2E tests, on the other hand, start up many hundreds of runtime instances in the same process in quick succession, rapidly creating and destroying them.
This PR adds a set of additional guards in the .NET FFI transport to detect or block use-after-free in both inbound and outbound message cases. But it's still not enough - there's a remaining use-after-free (or similar) in the Rust->napi-oop->Node bridge in certain shutdown cases. This manifests in the build error you can see for this PR:
It might be that before we get to dealing with this, the problem disappears naturally (in ~2 weeks we hope not to have napi-oop any more). Once we do remove napi-oop, we should see if the .NET+Windows+inproc combination immediately becomes robust when re-enabling its CI leg and then we can simply close this draft PR.
If not, we should consider productizing one or more of the use-after-free guards/locks from this PR.
This PR also temporarily disables the net472 part of the CI leg. That is simply to help diagnose whether issues are specific to .NET Framework or not. We do not plan to remove net472 support.