From f1058f8575eeaa5caea688195565811207b19cab Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Thu, 22 Mar 2018 11:13:06 -0700 Subject: [PATCH 1/2] Cleanup ReadAsync calls, make IISHttpContext private (#696) --- .../Server/IISHttpContext.ReadWrite.cs | 24 ++---- .../Server/IISHttpRequestBody.cs | 4 +- .../Server/IISHttpResponseBody.cs | 5 -- .../InvalidReadWriteOperationTests.cs | 33 +++++++- test/IISTestSite/Startup.cs | 82 ++++++++++++------- 5 files changed, 90 insertions(+), 58 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.ReadWrite.cs b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.ReadWrite.cs index 5329239167..699081e659 100644 --- a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.ReadWrite.cs +++ b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.ReadWrite.cs @@ -17,25 +17,11 @@ namespace Microsoft.AspNetCore.Server.IISIntegration /// /// Reads data from the Input pipe to the user. /// - /// - /// - /// + /// /// /// - public async Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) + internal async Task ReadAsync(Memory memory, CancellationToken cancellationToken) { - // Start a task which will continuously call ReadFromIISAsync and WriteToIISAsync - if (buffer == null) - { - throw new ArgumentNullException(nameof(buffer)); - } - if (count == 0) - { - throw new ArgumentOutOfRangeException(nameof(count)); - } - - var memory = new Memory(buffer, offset, count); - StartProcessingRequestAndResponseBody(); while (true) @@ -46,7 +32,7 @@ namespace Microsoft.AspNetCore.Server.IISIntegration { if (!readableBuffer.IsEmpty) { - var actual = Math.Min(readableBuffer.Length, count); + var actual = Math.Min(readableBuffer.Length, memory.Length); readableBuffer = readableBuffer.Slice(0, actual); readableBuffer.CopyTo(memory.Span); return (int)actual; @@ -69,7 +55,7 @@ namespace Microsoft.AspNetCore.Server.IISIntegration /// /// /// - public Task WriteAsync(ReadOnlyMemory memory, CancellationToken cancellationToken = default(CancellationToken)) + internal Task WriteAsync(ReadOnlyMemory memory, CancellationToken cancellationToken = default(CancellationToken)) { // Want to keep exceptions consistent, @@ -90,7 +76,7 @@ namespace Microsoft.AspNetCore.Server.IISIntegration /// /// /// - public Task FlushAsync(CancellationToken cancellationToken = default(CancellationToken)) + internal Task FlushAsync(CancellationToken cancellationToken = default(CancellationToken)) { if (!_hasResponseStarted) { diff --git a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpRequestBody.cs b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpRequestBody.cs index 0a51d9fc7a..be5746c0d3 100644 --- a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpRequestBody.cs +++ b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpRequestBody.cs @@ -39,7 +39,9 @@ namespace Microsoft.AspNetCore.Server.IISIntegration public override unsafe Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) { - return _httpContext.ReadAsync(buffer, offset, count, cancellationToken); + var memory = new Memory(buffer, offset, count); + + return _httpContext.ReadAsync(memory, cancellationToken); } public override long Seek(long offset, SeekOrigin origin) diff --git a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpResponseBody.cs b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpResponseBody.cs index 38c8e64c2a..f850994a4a 100644 --- a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpResponseBody.cs +++ b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpResponseBody.cs @@ -54,11 +54,6 @@ namespace Microsoft.AspNetCore.Server.IISIntegration public override unsafe Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) { - if (buffer == null) - { - throw new ArgumentNullException(nameof(buffer)); - } - return _httpContext.WriteAsync(new ReadOnlyMemory(buffer, offset, count), cancellationToken); } diff --git a/test/IISIntegration.FunctionalTests/Inprocess/InvalidReadWriteOperationTests.cs b/test/IISIntegration.FunctionalTests/Inprocess/InvalidReadWriteOperationTests.cs index 524bc2931f..95c05308bd 100644 --- a/test/IISIntegration.FunctionalTests/Inprocess/InvalidReadWriteOperationTests.cs +++ b/test/IISIntegration.FunctionalTests/Inprocess/InvalidReadWriteOperationTests.cs @@ -26,13 +26,11 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests } [ConditionalTheory] - [InlineData("/NullBuffer")] [InlineData("/InvalidOffsetSmall")] [InlineData("/InvalidOffsetLarge")] [InlineData("/InvalidCountSmall")] [InlineData("/InvalidCountLarge")] [InlineData("/InvalidCountWithOffset")] - [InlineData("/InvalidCountZeroRead")] public async Task TestInvalidReadOperations(string operation) { var result = await _fixture.Client.GetStringAsync($"/TestInvalidReadOperations{operation}"); @@ -41,6 +39,23 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests [ConditionalTheory] [InlineData("/NullBuffer")] + [InlineData("/InvalidCountZeroRead")] + public async Task TestValidReadOperations(string operation) + { + var result = await _fixture.Client.GetStringAsync($"/TestValidReadOperations{operation}"); + Assert.Equal("Success", result); + } + + [ConditionalTheory] + [InlineData("/NullBufferPost")] + [InlineData("/InvalidCountZeroReadPost")] + public async Task TestValidReadOperationsPost(string operation) + { + var result = await _fixture.Client.PostAsync($"/TestValidReadOperations{operation}", new StringContent("hello")); + Assert.Equal("Success", await result.Content.ReadAsStringAsync()); + } + + [ConditionalTheory] [InlineData("/InvalidOffsetSmall")] [InlineData("/InvalidOffsetLarge")] [InlineData("/InvalidCountSmall")] @@ -51,5 +66,19 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests var result = await _fixture.Client.GetStringAsync($"/TestInvalidWriteOperations{operation}"); Assert.Equal("Success", result); } + + [ConditionalFact] + public async Task TestValidWriteOperations() + { + var result = await _fixture.Client.GetStringAsync($"/TestValidWriteOperations/NullBuffer"); + Assert.Equal("Success", result); + } + + [ConditionalFact] + public async Task TestValidWriteOperationsPost() + { + var result = await _fixture.Client.PostAsync($"/TestValidWriteOperations/NullBufferPost", new StringContent("hello")); + Assert.Equal("Success", await result.Content.ReadAsStringAsync()); + } } } diff --git a/test/IISTestSite/Startup.cs b/test/IISTestSite/Startup.cs index dd035b01a5..24e7e5e457 100644 --- a/test/IISTestSite/Startup.cs +++ b/test/IISTestSite/Startup.cs @@ -47,7 +47,9 @@ namespace IISTestSite app.Map("/WebsocketRequest", WebsocketRequest); app.Map("/UpgradeFeatureDetection", UpgradeFeatureDetection); app.Map("/TestInvalidReadOperations", TestInvalidReadOperations); + app.Map("/TestValidReadOperations", TestValidReadOperations); app.Map("/TestInvalidWriteOperations", TestInvalidWriteOperations); + app.Map("/TestValidWriteOperations", TestValidWriteOperations); app.Map("/TestReadOffsetWorks", TestReadOffsetWorks); app.Map("/LargeResponseFile", LargeResponseFile); } @@ -471,10 +473,6 @@ namespace IISTestSite { await context.Request.Body.ReadAsync(null, 0, 0); } - catch (ArgumentNullException) - { - success = true; - } catch (Exception) { success = true; @@ -535,42 +533,46 @@ namespace IISTestSite success = true; } } - else if (context.Request.Path.StartsWithSegments("/InvalidCountZeroRead")) - { - try - { - await context.Request.Body.ReadAsync(new byte[1], 0, 0); - } - catch (ArgumentOutOfRangeException) - { - success = true; - } - catch (Exception) - { - success = true; - } - } + await context.Response.WriteAsync(success ? "Success" : "Failure"); }); } + + private void TestValidReadOperations(IApplicationBuilder app) + { + app.Run(async context => + { + var count = -1; + + if (context.Request.Path.StartsWithSegments("/NullBuffer")) + { + count = await context.Request.Body.ReadAsync(null, 0, 0); + } + else if (context.Request.Path.StartsWithSegments("/NullBufferPost")) + { + count = await context.Request.Body.ReadAsync(null, 0, 0); + } + else if (context.Request.Path.StartsWithSegments("/InvalidCountZeroRead")) + { + count = await context.Request.Body.ReadAsync(new byte[1], 0, 0); + } + else if (context.Request.Path.StartsWithSegments("/InvalidCountZeroReadPost")) + { + count = await context.Request.Body.ReadAsync(new byte[1], 0, 0); + } + + await context.Response.WriteAsync(count == 0 ? "Success" : "Failure"); + }); + } + private void TestInvalidWriteOperations(IApplicationBuilder app) { app.Run(async context => { var success = false; - if (context.Request.Path.StartsWithSegments("/NullBuffer")) - { - try - { - await context.Response.Body.WriteAsync(null, 0, 0); - } - catch (ArgumentNullException) - { - success = true; - } - } - else if (context.Request.Path.StartsWithSegments("/InvalidOffsetSmall")) + + if (context.Request.Path.StartsWithSegments("/InvalidOffsetSmall")) { try { @@ -630,6 +632,24 @@ namespace IISTestSite }); } + private void TestValidWriteOperations(IApplicationBuilder app) + { + app.Run(async context => + { + + if (context.Request.Path.StartsWithSegments("/NullBuffer")) + { + await context.Response.Body.WriteAsync(null, 0, 0); + } + else if (context.Request.Path.StartsWithSegments("/NullBufferPost")) + { + await context.Response.Body.WriteAsync(null, 0, 0); + } + + await context.Response.WriteAsync("Success"); + }); + } + private void LargeResponseFile(IApplicationBuilder app) { app.Run(async ctx => From ed801f0e882b5e32b93856cf465db0f52ce54807 Mon Sep 17 00:00:00 2001 From: Pavel Krymets Date: Thu, 22 Mar 2018 12:21:10 -0700 Subject: [PATCH 2/2] Do not return 200 for exceptions --- .../Properties/launchSettings.json | 1 + .../NativeMethods.cs | 4 +- .../Server/IISHttpContext.cs | 11 ++-- src/RequestHandler/managedexports.cxx | 6 +- .../IISIntegration.FunctionalTests.csproj | 1 + .../Inprocess/ResponseHeaderTests.cs | 20 +++++++ test/IISTestSite/Startup.cs | 55 +++++++++---------- 7 files changed, 58 insertions(+), 40 deletions(-) diff --git a/samples/NativeIISSample/Properties/launchSettings.json b/samples/NativeIISSample/Properties/launchSettings.json index 7d09a120ab..6d5ce43f73 100644 --- a/samples/NativeIISSample/Properties/launchSettings.json +++ b/samples/NativeIISSample/Properties/launchSettings.json @@ -12,6 +12,7 @@ "commandName": "Executable", "executablePath": "$(IISExpressPath)", "commandLineArgs": "$(IISExpressArguments)", + "nativeDebugging": true, "environmentVariables": { "IIS_SITE_PATH": "$(MSBuildThisFileDirectory)", "ANCM_PATH": "$(TargetDir)$(AncmPath)", diff --git a/src/Microsoft.AspNetCore.Server.IISIntegration/NativeMethods.cs b/src/Microsoft.AspNetCore.Server.IISIntegration/NativeMethods.cs index 8cfd0b256f..00c9d5beb1 100644 --- a/src/Microsoft.AspNetCore.Server.IISIntegration/NativeMethods.cs +++ b/src/Microsoft.AspNetCore.Server.IISIntegration/NativeMethods.cs @@ -68,8 +68,8 @@ namespace Microsoft.AspNetCore.Server.IISIntegration [DllImport(AspNetCoreModuleDll)] internal unsafe static extern HttpApiTypes.HTTP_RESPONSE_V2* http_get_raw_response(IntPtr pInProcessHandler); - [DllImport(AspNetCoreModuleDll)] - public unsafe static extern void http_set_response_status_code(IntPtr pInProcessHandler, ushort statusCode, byte* pszReason); + [DllImport(AspNetCoreModuleDll, CharSet = CharSet.Ansi)] + public unsafe static extern int http_set_response_status_code(IntPtr pInProcessHandler, ushort statusCode, string pszReason); [DllImport(AspNetCoreModuleDll)] public unsafe static extern int http_read_request_bytes(IntPtr pInProcessHandler, byte* pvBuffer, int cbBuffer, out int dwBytesReceived, out bool fCompletionExpected); diff --git a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs index f6cbe57255..32afb07033 100644 --- a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs +++ b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs @@ -223,7 +223,7 @@ namespace Microsoft.AspNetCore.Server.IISIntegration _reasonPhrase = value; } } - + internal IISHttpServer Server { get { return _server; } @@ -332,13 +332,10 @@ namespace Microsoft.AspNetCore.Server.IISIntegration public unsafe void SendResponseHeaders(bool appCompleted) { // Verifies we have sent the statuscode before writing a header - var reasonPhraseBytes = Encoding.UTF8.GetBytes(ReasonPhrase ?? ReasonPhrases.GetReasonPhrase(StatusCode)); + var reasonPhrase = string.IsNullOrEmpty(ReasonPhrase) ? ReasonPhrases.GetReasonPhrase(StatusCode) : ReasonPhrase; - fixed (byte* pReasonPhrase = reasonPhraseBytes) - { - // This copies data into the underlying buffer - NativeMethods.http_set_response_status_code(_pInProcessHandler, (ushort)StatusCode, pReasonPhrase); - } + // This copies data into the underlying buffer + NativeMethods.http_set_response_status_code(_pInProcessHandler, (ushort)StatusCode, reasonPhrase); HttpResponseHeaders.IsReadOnly = true; foreach (var headerPair in HttpResponseHeaders) diff --git a/src/RequestHandler/managedexports.cxx b/src/RequestHandler/managedexports.cxx index 80672a6667..46510a00b7 100644 --- a/src/RequestHandler/managedexports.cxx +++ b/src/RequestHandler/managedexports.cxx @@ -74,13 +74,15 @@ Finished: return hr; } -EXTERN_C __MIDL_DECLSPEC_DLLEXPORT VOID http_set_response_status_code( +EXTERN_C __MIDL_DECLSPEC_DLLEXPORT +HRESULT +http_set_response_status_code( _In_ IN_PROCESS_HANDLER* pInProcessHandler, _In_ USHORT statusCode, _In_ PCSTR pszReason ) { - pInProcessHandler->QueryHttpContext()->GetResponse()->SetStatus(statusCode, pszReason); + return pInProcessHandler->QueryHttpContext()->GetResponse()->SetStatus(statusCode, pszReason); } EXTERN_C __MIDL_DECLSPEC_DLLEXPORT diff --git a/test/IISIntegration.FunctionalTests/IISIntegration.FunctionalTests.csproj b/test/IISIntegration.FunctionalTests/IISIntegration.FunctionalTests.csproj index 77f3646a70..c09eeee095 100644 --- a/test/IISIntegration.FunctionalTests/IISIntegration.FunctionalTests.csproj +++ b/test/IISIntegration.FunctionalTests/IISIntegration.FunctionalTests.csproj @@ -10,6 +10,7 @@ + diff --git a/test/IISIntegration.FunctionalTests/Inprocess/ResponseHeaderTests.cs b/test/IISIntegration.FunctionalTests/Inprocess/ResponseHeaderTests.cs index 0ab845f1d4..47ca0dc808 100644 --- a/test/IISIntegration.FunctionalTests/Inprocess/ResponseHeaderTests.cs +++ b/test/IISIntegration.FunctionalTests/Inprocess/ResponseHeaderTests.cs @@ -40,5 +40,25 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests Assert.Equal("1", headerValues.First()); Assert.Equal("2", headerValues.Last()); } + + [ConditionalFact] + public async Task ErrorCodeIsSetForExceptionDuringRequest() + { + var response = await _fixture.Client.GetAsync("Throw"); + Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode); + Assert.Equal("Internal Server Error", response.ReasonPhrase); + } + + [ConditionalTheory] + [InlineData(200, "custom", "custom")] + [InlineData(500, "", "Internal Server Error")] + [InlineData(999, "", "")] + public async Task CustomErrorCodeWorks(int code, string reason, string expectedReason) + { + var response = await _fixture.Client.GetAsync($"SetCustomErorCode?code={code}&reason={reason}"); + Assert.Equal((HttpStatusCode)code, response.StatusCode); + Assert.Equal(expectedReason, response.ReasonPhrase); + Assert.Equal("Body", await response.Content.ReadAsStringAsync()); + } } } diff --git a/test/IISTestSite/Startup.cs b/test/IISTestSite/Startup.cs index 24e7e5e457..00b83e769d 100644 --- a/test/IISTestSite/Startup.cs +++ b/test/IISTestSite/Startup.cs @@ -5,6 +5,7 @@ using System; using System.Diagnostics; using System.IO; using System.Net; +using System.Reflection; using System.Text; using System.Threading; using System.Threading.Tasks; @@ -23,35 +24,16 @@ namespace IISTestSite { public void Configure(IApplicationBuilder app) { - app.Map("/ServerVariable", ServerVariable); - app.Map("/AuthenticationAnonymous", AuthenticationAnonymous); - app.Map("/AuthenticationRestricted", AuthenticationRestricted); - app.Map("/AuthenticationForbidden", AuthenticationForbidden); - app.Map("/AuthenticationRestrictedNTLM", AuthenticationRestrictedNTLM); - app.Map("/FeatureCollectionSetRequestFeatures", FeatureCollectionSetRequestFeatures); - app.Map("/FeatureCollectionSetResponseFeatures", FeatureCollectionSetResponseFeatures); - app.Map("/FeatureCollectionSetConnectionFeatures", FeatureCollectionSetConnectionFeatures); - app.Map("/HelloWorld", HelloWorld); - app.Map("/LargeResponseBody", LargeResponseBody); - app.Map("/ResponseHeaders", ResponseHeaders); - app.Map("/ResponseInvalidOrdering", ResponseInvalidOrdering); - app.Map("/CheckEnvironmentVariable", CheckEnvironmentVariable); - app.Map("/CheckEnvironmentLongValueVariable", CheckEnvironmentLongValueVariable); - app.Map("/CheckAppendedEnvironmentVariable", CheckAppendedEnvironmentVariable); - app.Map("/CheckRemoveAuthEnvironmentVariable", CheckRemoveAuthEnvironmentVariable); - app.Map("/ReadAndWriteSynchronously", ReadAndWriteSynchronously); - app.Map("/ReadAndWriteEcho", ReadAndWriteEcho); - app.Map("/ReadAndWriteCopyToAsync", ReadAndWriteCopyToAsync); - app.Map("/ReadAndWriteEchoTwice", ReadAndWriteEchoTwice); - app.Map("/ReadAndWriteSlowConnection", ReadAndWriteSlowConnection); - app.Map("/WebsocketRequest", WebsocketRequest); - app.Map("/UpgradeFeatureDetection", UpgradeFeatureDetection); - app.Map("/TestInvalidReadOperations", TestInvalidReadOperations); - app.Map("/TestValidReadOperations", TestValidReadOperations); - app.Map("/TestInvalidWriteOperations", TestInvalidWriteOperations); - app.Map("/TestValidWriteOperations", TestValidWriteOperations); - app.Map("/TestReadOffsetWorks", TestReadOffsetWorks); - app.Map("/LargeResponseFile", LargeResponseFile); + foreach (var method in GetType().GetMethods(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance)) + { + var parameters = method.GetParameters(); + if (method.Name != nameof(Configure) && + parameters.Length == 1 && + parameters[0].ParameterType == typeof(IApplicationBuilder)) + { + app.Map("/" + method.Name, innerAppBuilder => method.Invoke(this, new[] { innerAppBuilder })); + } + } } private void ServerVariable(IApplicationBuilder app) @@ -231,6 +213,21 @@ namespace IISTestSite }); } + private void Throw(IApplicationBuilder app) + { + app.Run(ctx => { throw new Exception(); }); + } + + private void SetCustomErorCode(IApplicationBuilder app) + { + app.Run(async ctx => { + var feature = ctx.Features.Get(); + feature.ReasonPhrase = ctx.Request.Query["reason"]; + feature.StatusCode = int.Parse(ctx.Request.Query["code"]); + await ctx.Response.WriteAsync("Body"); + }); + } + private void HelloWorld(IApplicationBuilder app) { app.Run(async ctx =>