Closed Applesauce314 closed 3 years ago
I have a proposed fix in https://github.com/Applesauce314/aspnetcore/tree/Applesauce314-patch-1
but have not made a PR yet as I do not currently have a setup capable of building and running tests.
It is a modified version of Proposed Solution 2. Except that it does not create a new WindowsPrincipal
, and clones any members of User.Identites
that are type WindowsIndentity
. This Clone should increment the reference count of the underlying SafeHandle
of the WindowsIndentity
and allow both the HttpContext
and the HttpConnectionContext
to be disposed in either order.
It removes a bunch of code from HttpConnectionDispatcher
that was doing half the work of Cloning sometimes as the implentation makes HttpConnectionContext
fully responsible.
@Applesauce314 Can you provide a stack trace for the ObjectDisposedException
that you're seeing?
Triage: LongPolling was a red-herring. In the websocket (and SSE) case, we shouldn't disposing the WindowsIdentity
Contoso.Common.Logging.UserEnricher uses an Injected HttpContextAccessor to access the HttpContext the same way as some built in Serilog providers. Link
I verified that dispose on the HttpContext has not yet been called. I put a breakpoint in WindowsIdentity.Dispose and found the point at which the object in question was being disposed. The call stack is below.
I then followed the path by which `HTTPConnectionContext.User is assigned, and that brought me to HttpConnectionDispatcher.
From this we see in our case (websockets) Clone
is not being called on the WindowsIdentity
objects, but Dispose is.
I then followed the source of the code that perform the dispose Blame HttpConnectionContext L257-264 and see that this was added to support LongPolling by https://github.com/dotnet/aspnetcore/commit/f0a34a4ca46129b8434020dedb8c360e751e2963 with bug fixes in https://github.com/dotnet/aspnetcore/pull/11051 .
Prior to these 2 changes, HttpConnectionContext
never disposed the WindowsIdentity
objects, and always assigned the HttpContext.User
without cloning, but after the changes it always disposes the WindowsIdentity
objects, but only sometimes clones the objects.
bug repo project created at https://github.com/Applesauce314/HttpConnectionDisposalIssue
From this we see in our case (websockets)
Clone
is not being called on theWindowsIdentity
objects, but Dispose is.
Right, the bug here is that we are disposing the user on non-longpolling transports https://github.com/dotnet/aspnetcore/issues/37521#issuecomment-944654017
Should be a simple fix of checking the transport type in the dispose logic.
Describe the bug
The changes introduced to fix issue #2985 (with issues corrected by #11051) update
Microsoft.AspNetCore.Http.Connections.Internal.HttpConnectionDispatcher.EnsureConnectionStateAsync
to CloneWindowsIdentity
objects when long polling instead of assigning theUser
Property from theHttpContext
.HttpConnectionDispatcher
https://github.com/dotnet/aspnetcore/blob/c83777e4e6ec8ded364cf45d398910e3575d5367/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs#L538-L572It also added disposal of these cloned objects to
Microsoft.AspNetCore.Http.Connections.Internal.HttpConnectionContext.DisposeAsync
. However it disposes anyWindowsIdentity
objects inHttpConnectionContext.User.Identities
even if theUser
property was assigned fromHttpContext.User
.HttpConnectionContext
https://github.com/dotnet/aspnetcore/blob/c83777e4e6ec8ded364cf45d398910e3575d5367/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionContext.cs#L258-L264In some circumstances the HttpContext outlives the HttpConnectionContext and an
ObjectDisposedException
will be thrown if certain properties of the identity is accessed viaHttpContext.User.Identities
orHttpContext.User.Identity
. My code is triggering this issue in logging code when logging where a Serilog Enricher is providing username using using an injectedHttpContextAccessor
. But I expect this could be triggered numerous ways.EndpointMiddleware
https://github.com/dotnet/aspnetcore/blob/c83777e4e6ec8ded364cf45d398910e3575d5367/src/Http/Routing/src/EndpointMiddleware.cs#L83Code
``` private readonly IHttpContextAccessor _contextAccessor; public UserEnricher() : this(new HttpContextAccessor()) { } internal UserEnricher(IHttpContextAccessor contextAccessor) { _contextAccessor = contextAccessor; } public void Enrich(LogEvent logEvent, ILogEventPropertyFactory propertyFactory) { string? username; username = _contextAccessor.HttpContext?.User?.Identity)?.Name; var userProperty = new LogEventProperty(UserPropertyName, new ScalarValue(username)); logEvent.AddPropertyIfAbsent(userProperty); } } } ```To Reproduce
more detailed steps can be provided, but the extra disposals are pretty apparent from the linked code.
Exceptions (if any)
ObjectDisposedException {"Safe handle has been closed. Object name: 'SafeHandle'."}
Proposed Solution
I think this would be solved one of two ways
HttpConnectionContext
should keep track ofWindowsIdentity
objects it is reponsible for and dispose only those.HttpConnectionContext
should always create a newWindowPrincipal
and Clone the IdentitiesFurther technical details
dotnet --info Output
``` .NET SDK (reflecting any global.json): Version: 5.0.303 Commit: 6409b42649 Runtime Environment: OS Name: Windows OS Version: 10.0.16299 OS Platform: Windows RID: win10-x64 Base Path: C:\Program Files\dotnet\sdk\5.0.303\ Host (useful for support): Version: 5.0.10 Commit: e1825b4928 .NET SDKs installed: 3.1.118 [C:\Program Files\dotnet\sdk] 5.0.301 [C:\Program Files\dotnet\sdk] 5.0.302 [C:\Program Files\dotnet\sdk] 5.0.303 [C:\Program Files\dotnet\sdk] .NET runtimes installed: Microsoft.AspNetCore.All 2.1.28 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All] Microsoft.AspNetCore.All 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All] Microsoft.AspNetCore.App 2.1.28 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 3.1.16 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 3.1.17 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 3.1.18 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 5.0.10 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.NETCore.App 2.1.28 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 2.1.30 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 3.1.16 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 3.1.17 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 3.1.18 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 5.0.10 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.WindowsDesktop.App 3.1.16 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 3.1.17 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 3.1.18 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.8 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.9 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 5.0.10 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] To install additional .NET runtimes or SDKs: https://aka.ms/dotnet-download ```Edited to include all the relevant lines from
HttpConnectionDispatcher