[release/9.0-staging] [Android] Prevent race condition by synchronizing buffer access#124629
Open
kotlarmilos wants to merge 3 commits intodotnet:release/9.0-stagingfrom
Open
Conversation
…et#117950) * Dispose SafeSslHandle and use thread-safe operations on PAL layer * Refactor SSL stream handling to remove atomic operations and use SafeHandle * Remove newline * Improve thread safety during disposal * Revert changes * Fix disposal logic in SafeDeleteSslContext to prevent double disposal. Fix race condition on _disposed * Update SafeDeleteSslContext to manage GC handles using a static ConcurrentDictionary * Update src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs Co-authored-by: Jan Kotas <[email protected]> * Eliminate GC handle ConcurrentDictionary * Add managed context cleanup * Improve exception handling in WriteToConnection and ReadFromConnection methods * Replace object lock with class * Always release native _sslContext * Improve error handling and refactor WriteToConnection and ReadFromConnection methods to use WeakGCHandle * Fix formatting * Update src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs Co-authored-by: Jan Kotas <[email protected]> * Replace handle.Free() with handle.Dispose() * Refactor SSL stream cleanup --------- Co-authored-by: Jan Kotas <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR backports a fix from .NET 10 (#117950) to address a race condition in Android SSL buffer access that causes crashes in read/write/close paths. The fix introduces synchronized access to SSL buffers using a dedicated lock object and adds a cleanup callback mechanism to properly manage the GCHandle lifecycle.
Changes:
- Adds a cleanup callback mechanism for managed context in native Android SSL code
- Introduces lock-based synchronization for all buffer access operations in SafeDeleteSslContext
- Implements disposed state checking to prevent use-after-dispose scenarios
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/System.Security.Cryptography.Native.Android/pal_sslstream.h | Adds MANAGED_CONTEXT_CLEANUP callback typedef and field to SSLStream struct |
| src/native/libs/System.Security.Cryptography.Native.Android/pal_sslstream.c | Implements cleanup callback invocation in FreeSSLStream and adds managedContextCleanup parameter to initialization |
| src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs | Adds Lock field, _disposed flag, synchronizes all buffer access with locks, implements CleanupManagedContext callback, but uses non-existent WeakGCHandle type |
| src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Ssl.cs | Adds managedContextCleanup parameter to SSLStreamInitialize interop signature |
| src/libraries/tests.proj | Removes trailing whitespace (cosmetic change) |
src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs
Outdated
Show resolved
Hide resolved
Comment on lines
117
to
118
| WeakGCHandle<SafeDeleteSslContext> h = WeakGCHandle<SafeDeleteSslContext>.FromIntPtr(connection); | ||
| if (!h.TryGetTarget(out SafeDeleteSslContext? context)) |
There was a problem hiding this comment.
Same issue as in WriteToConnection: WeakGCHandle does not exist. Should use GCHandle.FromIntPtr with IsAllocated check and Target casting pattern.
src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs
Outdated
Show resolved
Hide resolved
Contributor
|
Tagging subscribers to 'arch-android': @vitek-karas, @simonrozsival, @steveisok, @akoeplinger |
Member
Author
|
Only lock synchronization changes are backported, WeakGCHandle and managedContextCleanup changes are excluded. |
This was referenced Feb 20, 2026
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
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.
Backport of #117950 to release/9.0-staging
/cc @kotlarmilos
Customer Impact
The customer started getting a number of crashes in read/write/close paths #124215. The majority of the crashes occur on iOS, but a very similar issue was also observed on Android. All reported crashes were on .NET 9.0.12.
Regression
The issue is not a recent regression in .NET 9, but an existing race condition that has been present for some time.
The fix was implemented in .NET 10 as part of #117950 while addressing related issues (#117045, #117982), and this PR backports that fix to .NET 9 servicing.
Testing
Stress and concurrency testing of SSL read/write/close paths on Android platforms. Manual attempts of the original crash scenarios on iOS and Android, with no crashes observed after the fix.
Risk
Low
The change is narrowly scoped and mirrors an already-shipped fix in .NET 10. It only introduces a dedicated private lock to ensure consistent sync across all code paths accessing shared SSL buffers.
IMPORTANT: If this backport is for a servicing release, please verify that:
release/X.0-staging, notrelease/X.0.release/X.0(no-stagingsuffix).Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.