search

dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.18k stars 9.93k forks source link

CancellationTokenSource potential leaks in SignalR HubConnection #57232

Open leeriorio opened 1 month ago

leeriorio commented 1 month ago

Describe the bug

there is CancellationTokenSource type variable in src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs

https://github.com/dotnet/aspnetcore/blob/257d69079e0f7fc84e3f6cd5047272d7f79b4d66/src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs#L1599-L1600

and in finally block of this private method only Cancel() method calls

https://github.com/dotnet/aspnetcore/blob/257d69079e0f7fc84e3f6cd5047272d7f79b4d66/src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs#L1704

How about to Dispose this CTS object in finally block or add using construction in its declaration to eliminate potential memory leaks?

Found by Linux Verification Center (linuxtesting.org) with SVACE.

BrennanConroy commented 1 month ago

Sure, we should dispose as a good practice especially since it should be as easy as adding it after Cancel().

In this specific case there wouldn't be a leak because Cancel() disposes the internal timer and the native objects in the ManualResetEvent are wrapped in SafeHandles so are finalized. (It'd also be a super slow leak if there was one since we create one of these per connection)