[Android] Prevent race condition by synchronizing buffer access #117950

kotlarmilos · 2025-07-22T21:00:03Z

Problem

Android SSL callbacks WriteToConnection/ReadFromConnection receive an IntPtr that should reference a GCHandle to SafeDeleteSslContext. The handle was freed while native code could still invoke the callbacks with the IntPtr referencing a different object. This led to a NullReferenceException or InvalidCastException:

System.InvalidCastException: Unable to cast object of type 'System.Threading.Thread' to type 'System.Net.SafeDeleteSslContext'

Description

This PR synchronizes all buffer mutations behind a shared _lock, and updates the managed callbacks to verify that the handle is still allocated and its target is a SafeDeleteSslContext before using it.

Changes

Implemented WriteToConnection/ReadFromConnection checks for allocated GCHandle
Implemented lock in all buffer mutations to prevent race condition

Testing

The System.Net.Http.Functional.Tests now pass without the crash on Android.

Fixes #117045

…Handle

kotlarmilos · 2025-07-22T21:01:14Z

/azp run runtime-android,runtime-androidemulator

azure-pipelines · 2025-07-22T21:01:20Z

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

kotlarmilos · 2025-07-22T21:02:22Z

/azp run runtime-android,runtime-androidemulator

azure-pipelines · 2025-07-22T21:02:33Z

Azure Pipelines successfully started running 2 pipeline(s).

dotnet-policy-service · 2025-07-22T21:05:02Z

Tagging subscribers to 'arch-android': @vitek-karas, @simonrozsival, @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Copilot

Pull Request Overview

This PR fixes a race condition in Android SSL implementation where GCHandle references were being freed while native callbacks could still access them, causing InvalidCastException or NullReferenceException. The fix extends the GCHandle lifetime to match the SafeDeleteSslContext object and adds proper synchronization for buffer operations.

Key changes:

Replace weak GCHandle allocation with persistent handle that lives for the full object lifetime
Add comprehensive locking around all buffer operations to prevent race conditions
Implement null/disposed checks in native callbacks with graceful error handling

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
src/libraries/tests.proj	Re-enables System.Net.Http.Functional.Tests for Android and adds it to smoke tests
src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs	Implements persistent GCHandle, thread-safe buffer access, and defensive callback handling

src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs

…Android/SafeDeleteSslContext.cs Co-authored-by: Jan Kotas <[email protected]>

kotlarmilos · 2025-07-23T16:26:43Z

/azp run runtime-android,runtime-androidemulator

azure-pipelines · 2025-07-23T16:26:55Z

Azure Pipelines successfully started running 2 pipeline(s).

src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs

kotlarmilos · 2025-07-23T21:38:42Z

/azp run runtime-android,runtime-androidemulator

azure-pipelines · 2025-07-23T21:38:55Z

Azure Pipelines successfully started running 2 pipeline(s).

…n methods

kotlarmilos · 2025-07-24T08:49:08Z

/azp run runtime-android,runtime-androidemulator

azure-pipelines · 2025-07-24T08:49:25Z

Azure Pipelines successfully started running 2 pipeline(s).

kotlarmilos · 2025-07-24T09:06:56Z

/azp run runtime-android,runtime-androidemulator

azure-pipelines · 2025-07-24T09:07:08Z

Azure Pipelines successfully started running 2 pipeline(s).

kotlarmilos · 2025-07-24T10:07:10Z

/azp run runtime-android,runtime-androidemulator

azure-pipelines · 2025-07-24T10:07:21Z

Azure Pipelines successfully started running 2 pipeline(s).

jkotas · 2025-07-24T17:02:56Z

src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs

+            catch (Exception ex)
+            {
+                Debug.Write("Exception Caught. - " + ex);
+                throw;


Exceptions thrown from UnmanagedCallersOnly methods turn into unhandled exceptions (on non-Windows). This means that the app is going to crash if any exceptions are thrown by this method. Is it what we want? Should the exceptions be propagated to the user instead?

If you want to keep the behavior and crash the app if any exception is thrown here, it is better to do that without the try/catch. It is better for diagnostics. try/catch/rethrow can overwrite some of the details that may be useful for diagnosing the exceptions, and the unhandled exception handler should print the same details as the Debug.Write to the console.

Thanks for the context. I agree -- it should be propagated to the user.

jkotas · 2025-07-24T17:08:17Z

src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs

+                if (handle.IsAllocated)
+                {


Suggested change

if (handle.IsAllocated)

{

This is unnecessary. Non-null managedContextHandle is always going to turn into Allocated GCHandle (even if the handle was freed somewhere else already).

GCHandle is struct that is as dangerous as unsafe code. If you call Free on one copy of the struct, the other copies will still report IsAllocated==true even through the handle was freed.

jkotas · 2025-07-24T17:09:53Z

src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs

+            GCHandle h = GCHandle.FromIntPtr(connection);
+            Debug.Assert(h.IsAllocated);
+
+            SafeDeleteSslContext? context = (SafeDeleteSslContext?)h.Target;


You can consider switching to new GCHandle<T> to avoid this cast

(Actually, WeakGCHandle<T>.)

Thanks, updated. TryGetTarget handles both invalid GCHandle and null context.

…nection methods to use WeakGCHandle

kotlarmilos · 2025-07-25T08:31:21Z

/azp run runtime-android,runtime-androidemulator

azure-pipelines · 2025-07-25T08:31:32Z

Azure Pipelines successfully started running 2 pipeline(s).

kotlarmilos added 5 commits July 22, 2025 10:12

Dispose SafeSslHandle and use thread-safe operations on PAL layer

cc97c25

Refactor SSL stream handling to remove atomic operations and use Safe…

9c1b904

…Handle

Remove newline

1d36a5e

Improve thread safety during disposal

e5524a6

Revert changes

0c18e08

kotlarmilos requested review from simonrozsival and vitek-karas July 22, 2025 21:00

github-actions bot added the area-System.Net.Security label Jul 22, 2025

kotlarmilos assigned simonrozsival and kotlarmilos Jul 22, 2025

kotlarmilos added the os-android label Jul 22, 2025

kotlarmilos added this to the 10.0.0 milestone Jul 22, 2025

kotlarmilos unassigned simonrozsival Jul 22, 2025

Merge branch 'main' into bugfix/android-coreclr-ssl-context

b561265

kotlarmilos marked this pull request as ready for review July 23, 2025 08:14

Copilot AI review requested due to automatic review settings July 23, 2025 08:14

Copilot AI reviewed Jul 23, 2025

View reviewed changes

ManickaP reviewed Jul 23, 2025

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs Outdated Show resolved Hide resolved

vitek-karas reviewed Jul 23, 2025

View reviewed changes

build-analysis bot mentioned this pull request Jul 23, 2025

[WASM] Hundreds of tests failing without log #117772

Open

kotlarmilos mentioned this pull request Jul 23, 2025

[Apple] Lock over the same object in SafeDeleteSslContext to serialize access to the buffers #117982

Merged

kotlarmilos and others added 2 commits July 23, 2025 16:41

Update src/libraries/System.Net.Security/src/System/Net/Security/Pal.…

c813751

…Android/SafeDeleteSslContext.cs Co-authored-by: Jan Kotas <[email protected]>

Eliminate GC handle ConcurrentDictionary

4525c6d

kotlarmilos changed the title ~~[Android] Prevent race condition by extending GCHandle lifetime and synchronizing buffer access~~ [Android] Prevent race condition by synchronizing buffer access Jul 23, 2025

MihaZupan reviewed Jul 23, 2025

View reviewed changes

src/libraries/System.Net.Security/src/System/Net/Security/Pal.Android/SafeDeleteSslContext.cs Outdated Show resolved Hide resolved

This was referenced Jul 23, 2025

[QUIC & HTTP/3] Handshake Timeout on tests #104426

Open

System.OperationCanceledException : The operation was canceled. dotnet/dnceng#5278

Open

System.Xml.Tests failing #117614

Open

browser-wasm linux Release LibraryTests queues timing out #117974

Open

Add managed context cleanup

66dfb1f

Improve exception handling in WriteToConnection and ReadFromConnectio…

e228f91

…n methods

Replace object lock with class

e95bc03

Always release native _sslContext

e38433f

jkotas reviewed Jul 24, 2025

View reviewed changes

kotlarmilos added 2 commits July 25, 2025 10:26

Improve error handling and refactor WriteToConnection and ReadFromCon…

d35aa7e

…nection methods to use WeakGCHandle

Fix formatting

9fd0e8e

[Android] Prevent race condition by synchronizing buffer access #117950

Are you sure you want to change the base?

[Android] Prevent race condition by synchronizing buffer access #117950

Conversation

kotlarmilos commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Description

Changes

Testing

Uh oh!

kotlarmilos commented Jul 22, 2025

Uh oh!

azure-pipelines bot commented Jul 22, 2025

Uh oh!

kotlarmilos commented Jul 22, 2025

Uh oh!

azure-pipelines bot commented Jul 22, 2025

Uh oh!

dotnet-policy-service bot commented Jul 22, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kotlarmilos commented Jul 23, 2025

Uh oh!

azure-pipelines bot commented Jul 23, 2025

Uh oh!

Uh oh!

kotlarmilos commented Jul 23, 2025

Uh oh!

azure-pipelines bot commented Jul 23, 2025

Uh oh!

kotlarmilos commented Jul 24, 2025

Uh oh!

azure-pipelines bot commented Jul 24, 2025

Uh oh!

kotlarmilos commented Jul 24, 2025

Uh oh!

azure-pipelines bot commented Jul 24, 2025

Uh oh!

kotlarmilos commented Jul 24, 2025

Uh oh!

azure-pipelines bot commented Jul 24, 2025

Uh oh!

jkotas Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

kotlarmilos Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

jkotas Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

jkotas Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

kotlarmilos Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

kotlarmilos commented Jul 25, 2025

Uh oh!

azure-pipelines bot commented Jul 25, 2025

Uh oh!

Uh oh!

kotlarmilos commented Jul 22, 2025 •

edited

Loading

kotlarmilos Jul 25, 2025 •

edited

Loading