From 5a6ec8e00a482016cc8b50eb4d69101781a97513 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Thu, 13 Mar 2025 08:50:18 +0100 Subject: [PATCH 1/2] Fix double dispose of GCHandle in BrowserWebSocket MemoryHandle is a struct, so passing it into an IDisposable parameter creates a box. This causes two copies of GCHandle to exist which are no longer synchronized and can be double disposed. This happened when passing pinBuffer into CancellationHelper in ReceiveAsyncCore and SendAsyncCore. In both cases the caller does the Dispose itself inside a finally, so there's no need to pass the pinBuffer to CancellationHelper at all. --- .../Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index d21e80ba41fee5..ed954a6006c456 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -399,7 +399,7 @@ private async Task SendAsyncCore(ArraySegment buffer, WebSocketMessageType if (sendTask != null) // this is optimization for single-threaded build, see resolvedPromise() in web-socket.ts. Null means synchronously resolved. { - await CancellationHelper(sendTask, cancellationToken, previousState, pinBuffer).ConfigureAwait(false); + await CancellationHelper(sendTask, cancellationToken, previousState).ConfigureAwait(false); } } catch (JSException ex) @@ -442,7 +442,7 @@ private async Task ReceiveAsyncCore(ArraySegment b if (receiveTask != null) // this is optimization for single-threaded build, see resolvedPromise() in web-socket.ts. Null means synchronously resolved. { - await CancellationHelper(receiveTask, cancellationToken, previousState, pinBuffer).ConfigureAwait(false); + await CancellationHelper(receiveTask, cancellationToken, previousState).ConfigureAwait(false); } return ConvertResponse(); From 871cbb925fa723f41b171fb100a16440700be12f Mon Sep 17 00:00:00 2001 From: pavelsavara Date: Fri, 14 Mar 2025 13:14:19 +0100 Subject: [PATCH 2/2] cleanup --- .../Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs index ed954a6006c456..826c3beda3e3dd 100644 --- a/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs +++ b/src/libraries/System.Net.WebSockets.Client/src/System/Net/WebSockets/BrowserWebSockets/BrowserWebSocket.cs @@ -550,13 +550,12 @@ private async Task CloseAsyncCore(WebSocketCloseStatus closeStatus, string? stat } } - private async Task CancellationHelper(Task promise, CancellationToken cancellationToken, WebSocketState previousState, IDisposable? disposable = null) + private async Task CancellationHelper(Task promise, CancellationToken cancellationToken, WebSocketState previousState) { try { if (promise.IsCompletedSuccessfully) { - disposable?.Dispose(); return; } if (promise.IsCompleted) @@ -602,10 +601,6 @@ private async Task CancellationHelper(Task promise, CancellationToken cancellati throw new WebSocketException(WebSocketError.NativeError, ex); } } - finally - { - disposable?.Dispose(); - } } // needs to be called with locked _lockObject