[Android] Prevent race condition by synchronizing buffer access (#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]>

Author: kotlarmilos

src/libraries/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Ssl.cs

@@ -78,6 +78,7 @@ private static unsafe partial int SSLStreamInitializeImpl(
             IntPtr managedContextHandle,
             delegate* unmanaged<IntPtr, byte*, int*, PAL_SSLStreamStatus> streamRead,
             delegate* unmanaged<IntPtr, byte*, int, void> streamWrite,
+            delegate* unmanaged<IntPtr, void> managedContextCleanup,
             int appBufferSize,
             [MarshalAs(UnmanagedType.LPUTF8Str)] string? peerHost);
         internal static unsafe void SSLStreamInitialize(
@@ -86,10 +87,11 @@ internal static unsafe void SSLStreamInitialize(
             IntPtr managedContextHandle,
             delegate* unmanaged<IntPtr, byte*, int*, PAL_SSLStreamStatus> streamRead,
             delegate* unmanaged<IntPtr, byte*, int, void> streamWrite,
+            delegate* unmanaged<IntPtr, void> managedContextCleanup,
             int appBufferSize,
             string? peerHost)
         {
-            int ret = SSLStreamInitializeImpl(sslHandle, isServer, managedContextHandle, streamRead, streamWrite, appBufferSize, peerHost);
+            int ret = SSLStreamInitializeImpl(sslHandle, isServer, managedContextHandle, streamRead, streamWrite, managedContextCleanup, appBufferSize, peerHost);
             if (ret != SUCCESS)
                 throw new SslException();
         }

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

@@ -7,6 +7,7 @@
 using System.Security.Authentication;
 using System.Security.Cryptography;
 using System.Security.Cryptography.X509Certificates;
+using System.Threading;
 
 using PAL_KeyAlgorithm = Interop.AndroidCrypto.PAL_KeyAlgorithm;
 using PAL_SSLStreamStatus = Interop.AndroidCrypto.PAL_SSLStreamStatus;
@@ -29,13 +30,17 @@ internal sealed class SafeDeleteSslContext : SafeDeleteContext
 
         private readonly SafeSslHandle _sslContext;
 
+        private readonly Lock _lock = new Lock();
+
         private ArrayBuffer _inputBuffer = new ArrayBuffer(InitialBufferSize);
         private ArrayBuffer _outputBuffer = new ArrayBuffer(InitialBufferSize);
 
         public SslStream.JavaProxy SslStreamProxy { get; }
 
         public SafeSslHandle SslContext => _sslContext;
 
+        private volatile bool _disposed;
+
         public SafeDeleteSslContext(SslAuthenticationOptions authOptions)
             : base(IntPtr.Zero)
         {
@@ -59,13 +64,21 @@ public SafeDeleteSslContext(SslAuthenticationOptions authOptions)
 
         protected override void Dispose(bool disposing)
         {
-            if (disposing)
+            lock (_lock)
             {
-                if (_sslContext is SafeSslHandle sslContext)
+                if (!_disposed)
                 {
-                    _inputBuffer.Dispose();
-                    _outputBuffer.Dispose();
-                    sslContext.Dispose();
+                    _disposed = true;
+
+                    // First dispose the SSL context to trigger native cleanup
+                    _sslContext.Dispose();
+
+                    if (disposing)
+                    {
+                        // Then dispose the buffers
+                        _inputBuffer.Dispose();
+                        _outputBuffer.Dispose();
+                    }
                 }
             }
 
@@ -75,61 +88,105 @@ protected override void Dispose(bool disposing)
         [UnmanagedCallersOnly]
         private static unsafe void WriteToConnection(IntPtr connection, byte* data, int dataLength)
         {
-            SafeDeleteSslContext? context = (SafeDeleteSslContext?)GCHandle.FromIntPtr(connection).Target;
-            Debug.Assert(context != null);
+            WeakGCHandle<SafeDeleteSslContext> h = WeakGCHandle<SafeDeleteSslContext>.FromIntPtr(connection);
+            if (!h.TryGetTarget(out SafeDeleteSslContext? context))
+            {
+                Debug.Write("WriteToConnection: failed to get target context");
+                return;
+            }
+
+            lock (context._lock)
+            {
+                if (context._disposed)
+                {
+                    Debug.Write("WriteToConnection: context is disposed");
+                    return;
+                }
 
-            var inputBuffer = new ReadOnlySpan<byte>(data, dataLength);
+                var inputBuffer = new ReadOnlySpan<byte>(data, dataLength);
 
-            context._outputBuffer.EnsureAvailableSpace(dataLength);
-            inputBuffer.CopyTo(context._outputBuffer.AvailableSpan);
-            context._outputBuffer.Commit(dataLength);
+                context._outputBuffer.EnsureAvailableSpace(dataLength);
+                inputBuffer.CopyTo(context._outputBuffer.AvailableSpan);
+                context._outputBuffer.Commit(dataLength);
+            }
         }
 
         [UnmanagedCallersOnly]
         private static unsafe PAL_SSLStreamStatus ReadFromConnection(IntPtr connection, byte* data, int* dataLength)
         {
-            SafeDeleteSslContext? context = (SafeDeleteSslContext?)GCHandle.FromIntPtr(connection).Target;
-            Debug.Assert(context != null);
-
-            int toRead = *dataLength;
-            if (toRead == 0)
-                return PAL_SSLStreamStatus.OK;
-
-            if (context._inputBuffer.ActiveLength == 0)
+            WeakGCHandle<SafeDeleteSslContext> h = WeakGCHandle<SafeDeleteSslContext>.FromIntPtr(connection);
+            if (!h.TryGetTarget(out SafeDeleteSslContext? context))
             {
+                Debug.Write("ReadFromConnection: failed to get target context");
                 *dataLength = 0;
-                return PAL_SSLStreamStatus.NeedData;
+                return PAL_SSLStreamStatus.Error;
             }
 
-            toRead = Math.Min(toRead, context._inputBuffer.ActiveLength);
+            lock (context._lock)
+            {
+                if (context._disposed)
+                {
+                    Debug.Write("ReadFromConnection: context is disposed");
+                    *dataLength = 0;
+                    return PAL_SSLStreamStatus.Error;
+                }
+
+                int toRead = *dataLength;
+                if (toRead == 0)
+                    return PAL_SSLStreamStatus.OK;
+
+                if (context._inputBuffer.ActiveLength == 0)
+                {
+                    *dataLength = 0;
+                    return PAL_SSLStreamStatus.NeedData;
+                }
+
+                toRead = Math.Min(toRead, context._inputBuffer.ActiveLength);
 
-            context._inputBuffer.ActiveSpan.Slice(0, toRead).CopyTo(new Span<byte>(data, toRead));
-            context._inputBuffer.Discard(toRead);
+                context._inputBuffer.ActiveSpan.Slice(0, toRead).CopyTo(new Span<byte>(data, toRead));
+                context._inputBuffer.Discard(toRead);
+
+                *dataLength = toRead;
+                return PAL_SSLStreamStatus.OK;
+            }
+        }
 
-            *dataLength = toRead;
-            return PAL_SSLStreamStatus.OK;
+        [UnmanagedCallersOnly]
+        private static void CleanupManagedContext(IntPtr managedContextHandle)
+        {
+            if (managedContextHandle != IntPtr.Zero)
+            {
+                WeakGCHandle<SafeDeleteSslContext> handle = WeakGCHandle<SafeDeleteSslContext>.FromIntPtr(managedContextHandle);
+                handle.Dispose();
+            }
         }
 
         internal void Write(ReadOnlySpan<byte> buf)
         {
-            _inputBuffer.EnsureAvailableSpace(buf.Length);
-            buf.CopyTo(_inputBuffer.AvailableSpan);
-            _inputBuffer.Commit(buf.Length);
+            lock (_lock)
+            {
+                _inputBuffer.EnsureAvailableSpace(buf.Length);
+                buf.CopyTo(_inputBuffer.AvailableSpan);
+                _inputBuffer.Commit(buf.Length);
+            }
         }
 
         internal int BytesReadyForConnection => _outputBuffer.ActiveLength;
 
         internal void ReadPendingWrites(ref ProtocolToken token)
         {
-            if (_outputBuffer.ActiveLength == 0)
+            lock (_lock)
             {
-                token.Size = 0;
-                token.Payload = null;
-                return;
-            }
+                if (_outputBuffer.ActiveLength == 0)
+                {
+                    token.Size = 0;
+                    token.Payload = null;
+                    return;
+                }
 
-            token.SetPayload(_outputBuffer.ActiveSpan);
-            _outputBuffer.Discard(_outputBuffer.ActiveLength);
+                token.SetPayload(_outputBuffer.ActiveSpan);
+                _outputBuffer.Discard(_outputBuffer.ActiveLength);
+            }
         }
 
         internal int ReadPendingWrites(byte[] buf, int offset, int count)
@@ -139,12 +196,15 @@ internal int ReadPendingWrites(byte[] buf, int offset, int count)
             Debug.Assert(count >= 0);
             Debug.Assert(count <= buf.Length - offset);
 
-            int limit = Math.Min(count, _outputBuffer.ActiveLength);
+            lock (_lock)
+            {
+                int limit = Math.Min(count, _outputBuffer.ActiveLength);
 
-            _outputBuffer.ActiveSpan.Slice(0, limit).CopyTo(new Span<byte>(buf, offset, limit));
-            _outputBuffer.Discard(limit);
+                _outputBuffer.ActiveSpan.Slice(0, limit).CopyTo(new Span<byte>(buf, offset, limit));
+                _outputBuffer.Discard(limit);
 
-            return limit;
+                return limit;
+            }
         }
 
         private static SafeSslHandle CreateSslContext(SslStream.JavaProxy sslStreamProxy, SslAuthenticationOptions authOptions)
@@ -225,11 +285,11 @@ private unsafe void InitializeSslContext(
                 throw new NotImplementedException(nameof(SafeDeleteSslContext));
             }
 
-            // Make sure the class instance is associated to the session and is provided
-            // in the Read/Write callback connection parameter
+            // Make sure the class instance is associated to the session and is provided in the Read/Write callback connection parameter
+            // Additionally, all calls should be synchronous so there's no risk of the managed object being collected while native code is executing.
             IntPtr managedContextHandle = GCHandle.ToIntPtr(GCHandle.Alloc(this, GCHandleType.Weak));
             string? peerHost = !isServer && !string.IsNullOrEmpty(authOptions.TargetHost) ? authOptions.TargetHost : null;
-            Interop.AndroidCrypto.SSLStreamInitialize(handle, isServer, managedContextHandle, &ReadFromConnection, &WriteToConnection, InitialBufferSize, peerHost);
+            Interop.AndroidCrypto.SSLStreamInitialize(handle, isServer, managedContextHandle, &ReadFromConnection, &WriteToConnection, &CleanupManagedContext, InitialBufferSize, peerHost);
 
             if (authOptions.EnabledSslProtocols != SslProtocols.None)
             {

src/libraries/tests.proj

@@ -225,8 +225,6 @@
   </ItemGroup>
 
   <ItemGroup Condition="'$(TargetOS)' == 'android' and '$(RuntimeFlavor)' == 'CoreCLR' and '$(RunDisabledAndroidTests)' != 'true'">
-    <!-- https://github.com/dotnet/runtime/issues/117045 -->
-    <ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Net.Http\tests\FunctionalTests\System.Net.Http.Functional.Tests.csproj" />
     <!-- https://github.com/dotnet/runtime/issues/114951 -->
     <!-- https://github.com/dotnet/runtime/issues/62547 -->
     <ProjectExclusions Include="$(MSBuildThisFileDirectory)System.Security.Cryptography\tests\System.Security.Cryptography.Tests.csproj" />
@@ -604,6 +602,7 @@
     <SmokeTestProject Include="$(MSBuildThisFileDirectory)System.Diagnostics.Tracing\tests\System.Diagnostics.Tracing.Tests.csproj" />
     <SmokeTestProject Include="$(MSBuildThisFileDirectory)System.Security.Cryptography\tests\System.Security.Cryptography.Tests.csproj" />
     <SmokeTestProject Include="$(MSBuildThisFileDirectory)System.Net.WebSockets.Client\tests\System.Net.WebSockets.Client.Tests.csproj" />
+    <SmokeTestProject Include="$(MSBuildThisFileDirectory)System.Net.Http\tests\FunctionalTests\System.Net.Http.Functional.Tests.csproj" />
     <SmokeTestProject Include="$(RepoRoot)\src\tests\FunctionalTests\Android\Device_Emulator\JIT\*.Test.csproj"/>
   </ItemGroup>
 

src/native/libs/System.Security.Cryptography.Native.Android/pal_sslstream.c

@@ -411,6 +411,9 @@ ARGS_NON_NULL_ALL static void FreeSSLStream(JNIEnv* env, SSLStream* sslStream)
     ReleaseGRef(env, sslStream->netOutBuffer);
     ReleaseGRef(env, sslStream->netInBuffer);
     ReleaseGRef(env, sslStream->appInBuffer);
+
+    sslStream->managedContextCleanup(sslStream->managedContextHandle);
+
     free(sslStream);
 }
 
@@ -650,7 +653,7 @@ SSLStream* AndroidCryptoNative_SSLStreamCreateWithKeyStorePrivateKeyEntry(intptr
 }
 
 int32_t AndroidCryptoNative_SSLStreamInitialize(
-    SSLStream* sslStream, bool isServer, ManagedContextHandle managedContextHandle, STREAM_READER streamReader, STREAM_WRITER streamWriter, int32_t appBufferSize, char* peerHost)
+    SSLStream* sslStream, bool isServer, ManagedContextHandle managedContextHandle, STREAM_READER streamReader, STREAM_WRITER streamWriter, MANAGED_CONTEXT_CLEANUP managedContextCleanup, int32_t appBufferSize, char* peerHost)
 {
     abort_if_invalid_pointer_argument (sslStream);
     abort_unless(sslStream->sslContext != NULL, "sslContext is NULL in SSL stream");
@@ -706,6 +709,7 @@ int32_t AndroidCryptoNative_SSLStreamInitialize(
     sslStream->managedContextHandle = managedContextHandle;
     sslStream->streamReader = streamReader;
     sslStream->streamWriter = streamWriter;
+    sslStream->managedContextCleanup = managedContextCleanup;
 
     ret = SUCCESS;
 

src/native/libs/System.Security.Cryptography.Native.Android/pal_sslstream.h

@@ -11,6 +11,7 @@
 typedef intptr_t ManagedContextHandle;
 typedef void (*STREAM_WRITER)(ManagedContextHandle, uint8_t*, int32_t);
 typedef int32_t (*STREAM_READER)(ManagedContextHandle, uint8_t*, int32_t*);
+typedef void (*MANAGED_CONTEXT_CLEANUP)(ManagedContextHandle);
 
 typedef struct SSLStream
 {
@@ -24,6 +25,7 @@ typedef struct SSLStream
     ManagedContextHandle managedContextHandle;
     STREAM_READER streamReader;
     STREAM_WRITER streamWriter;
+    MANAGED_CONTEXT_CLEANUP managedContextCleanup;
 } SSLStream;
 
 typedef struct ApplicationProtocolData_t ApplicationProtocolData;
@@ -67,15 +69,16 @@ PALEXPORT SSLStream* AndroidCryptoNative_SSLStreamCreateWithKeyStorePrivateKeyEn
 
 /*
 Initialize an SSL context
-  - isServer      : true if the context should be created in server mode
-  - streamReader  : callback for reading data from the connection
-  - streamWriter  : callback for writing data to the connection
-  - appBufferSize : initial buffer size for application data
+  - isServer                : true if the context should be created in server mode
+  - streamReader            : callback for reading data from the connection
+  - streamWriter            : callback for writing data to the connection
+  - managedContextCleanup   : callback for cleaning up the managed context
+  - appBufferSize           : initial buffer size for application data
 
 Returns 1 on success, 0 otherwise
 */
 PALEXPORT int32_t AndroidCryptoNative_SSLStreamInitialize(
-    SSLStream* sslStream, bool isServer, ManagedContextHandle managedContextHandle, STREAM_READER streamReader, STREAM_WRITER streamWriter, int32_t appBufferSize, char* peerHost);
+    SSLStream* sslStream, bool isServer, ManagedContextHandle managedContextHandle, STREAM_READER streamReader, STREAM_WRITER streamWriter, MANAGED_CONTEXT_CLEANUP managedContextCleanup, int32_t appBufferSize, char* peerHost);
 
 /*
 Set target host