Fix two string allocation issues (#1539)

src/AspNetCoreModuleV2/CommonLib/ServerErrorHandler.h

@@ -63,13 +63,15 @@ private:
         {
             HRSRC rc = nullptr;
             HGLOBAL rcData = nullptr;
-            const char* data = nullptr;
+            std::string data;
+            const char* pTempData = nullptr;
 
             THROW_LAST_ERROR_IF_NULL(rc = FindResource(module, MAKEINTRESOURCE(page), RT_HTML));
             THROW_LAST_ERROR_IF_NULL(rcData = LoadResource(module, rc));
             auto const size = SizeofResource(module, rc);
             THROW_LAST_ERROR_IF(size == 0);
-            THROW_LAST_ERROR_IF_NULL(data = static_cast<const char*>(LockResource(rcData)));
+            THROW_LAST_ERROR_IF_NULL(pTempData = static_cast<const char*>(LockResource(rcData)));
+            data = std::string(pTempData, size);
 
             auto additionalErrorLink = Environment::GetEnvironmentVariableValue(L"ANCM_ADDITIONAL_ERROR_PAGE_LINK");
             std::string additionalHtml;

src/AspNetCoreModuleV2/CommonLib/StringHelpers.h

@@ -22,8 +22,8 @@ std::wstring format(const std::wstring& format, Args ... args)
     if (!format.empty())
     {
         const size_t size = swprintf(nullptr, 0, format.c_str(), args ...); // Extra char for '\0'
-        result.resize(size + 1);
+        result.resize(size);
-        if (swprintf(result.data(), result.size(), format.c_str(), args ... ) == -1)
+        if (swprintf(result.data(), result.size() + 1, format.c_str(), args ... ) == -1)
         {
             throw std::system_error(std::error_code(errno, std::system_category()));
         }
@@ -39,8 +39,8 @@ std::string format(const std::string& format, Args ... args)
     if (!format.empty())
     {
         const size_t size = snprintf(nullptr, 0, format.c_str(), args ...); // Extra char for '\0'
-        result.resize(size + 1);
+        result.resize(size);
-        if (snprintf(result.data(), result.size(), format.c_str(), args ... ) == -1)
+        if (snprintf(result.data(), result.size() + 1, format.c_str(), args ... ) == -1)
         {
             throw std::system_error(std::error_code(errno, std::system_category()));
         }

test/Common.FunctionalTests/Inprocess/ErrorPagesTests.cs

@@ -24,7 +24,7 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests
 
         [ConditionalFact]
         [RequiresIIS(IISCapability.PoolEnvironmentVariables)]
-        public async Task IncludesAdditionalErrorPageTextInProcessHandlerLoadFailure()
+        public async Task IncludesAdditionalErrorPageTextInProcessHandlerLoadFailure_CorrectString()
         {
             var deploymentParameters = _fixture.GetBaseDeploymentParameters(publish: true);
             var response = await DeployAppWithStartupFailure(deploymentParameters);
@@ -33,13 +33,16 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests
 
             StopServer();
 
-            Assert.Contains("HTTP Error 500.0 - ANCM In-Process Handler Load Failure", await response.Content.ReadAsStringAsync());
+            var responseString = await response.Content.ReadAsStringAsync();
+            Assert.Contains("HTTP Error 500.0 - ANCM In-Process Handler Load Failure", responseString);
+            VerifyNoExtraTrailingBytes(responseString);
+
             await AssertLink(response);
         }
 
         [ConditionalFact]
         [RequiresIIS(IISCapability.PoolEnvironmentVariables)]
-        public async Task IncludesAdditionalErrorPageTextOutOfProcessStartupFailure()
+        public async Task IncludesAdditionalErrorPageTextOutOfProcessStartupFailure_CorrectString()
         {
             var deploymentParameters = _fixture.GetBaseDeploymentParameters(HostingModel.OutOfProcess, publish: true);
             var response = await DeployAppWithStartupFailure(deploymentParameters);
@@ -48,13 +51,16 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests
 
             StopServer();
 
-            Assert.Contains("HTTP Error 502.5 - ANCM Out-Of-Process Startup Failure", await response.Content.ReadAsStringAsync());
+            var responseString = await response.Content.ReadAsStringAsync();
+            Assert.Contains("HTTP Error 502.5 - ANCM Out-Of-Process Startup Failure", responseString);
+            VerifyNoExtraTrailingBytes(responseString);
+
             await AssertLink(response);
         }
 
         [ConditionalFact]
         [RequiresIIS(IISCapability.PoolEnvironmentVariables)]
-        public async Task IncludesAdditionalErrorPageTextOutOfProcessHandlerLoadFailure()
+        public async Task IncludesAdditionalErrorPageTextOutOfProcessHandlerLoadFailure_CorrectString()
         {
             var deploymentParameters = _fixture.GetBaseDeploymentParameters(HostingModel.OutOfProcess, publish: true);
             deploymentParameters.HandlerSettings["handlerVersion"] = "88.93";
@@ -67,13 +73,16 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests
 
             StopServer();
 
-            Assert.Contains("HTTP Error 500.0 - ANCM Out-Of-Process Handler Load Failure", await response.Content.ReadAsStringAsync());
+            var responseString = await response.Content.ReadAsStringAsync();
+            Assert.Contains("HTTP Error 500.0 - ANCM Out-Of-Process Handler Load Failure", responseString);
+            VerifyNoExtraTrailingBytes(responseString);
+
             await AssertLink(response);
         }
 
         [ConditionalFact]
         [RequiresIIS(IISCapability.PoolEnvironmentVariables)]
-        public async Task IncludesAdditionalErrorPageTextInProcessStartupFailure()
+        public async Task IncludesAdditionalErrorPageTextInProcessStartupFailure_CorrectString()
         {
             var deploymentParameters = _fixture.GetBaseDeploymentParameters(publish: true);
             deploymentParameters.TransformArguments((a, _) => $"{a} EarlyReturn");
@@ -86,10 +95,21 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests
 
             StopServer();
 
-            Assert.Contains("HTTP Error 500.30 - ANCM In-Process Start Failure", await response.Content.ReadAsStringAsync());
+            var responseString = await response.Content.ReadAsStringAsync();
+            Assert.Contains("HTTP Error 500.30 - ANCM In-Process Start Failure", responseString);
+            VerifyNoExtraTrailingBytes(responseString);
+
             await AssertLink(response);
         }
 
+        private static void VerifyNoExtraTrailingBytes(string responseString)
+        {
+            if (!DeployerSelector.IsBackwardsCompatiblityTest)
+            {
+                Assert.EndsWith("</html>\r\n", responseString);
+            }
+        }
+
         private static async Task AssertLink(HttpResponseMessage response)
         {
             Assert.Contains("<a href=\"http://example\"> <cite> http://example </cite></a> and ", await response.Content.ReadAsStringAsync());

test/CommonLibTests/utility_tests.cpp

@@ -3,6 +3,7 @@
 
 #include "stdafx.h"
 #include "Environment.h"
+#include "StringHelpers.h"
 
 TEST(PassUnexpandedEnvString, ExpandsResult)
 {
@@ -58,3 +59,17 @@ TEST(GetEnvironmentVariableValue, ReturnsNulloptWhenNotFound)
     auto result = Environment::GetEnvironmentVariableValue(L"RANDOM_ENV_VAR_2");
     EXPECT_FALSE(result.has_value());
 }
+
+TEST(CheckStringHelpers, FormatWithoutContentDoesNotIncreaseSizeString)
+{
+    std::string testString = "test";
+    auto result = format(testString);
+    EXPECT_EQ(testString.size(), result.size());
+}
+
+TEST(CheckStringHelpers, FormatWithoutContentDoesNotIncreaseSizeWstring)
+{
+    std::wstring testString = L"test";
+    auto result = format(testString);
+    EXPECT_EQ(testString.size(), result.size());
+}

test/IIS.BackwardsCompatibility.FunctionalTests/DeployerSelector.cs

@@ -8,5 +8,6 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests
     public static class DeployerSelector
     {
         public static ServerType ServerType => ServerType.IIS;
+        public static bool IsBackwardsCompatiblityTest => true;
     }
 }

test/IIS.FunctionalTests/DeployerSelector.cs

@@ -0,0 +1,13 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using Microsoft.AspNetCore.Server.IntegrationTesting;
+
+namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests
+{
+    public static class DeployerSelector
+    {
+        public static ServerType ServerType => ServerType.IIS;
+        public static bool IsBackwardsCompatiblityTest => false;
+    }
+}

test/IISExpress.FunctionalTests/DeployerSelector.cs

@@ -8,5 +8,6 @@ namespace Microsoft.AspNetCore.Server.IISIntegration.FunctionalTests
     public static class DeployerSelector
     {
         public static ServerType ServerType => ServerType.IISExpress;
+        public static bool IsBackwardsCompatiblityTest => false;
     }
 }