Server crashes due to null reference when connection is aborted before frame is created

[yuwaMSFT]

InnerException: StackTrace (generated): SP IP Function 00007F0C7E7FB810 00007F0CD98A161A Microsoft.AspNetCore.Server.Kestrel.dll!Microsoft.AspNetCore.Server.Kestrel.Http.Connection+<>c.b__17_0(System.Object)+0x7a 00007F0C7E7FB840 00007F0CD7C7A061 mscorlib.ni.dll!System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)+0xe1 00007F0C7E7FB890 00007F0CD7DEB613 mscorlib.ni.dll!System.Threading.QueueUserWorkItemCallbackDefaultContext.System.Threading.IThreadPoolWorkItem.ExecuteWorkItem()+0x43 00007F0C7E7FB8B0 00007F0CD7CE5DA1 mscorlib.ni.dll!System.Threading.ThreadPoolWorkQueue.Dispatch()+0x1b1

StackTraceString: HResult: 80004003

Nested exception ------------------------------------------------------------- Exception object: 00007f0cb8e5d7f8 Exception type: System.NullReferenceException Message: Object reference not set to an instance of an object. InnerException: StackTrace (generated): SP IP Function 00007F0C7E7FB810 00007F0CD98A161A Microsoft.AspNetCore.Server.Kestrel.dll!Microsoft.AspNetCore.Server.Kestrel.Http.Connection+<>c.b__17_0(System.Object)+0x7a 00007F0C7E7FB840 00007F0CD7C7A061 mscorlib.ni.dll!System.Threading.ExecutionContext.Run(System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)+0xe1 00007F0C7E7FB890 00007F0CD7DEB613 mscorlib.ni.dll!System.Threading.QueueUserWorkItemCallbackDefaultContext.System.Threading.IThreadPoolWorkItem.ExecuteWorkItem()+0x43 00007F0C7E7FB8B0 00007F0CD7CE5DA1 mscorlib.ni.dll!System.Threading.ThreadPoolWorkQueue.Dispatch()+0x1b1

(lldb) sos DumpObj 00007f0cb8e2ea48 Name: Microsoft.AspNetCore.Server.Kestrel.Http.Connection MethodTable: 00007f0cd82afb80 EEClass: 00007f0cd82bc328 Size: 240(0xf0) bytes File: /mnt/aspnet/PerformanceSSL/testapp/StressMvc/bin/Debug/netstandardapp1.5/ubuntu.14.04-x64/publish/Microsoft.AspNetCore.Server.Kestrel.dll Fields: MT Field Offset Type VT Attr Value Name 00007f0cd8344578 4000015 8 ...plicationLifetime 0 instance 00007f0cb8019fd0 kBackingField 00007f0cd82ac1f8 4000016 10 ...ure.IKestrelTrace 0 instance 00007f0cb80719d8 kBackingField 00007f0cd82ae510 4000017 18 ...cture.IThreadPool 0 instance 00007f0cb80751d8 kBackingField 00007f0cd87776d0 4000018 20 ....Server.Kestrel]] 0 instance 00007f0cb8075198 kBackingField 00007f0cd82afef0 4000019 28 ...eaderValueManager 0 instance 00007f0cb8071798 kBackingField 00007f0cd82ab218 400001a 30 ...ServerInformation 0 instance 00007f0cb8041080 kBackingField 00007f0cd82ae030 400001b 38 ...pComponentFactory 0 instance 00007f0cb80413f8 kBackingField 00007f0cd82abb80 4000170 40 ...rel.ServerAddress 0 instance 00007f0cb8088450 kBackingField 00007f0cd82ac0d8 4000171 48 ...rel.KestrelThread 0 instance 00007f0cb8075e40 kBackingField 00007f0cd82ae7b8 4000172 50 ...ucture.MemoryPool 0 instance 00007f0cb8088660 kBackingField 00007f0cd82afce8 4000173 58 ...ConnectionManager 0 instance 00007f0cb808c4d8 kBackingField 00007f0cd879ee68 4000174 60 ....Server.Kestrel]] 0 instance 00007f0cb8088858 kBackingField 00007f0cd82ea658 40000e9 68 ....Http.SocketInput 0 instance 0000000000000000 kBackingField 00007f0cd82b8770 40000ea 70 ...ttp.ISocketOutput 0 instance 0000000000000000 kBackingField 00007f0cd82af780 40000eb 78 ...ConnectionControl 0 instance 00007f0cb8e2ea48 kBackingField 00007f0cd87a0b50 40000ec 80 ...em.Net.IPEndPoint 0 instance 00007f0cb8e2f6b0 kBackingField 00007f0cd87a0b50 40000ed 88 ...em.Net.IPEndPoint 0 instance 00007f0cb8e2f6f8 kBackingField 00007f0cd7f60510 40000ee 90 System.String 0 instance 00007f0cb8e2eb50 kBackingField 00007f0cd87ba2f0 40000ef 98 ...e.Http.Features]] 0 instance 0000000000000000 k__BackingField 00007f0cd82ad9b8 40000e0 a0 ...ng.UvStreamHandle 0 instance 00007f0cb8e2e9c8 socket **# 00007f0cd82b4320 40000e1 a8 ...estrel.Http.Frame 0 instance 0000000000000000 frame** 00007f0cd82eb090 40000e2 b0 ...tionFilterContext 0 instance 00007f0cb8e2f750 _filterContext 00007f0cd82eb438 40000e3 b8 ...ilter.LibuvStream 0 instance 00007f0cb8e2f718 _libuvStream 00007f0cd82ea658 40000e4 c0 ....Http.SocketInput 0 instance 00007f0cb8e2eb88 _rawSocketInput 00007f0cd82eaa98 40000e5 c8 ...Http.SocketOutput 0 instance 00007f0cb8e2ec28 _rawSocketOutput 00007f0cd7f62e18 40000e6 d0 System.Object 0 instance 00007f0cb8e2eb38 _stateLock 00007f0cd82af9f8 40000e7 e0 System.Int32 1 instance 4 _connectionState 00007f0cd82e79f8 40000e8 d8 ...bject, mscorlib]] 0 instance 0000000000000000 _socketClosedTcs 00007f0cd7f60510 40000dc 108 System.String 0 static 00007f0cb8089028 _encode32Chars 00007f0cd87a6720 40000dd 110 ...bject, mscorlib]] 0 static 00007f0cb8089088 _readCallback 00007f0cd87a6a28 40000de 118 ....Server.Kestrel]] 0 static 00007f0cb80890e0 _allocCallback 00007f0cd7f86548 40000df 180 System.Int64 1 static 635960817163641803 _lastConnectionId

[yuwaMSFT]

    public virtual void Abort()
    {
        // Frame.Abort calls user code while this method is always
        // called from a libuv thread.
        System.Threading.ThreadPool.QueueUserWorkItem(state =>
        {
            var connection = (Connection)state;

            lock (connection._stateLock)
            {
                if (connection._connectionState == ConnectionState.CreatingFrame)
                {
                    connection._connectionState = ConnectionState.ToDisconnect;
                }
                else
                {
                    **connection._frame.Abort(); -------------------->_frame is not generated yet.**
                }
            }
        }, this);
    }

[yuwaMSFT]

@halter73 @muratg @sivagms @SajayAntony

[muratg]

@yuwaMSFT Nice find!

[halter73]

Nice find. What scenario did you find this in?

I think that the socket was closed prior to the frame object being created, which lead to this race. The thing that indicates this to me is:

00007f0cd82af9f8 40000e7 e0 System.Int32 1 instance 4 _connectionState

A _connectionState of 4 means that the socket is closed. We set the socket to closed with the _stateLock, but we change the state even if we haven't finished creating the _frame object (which is during ConnectionState.CreatingFrame).

I think the fix might be to change

if (connection._connectionState == ConnectionState.CreatingFrame)
{
    connection._connectionState = ConnectionState.ToDisconnect;
}
else
{
    **connection._frame.Abort(); -------------------->_frame is not generated yet.**
}

to

if (connection._connectionState == ConnectionState.CreatingFrame)
{
    connection._connectionState = ConnectionState.ToDisconnect;
}
else if (connection._connectionState == ConnectionState.Open)
{
    connection._frame.Abort();
}

[halter73]

@muratg @Eilon @DamianEdwards Should we fix this for RC2?

[muratg]

How easy is it to hit this?

[sajayantony]

I guess its worth fixing this for RC2. One bad client can bring down the server due to this.

[benaadams]

If its a full on server crash; which as its a naked exception thrown on threadpool it probably needs fixing.

@halter73 other work around, though not as good as fixing the race, would be to run it through the logging threadpool, so the exception is at least localized and doesn't take down the server.

[yuwaMSFT]

It's a race condition so as Sajay mentioned one bad client could take down the server.

I was running some HTTPS test where clients could accidently close the connection immediately due to server certificate validation error (I was using the self-signed certificate).

[sajayantony]

@halter73 Race or not - Aborts/Close for Socket APIs should always be handled and should NOT be scheduled at the bottom of the stack. That QUWI seems odd.

[muratg]

I see. I think it makes sense to consider this for RC2. @cesarbs / @halter73 -- could you guys look into a fix and request approval from @Eilon and @DamianEdwards?

[benaadams]

@SajayAntony I assume the QUWI is to get the abort off the libuv IO thread rather than blocking its IO for all connections.

@halter73 did you mean: connection._frame.Abort(); -> connection._frame?.Abort();

[sajayantony]

@benaadams scheduling is ok but without exception handling is the odd part 😨

[DamianEdwards]

Approved for RC2

[benaadams]

@SajayAntony all the sub calls have exception handling (e.g. _frame.Abort())

[sajayantony]

@benaadams thats good and yet I think it makes sense to have a guard frame at the bottom since QUIW obviously doesn't protect the process. For e.g. - https://github.com/dotnet/wcf/blob/be6afd58c2af0bdd3c2561403934deed4d2bb5d9/src/System.Private.ServiceModel/src/Internals/System/Runtime/Fx.cs#L953

[benaadams]

@SajayAntony there is one for QUIW that calls into user code as the user code may not handle their exceptions https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNetCore.Server.Kestrel/Infrastructure/LoggingThreadPool.cs#L24-L64

Also I have requested it in coreclr, but its not got much traction so far https://github.com/dotnet/coreclr/issues/2144

[halter73]

We should also be using the LoggingThreadPool instead of QUWI directly. The ThreadPool property in Connection which would catch and log this instead of crashing the server.

_frame?.Abort() is safe, but unnecessary if we only call abort in the ConnectionState.Open state.

[benaadams]

My mistake is in api review https://github.com/dotnet/corefx/issues/6398

[Eilon]

Double approved by me too, I'll track it in my Ask Mode notes.