From 205fe9daf1736535f11397ae883df3dfa8f48d93 Mon Sep 17 00:00:00 2001 From: Pavel Krymets Date: Thu, 22 Mar 2018 12:21:10 -0700 Subject: [PATCH 1/4] Do not return 200 for exceptions --- .../Properties/launchSettings.json | 1 + .../Server/IISHttpContext.cs | 4 +- .../Inprocess/ResponseHeaderTests.cs | 8 ++++ test/IISTestSite/Startup.cs | 43 +++++++------------ 4 files changed, 28 insertions(+), 28 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/Server/IISHttpContext.cs b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs index f6cbe57255..e7fe04d9d4 100644 --- a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs +++ b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs @@ -332,10 +332,12 @@ 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.IsNullOrWhiteSpace(ReasonPhrase) ? ReasonPhrases.GetReasonPhrase(StatusCode) : ReasonPhrase; + var reasonPhraseBytes = Encoding.UTF8.GetBytes(reasonPhrase); fixed (byte* pReasonPhrase = reasonPhraseBytes) { + Debug.Assert((IntPtr)pReasonPhrase != IntPtr.Zero); // This copies data into the underlying buffer NativeMethods.http_set_response_status_code(_pInProcessHandler, (ushort)StatusCode, pReasonPhrase); } diff --git a/test/IISIntegration.FunctionalTests/Inprocess/ResponseHeaderTests.cs b/test/IISIntegration.FunctionalTests/Inprocess/ResponseHeaderTests.cs index 0ab845f1d4..8cee6643a4 100644 --- a/test/IISIntegration.FunctionalTests/Inprocess/ResponseHeaderTests.cs +++ b/test/IISIntegration.FunctionalTests/Inprocess/ResponseHeaderTests.cs @@ -40,5 +40,13 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests Assert.Equal("1", headerValues.First()); Assert.Equal("2", headerValues.Last()); } + + [ConditionalFact] + public async Task ErrorCodeIsSetForExceoptionDuringRequest() + { + var response = await _fixture.Client.GetAsync("Throw"); + Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode); + Assert.Equal("Internal Server Error", response.ReasonPhrase); + } } } diff --git a/test/IISTestSite/Startup.cs b/test/IISTestSite/Startup.cs index dd035b01a5..a74524587a 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,33 +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("/TestInvalidWriteOperations", TestInvalidWriteOperations); - 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) @@ -229,6 +213,11 @@ namespace IISTestSite }); } + private void Throw(IApplicationBuilder app) + { + app.Run(ctx => { throw new Exception(); }); + } + private void HelloWorld(IApplicationBuilder app) { app.Run(async ctx => From f6f5e575fb5302df74d4b52d5159a1bf8e4d6c3c Mon Sep 17 00:00:00 2001 From: Pavel Krymets Date: Thu, 22 Mar 2018 13:11:29 -0700 Subject: [PATCH 2/4] Test more things --- .../Server/IISHttpContext.cs | 7 ++++--- .../IISIntegration.FunctionalTests.csproj | 1 + .../Inprocess/ResponseHeaderTests.cs | 14 +++++++++++++- test/IISTestSite/Startup.cs | 10 ++++++++++ 4 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs index e7fe04d9d4..fb7f7cbb2f 100644 --- a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs +++ b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs @@ -332,9 +332,10 @@ namespace Microsoft.AspNetCore.Server.IISIntegration public unsafe void SendResponseHeaders(bool appCompleted) { // Verifies we have sent the statuscode before writing a header - var reasonPhrase = string.IsNullOrWhiteSpace(ReasonPhrase) ? ReasonPhrases.GetReasonPhrase(StatusCode) : ReasonPhrase; - var reasonPhraseBytes = Encoding.UTF8.GetBytes(reasonPhrase); - + var reasonPhrase = string.IsNullOrEmpty(ReasonPhrase) ? ReasonPhrases.GetReasonPhrase(StatusCode) : ReasonPhrase; + var reasonPhraseBytes = new byte [reasonPhrase.Length + 1]; + Encoding.ASCII.GetBytes(reasonPhrase, 0, reasonPhrase.Length, reasonPhraseBytes, 0); + fixed (byte* pReasonPhrase = reasonPhraseBytes) { Debug.Assert((IntPtr)pReasonPhrase != IntPtr.Zero); 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 8cee6643a4..e54920e59d 100644 --- a/test/IISIntegration.FunctionalTests/Inprocess/ResponseHeaderTests.cs +++ b/test/IISIntegration.FunctionalTests/Inprocess/ResponseHeaderTests.cs @@ -40,7 +40,7 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests Assert.Equal("1", headerValues.First()); Assert.Equal("2", headerValues.Last()); } - + [ConditionalFact] public async Task ErrorCodeIsSetForExceoptionDuringRequest() { @@ -48,5 +48,17 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode); Assert.Equal("Internal Server Error", response.ReasonPhrase); } + + [ConditionalTheory] + [InlineData(1, "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 a74524587a..28460cdaf5 100644 --- a/test/IISTestSite/Startup.cs +++ b/test/IISTestSite/Startup.cs @@ -218,6 +218,16 @@ namespace IISTestSite 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 => From e2e5a226c268138463977183233e7f9a125fcda4 Mon Sep 17 00:00:00 2001 From: Pavel Krymets Date: Thu, 22 Mar 2018 13:14:20 -0700 Subject: [PATCH 3/4] Better fix --- .../NativeMethods.cs | 4 ++-- .../Server/IISHttpContext.cs | 12 +++--------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.IISIntegration/NativeMethods.cs b/src/Microsoft.AspNetCore.Server.IISIntegration/NativeMethods.cs index 8cfd0b256f..a2995dd3d0 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 void 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 fb7f7cbb2f..7e04b68edf 100644 --- a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs +++ b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs @@ -333,15 +333,9 @@ namespace Microsoft.AspNetCore.Server.IISIntegration { // Verifies we have sent the statuscode before writing a header var reasonPhrase = string.IsNullOrEmpty(ReasonPhrase) ? ReasonPhrases.GetReasonPhrase(StatusCode) : ReasonPhrase; - var reasonPhraseBytes = new byte [reasonPhrase.Length + 1]; - Encoding.ASCII.GetBytes(reasonPhrase, 0, reasonPhrase.Length, reasonPhraseBytes, 0); - - fixed (byte* pReasonPhrase = reasonPhraseBytes) - { - Debug.Assert((IntPtr)pReasonPhrase != IntPtr.Zero); - // 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) From 1686a710460be0c5af5a32a3e288153c3b4a3217 Mon Sep 17 00:00:00 2001 From: Pavel Krymets Date: Thu, 22 Mar 2018 14:01:00 -0700 Subject: [PATCH 4/4] Return HRESULT --- .../NativeMethods.cs | 2 +- .../Server/IISHttpContext.cs | 2 +- src/RequestHandler/managedexports.cxx | 6 ++++-- .../Inprocess/ResponseHeaderTests.cs | 4 ++-- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.AspNetCore.Server.IISIntegration/NativeMethods.cs b/src/Microsoft.AspNetCore.Server.IISIntegration/NativeMethods.cs index a2995dd3d0..00c9d5beb1 100644 --- a/src/Microsoft.AspNetCore.Server.IISIntegration/NativeMethods.cs +++ b/src/Microsoft.AspNetCore.Server.IISIntegration/NativeMethods.cs @@ -69,7 +69,7 @@ namespace Microsoft.AspNetCore.Server.IISIntegration internal unsafe static extern HttpApiTypes.HTTP_RESPONSE_V2* http_get_raw_response(IntPtr pInProcessHandler); [DllImport(AspNetCoreModuleDll, CharSet = CharSet.Ansi)] - public unsafe static extern void http_set_response_status_code(IntPtr pInProcessHandler, ushort statusCode, string pszReason); + 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 7e04b68edf..7690e8450c 100644 --- a/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs +++ b/src/Microsoft.AspNetCore.Server.IISIntegration/Server/IISHttpContext.cs @@ -336,7 +336,7 @@ namespace Microsoft.AspNetCore.Server.IISIntegration // 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/Inprocess/ResponseHeaderTests.cs b/test/IISIntegration.FunctionalTests/Inprocess/ResponseHeaderTests.cs index e54920e59d..6b9e561e40 100644 --- a/test/IISIntegration.FunctionalTests/Inprocess/ResponseHeaderTests.cs +++ b/test/IISIntegration.FunctionalTests/Inprocess/ResponseHeaderTests.cs @@ -42,7 +42,7 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests } [ConditionalFact] - public async Task ErrorCodeIsSetForExceoptionDuringRequest() + public async Task ErrorCodeIsSetForExceptionDuringRequest() { var response = await _fixture.Client.GetAsync("Throw"); Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode); @@ -50,7 +50,7 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests } [ConditionalTheory] - [InlineData(1, "custom", "custom")] + [InlineData(200, "custom", "custom")] [InlineData(500, "", "Internal Server Error")] [InlineData(999, "", "")] public async Task CustomErrorCodeWorks(int code, string reason, string expectedReason)