Don't reset frame state when connection is aborted (#1103).

aspnet · Sep 28, 2016 · 09fda74 · 09fda74
1 parent 73656f6
commit 09fda74
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 3 deletions.
diff --git a/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs b/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs
@@ -141,8 +141,11 @@ public override async Task RequestProcessingAsync()
                         }
                     }
 
-                    // Don't lose request rejection state
-                    if (!_requestRejected)
+                    // Don't reset frame state if we're exiting the loop. This avoids losing request rejection
+                    // information (for 4xx response), and prevents ObjectDisposedException on HTTPS (ODEs
+                    // will be thrown if PrepareRequest is not null and references objects disposed on connection
+                    // close - see https://github.com/aspnet/KestrelHttpServer/issues/1103#issuecomment-250237677).
+                    if (!_requestProcessingStopping)
                     {
                         Reset();
                     }

diff --git a/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/HttpsTests.cs b/test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/HttpsTests.cs
@@ -2,12 +2,19 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Net.Security;
 using System.Net.Sockets;
+using System.Security.Authentication;
+using System.Security.Cryptography.X509Certificates;
+using System.Text;
 using System.Threading.Tasks;
+using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Hosting;
+using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Server.Kestrel.Https;
 using Microsoft.AspNetCore.Testing;
 using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Abstractions.Internal;
 using Xunit;
 
 namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
@@ -78,6 +85,57 @@ public async Task ClientHandshakeFailureLoggedAsInformation()
             Assert.Equal(0, loggerFactory.ErrorLogger.TotalErrorsLogged);
         }
 
+        // Regression test for https://github.com/aspnet/KestrelHttpServer/issues/1103#issuecomment-246971172
+        [Fact]
+        public async Task DoesNotThrowObjectDisposedExceptionOnConnectionAbort()
+        {
+            var x509Certificate2 = new X509Certificate2(@"TestResources/testCert.pfx", "testPassword");
+            var loggerFactory = new HandshakeErrorLoggerFactory();
+            var hostBuilder = new WebHostBuilder()
+                .UseKestrel(options =>
+                {
+                    options.UseHttps(@"TestResources/testCert.pfx", "testPassword");
+                })
+                .UseUrls("https://127.0.0.1:0/")
+                .UseLoggerFactory(loggerFactory)
+                .Configure(app => app.Run(async httpContext =>
+                {
+                    var ct = httpContext.RequestAborted;
+                    while (!ct.IsCancellationRequested)
+                    {
+                        try
+                        {
+                            await httpContext.Response.WriteAsync($"hello, world\r\r", ct);
+                            await Task.Delay(1000, ct);
+                        }
+                        catch (TaskCanceledException)
+                        {
+                            // Don't regard connection abort as an error
+                        }
+                    }
+                }));
+
+            using (var host = hostBuilder.Build())
+            {
+                host.Start();
+
+                using (var socket = await HttpClientSlim.GetSocket(new Uri($"https://127.0.0.1:{host.GetPort()}/")))
+                using (var stream = new NetworkStream(socket, ownsSocket: false))
+                using (var sslStream = new SslStream(stream, true, (sender, certificate, chain, errors) => true))
+                {
+                    await sslStream.AuthenticateAsClientAsync("127.0.0.1", clientCertificates: null,
+                        enabledSslProtocols: SslProtocols.Tls11 | SslProtocols.Tls12,
+                        checkCertificateRevocation: false);
+
+                    var request = Encoding.ASCII.GetBytes("GET / HTTP/1.1\r\n\r\n");
+                    await sslStream.WriteAsync(request, 0, request.Length);
+                    await sslStream.ReadAsync(new byte[32], 0, 32);
+                }
+            }
+
+            Assert.False(loggerFactory.ErrorLogger.ObjectDisposedExceptionLogged);
+        }
+
         private class HandshakeErrorLoggerFactory : ILoggerFactory
         {
             public HttpsConnectionFilterLogger FilterLogger { get; } = new HttpsConnectionFilterLogger();
@@ -133,12 +191,19 @@ private class ApplicationErrorLogger : ILogger
         {
             public int TotalErrorsLogged { get; set; }
 
+            public bool ObjectDisposedExceptionLogged { get; set; }
+
             public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter)
             {
                 if (logLevel == LogLevel.Error)
                 {
                     TotalErrorsLogged++;
                 }
+
+                if (exception is ObjectDisposedException)
+                {
+                    ObjectDisposedExceptionLogged = true;
+                }
             }
 
             public bool IsEnabled(LogLevel logLevel)
@@ -148,7 +213,7 @@ public bool IsEnabled(LogLevel logLevel)
 
             public IDisposable BeginScope<TState>(TState state)
             {
-                throw new NotImplementedException();
+                return NullScope.Instance;
             }
         }
     }