Runtime: AccessViolationException when retrieving enum from dependency container

[chrarnoldus]

When trying to run our app on .NET 5 preview4 we ran into the following issue. Consider the following simplified code:

```c#
using System.Threading.Tasks;
using Microsoft.AspNetCore;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.DependencyInjection;

namespace Progress.WebApp
{
public enum TheEnum
{
HelloWorld = -1,
}

public sealed class Startup
{
    static void Main(string[] args)
        => WebHost.CreateDefaultBuilder(args)
            .UseStartup<Startup>()
            .Build()
            .Run();

    public void ConfigureServices(IServiceCollection services)
        => services.AddSingleton(typeof(TheEnum), TheEnum.HelloWorld);

    public void Configure(IApplicationBuilder app)
        => app.UseMiddleware<Middleware>();
}

public sealed class Middleware
{
    readonly RequestDelegate next;

    public Middleware(RequestDelegate next)
    {
        this.next = next;
    }

    public async Task Invoke(HttpContext context, TheEnum theEnum)
        => await context.Response.WriteAsync(theEnum.ToString());
}

}

Project:
```xml
<Project Sdk="Microsoft.NET.Sdk.Web">
  <PropertyGroup>
    <TargetFramework>net5.0</TargetFramework>
  </PropertyGroup>
</Project>

Running this code using dotnet run and navigating to localhost:5000 prints "HelloWorld" the first time as expected, but after hitting F5 a few times the server crashes with the following exception:

Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at DynamicClass.lambda_method1(System.Runtime.CompilerServices.Closure, System.Object, Microsoft.AspNetCore.Http.HttpContext, System.IServiceProvider)
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+<>c__DisplayClass4_1.<UseMiddleware>b__2(Microsoft.AspNetCore.Http.HttpContext)
   at Microsoft.AspNetCore.HostFiltering.HostFilteringMiddleware.Invoke(Microsoft.AspNetCore.Http.HttpContext)
   at Microsoft.AspNetCore.Hosting.HostingApplication.ProcessRequestAsync(Context)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequest>d__218`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[[Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequest>d__218`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], Microsoft.AspNetCore.Server.Kestrel.Core, Version=5.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]](<ProcessRequest>d__218`1<System.__Canon> ByRef)
   at System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder.Start[[Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequest>d__218`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], Microsoft.AspNetCore.Server.Kestrel.Core, Version=5.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]](<ProcessRequest>d__218`1<System.__Canon> ByRef)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequest[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]](Microsoft.AspNetCore.Hosting.Server.IHttpApplication`1<System.__Canon>)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__217`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Threading.Tasks.VoidTaskResult, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__217`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], Microsoft.AspNetCore.Server.Kestrel.Core, Version=5.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]].ExecutionContextCallback(System.Object)
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(System.Threading.Thread, System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Threading.Tasks.VoidTaskResult, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__217`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], Microsoft.AspNetCore.Server.Kestrel.Core, Version=5.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]].MoveNext(System.Threading.Thread)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Threading.Tasks.VoidTaskResult, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__217`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], Microsoft.AspNetCore.Server.Kestrel.Core, Version=5.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]].ExecuteFromThreadPool(System.Threading.Thread)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading._ThreadPoolWaitCallback.PerformWaitCallback()

Changing the underlying enum value to 1 (from -1) causes the server to no longer crash, but instead a NullReferenceException is thrown during request processing (despite the first few request still working as expected):

      Connection id "0HLVSO0VBBCEI", Request id "0HLVSO0VBBCEI:00000004": An unhandled exception was thrown by the application.
System.NullReferenceException: Object reference not set to an instance of an object.
   at lambda_method1(Closure , Object , HttpContext , IServiceProvider )
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass4_1.<UseMiddleware>b__2(HttpContext context)
   at Microsoft.AspNetCore.HostFiltering.HostFilteringMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Hosting.HostingApplication.ProcessRequestAsync(Context context)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequest[TContext](IHttpApplication`1 application)

Output dotnet --info

.NET SDK (reflecting any global.json):
 Version:   5.0.100-preview.4.20258.7
 Commit:    65f0fc2cad

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19041
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\5.0.100-preview.4.20258.7\

Host (useful for support):
  Version: 5.0.0-preview.4.20251.6
  Commit:  47ec733ba7

[davidfowl]

Hmm I thought we fixed this here https://github.com/dotnet/extensions/issues/2431

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[msftbot[bot]]

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

[ericstj]

Confirmed that this repro's on RC1.

Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at DynamicClass.lambda_method1(System.Runtime.CompilerServices.Closure, System.Object, Microsoft.AspNetCore.Http.HttpContext, System.IServiceProvider)
   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions+<>c__DisplayClass5_1.<UseMiddleware>b__2(Microsoft.AspNetCore.Http.HttpContext)
   at Microsoft.AspNetCore.HostFiltering.HostFilteringMiddleware.Invoke(Microsoft.AspNetCore.Http.HttpContext)
   at Microsoft.AspNetCore.Hosting.HostingApplication.ProcessRequestAsync(Context)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__223`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].MoveNext()
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Threading.Tasks.VoidTaskResult, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__223`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], Microsoft.AspNetCore.Server.Kestrel.Core, Version=5.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]].ExecutionContextCallback(System.Object)
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(System.Threading.Thread, System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Threading.Tasks.VoidTaskResult, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__223`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], Microsoft.AspNetCore.Server.Kestrel.Core, Version=5.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]].MoveNext(System.Threading.Thread)
   at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.Threading.Tasks.VoidTaskResult, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol+<ProcessRequests>d__223`1[[System.__Canon, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]], Microsoft.AspNetCore.Server.Kestrel.Core, Version=5.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60]].ExecuteFromThreadPool(System.Threading.Thread)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading._ThreadPoolWaitCallback.PerformWaitCallback()

It seems similar to https://github.com/dotnet/extensions/issues/2431 as @davidfowl mentioned,. In fact the current issue does not reproduce on 3.1 and as far as I can tell the only significant change here is the the fix for that, so it's possible that fix regressed this behavior.

[ericstj]

I've confirmed that reverting the fix https://github.com/dotnet/extensions/commit/b248ede5665ef0ba59252731a40f697cc4ac40ad fixes this issue. We should decide if we need to keep that fix and patch it or just undo it for 5.0. cc @maryamariyan @eerhardt

[davidfowl]

That's extremely odd, but we have a test so it's worth doing if we can prove it. That fix wasn't there originally https://github.com/dotnet/extensions/issues/2431

[eerhardt]

I’ll dig more today, but debugging it last night the method would return the right result the first two times it was called. And then the third time, the JIT would kick in again and then the AV would occur during the 3rd invocation. This happened pretty regularly.

~So my initial assumption is that it might happen only with tiered compilation. I’ll test this hypothesis today by shutting tiered compilation off and trying it.~

UPDATE: I turned off tiered JIT and it still occurs. Still digging.

[ericstj]

cc @pakrym
One thing we might try to check is write the same code that accesses the ILEmitResolverBuilderRuntimeContext using these types and see what the compiler generates, and compare that to what this code is Emitting.

[eerhardt]

So more things I've learned:

1. The reason this only happens after the 3rd invocation is because of this code:

https://github.com/dotnet/runtime/blob/b1ca1f4eb040e42045718e265775f79eea982d32/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/DynamicServiceProviderEngine.cs#L23-L28

The first 2 times a service gets retrieved, it uses reflection (RuntimeResolver.Resolve) to get the service. But once it is called a 2nd time, it calls the base.RealizeService in the background, which generates the IL and sticks it in the cache. So the next time it is retrieved, the generated IL is invoked.

1. The reason the A/V is happening is because 0xFFFFFFFF is being returned as an object from ServiceProvider.GetService. This "object" is then trying to be cast down/unboxed to a TheEnum value. 0xFFFFFFFF is -1, which is the HelloWorld value. So what is happening is a double unboxing of the enum, causing the runtime to A/V.

2. The reason the double unboxing is occuring is what @ericstj pointed to above - https://github.com/dotnet/extensions/commit/b248ede5665ef0ba59252731a40f697cc4ac40ad that change added an unbox call during VisitConstant. The bug that was fixing was when a constant ValueType was being passed into a constructor of a service. In this scenario, the constant ValueType is the service itself. In this scenario, we shouldn't be unboxing the value during VisitConstant.

To fix this, I think we can simply set a bool during VisitConstructor saying that we are inConstructor. And only when we are inConstructor during VisitConstant do we do the unboxing. When we are visiting a constant at the top-level, skip adding the unboxing code.

I'll prepare a fix for this.

[ericstj]

To fix this, I think we can simply set a bool during VisitConstructor

Are we certain the only scenario this code is used is in this case? Is there a part of this visitor pattern that "knows" what the constant will be used for? I think the reason this bug was introduced was an assumption that "value types should always be unboxed" and that assumption didn't consider that there's multiple contexts where the code would be used where some require the actual type and others require it to remain object. It'd be better if we could understand if ref here is value type or not.

Or even lift up the unboxing to the caller who knows that the value needs to be the specific type, and keep the assumption of VisitConstant that it always returns the boxed value?

[ericstj]

Looking through the code some more I see that ConstructorCallSite is the only thing that includes other ServiceCallSites so I guess this is the only case that a ConstantCallSite would be used where it needed the actual type? I'm still not 100% convinced.

[eerhardt]

I looked through as well and found that

https://github.com/dotnet/runtime/blob/b1ca1f4eb040e42045718e265775f79eea982d32/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ILEmit/ILEmitResolverBuilder.cs#L217-L236

will call it as well. Both the Ctor and the IEnumerable code paths will need the value unboxed, from what I can tell. I've added a test for IEnumerable as well.

[ericstj]

Yeah, you beat me to it. I discovered that as I was looking for more callers to ConstantCallSite. I think if we trace all callers to ConstantCallSite.ctor we'll find the paths that need to consider this.

[davidfowl]

I love watching this analysis in real time 😄

[eerhardt]

I've opened https://github.com/dotnet/runtime/pull/42152 which will fix this issue. PTAL.

[eerhardt]

Re-opening for the backport to 5.0.

[eerhardt]

backport to 5.0 has been merged. Closing

[davidfowl]

@eerhardt did we backport this to 3.x as well?

[eerhardt]

No. According to @ericstj above:

In fact the current issue does not reproduce on 3.1

[davidfowl]

I'm not sure about that

[ericstj]

The original report was a result of https://github.com/dotnet/extensions/commit/b248ede5665ef0ba59252731a40f697cc4ac40ad which was made in 5.0. This change introduced double-unboxing which caused the AV. When running without that fix I didn’t observe an AV. My understanding was that most of the scenarios enabled by that change and this one were not possible in 3.1 (value types), but https://github.com/dotnet/extensions/commit/b248ede5665ef0ba59252731a40f697cc4ac40ad regressed this scenario (enums) which did work in 3.1. You are welcome to backport to 3.1 if it’s worth it but you’d need to take that dependent PR and I suspect it’s more like adding a feature. Cc @Pilchie