Move _cachedSendPinnedBuffer ownership to WinHttpRequestState (#101725)

Fixes a race between `WinHttpRequestStream.WriteAsync` and `WinHttpRequestStream.Dispose` that can lead to AV.
dotnet · May 8, 2024 · a7702f4 · a7702f4
1 parent b42426c
commit a7702f4
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 22 deletions.
diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestState.cs b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestState.cs
@@ -157,6 +157,7 @@ public WinHttpTransportContext TransportContext
         public long CurrentBytesRead { get; set; }
 
         private GCHandle _cachedReceivePinnedBuffer;
+        private GCHandle _cachedSendPinnedBuffer;
 
         public void PinReceiveBuffer(byte[] buffer)
         {
@@ -171,6 +172,19 @@ public void PinReceiveBuffer(byte[] buffer)
             }
         }
 
+        public void PinSendBuffer(byte[] buffer)
+        {
+            if (!_cachedSendPinnedBuffer.IsAllocated || _cachedSendPinnedBuffer.Target != buffer)
+            {
+                if (_cachedSendPinnedBuffer.IsAllocated)
+                {
+                    _cachedSendPinnedBuffer.Free();
+                }
+
+                _cachedSendPinnedBuffer = GCHandle.Alloc(buffer, GCHandleType.Pinned);
+            }
+        }
+
         #region IDisposable Members
         private void Dispose(bool disposing)
         {
@@ -193,12 +207,18 @@ private void Dispose(bool disposing)
             {
                 // This method only gets called when the WinHTTP request handle is fully closed and thus all
                 // async operations are done. So, it is safe at this point to unpin the buffers and release
-                // the strong GCHandle for this object.
+                // the strong GCHandle for the pinned buffers.
                 if (_cachedReceivePinnedBuffer.IsAllocated)
                 {
                     _cachedReceivePinnedBuffer.Free();
                     _cachedReceivePinnedBuffer = default(GCHandle);
                 }
+
+                if (_cachedSendPinnedBuffer.IsAllocated)
+                {
+                    _cachedSendPinnedBuffer.Free();
+                    _cachedSendPinnedBuffer = default(GCHandle);
+                }
 #if DEBUG
                 Interlocked.Increment(ref s_dbg_operationHandleFree);
 #endif

diff --git a/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestStream.cs b/src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpRequestStream.cs
@@ -24,8 +24,6 @@ internal sealed class WinHttpRequestStream : Stream
         private readonly SafeWinHttpHandle _requestHandle;
         private readonly WinHttpChunkMode _chunkedMode;
 
-        private GCHandle _cachedSendPinnedBuffer;
-
         internal WinHttpRequestStream(WinHttpRequestState state, WinHttpChunkMode chunkedMode)
         {
             _state = state;
@@ -182,15 +180,7 @@ internal async Task EndUploadAsync(CancellationToken token)
 
         protected override void Dispose(bool disposing)
         {
-            if (!_disposed)
-            {
-                _disposed = true;
-                if (_cachedSendPinnedBuffer.IsAllocated)
-                {
-                    _cachedSendPinnedBuffer.Free();
-                }
-            }
-
+            _disposed = true;
             base.Dispose(disposing);
         }
 
@@ -234,16 +224,7 @@ private Task<bool> InternalWriteDataAsync(byte[] buffer, int offset, int count,
         {
             Debug.Assert(count > 0);
 
-            if (!_cachedSendPinnedBuffer.IsAllocated || _cachedSendPinnedBuffer.Target != buffer)
-            {
-                if (_cachedSendPinnedBuffer.IsAllocated)
-                {
-                    _cachedSendPinnedBuffer.Free();
-                }
-
-                _cachedSendPinnedBuffer = GCHandle.Alloc(buffer, GCHandleType.Pinned);
-            }
-
+            _state.PinSendBuffer(buffer);
             _state.TcsInternalWriteDataToRequestStream =
                 new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);