From ffa72f5a0cb2a059ace96ac79a6811ffdc37d824 Mon Sep 17 00:00:00 2001 From: Justin Kotalik Date: Mon, 22 Oct 2018 11:23:14 -0700 Subject: [PATCH] Fix two string allocation issues (#1539) --- .../CommonLib/ServerErrorHandler.h | 6 ++-- .../CommonLib/StringHelpers.h | 8 ++--- .../Inprocess/ErrorPagesTests.cs | 36 ++++++++++++++----- test/CommonLibTests/utility_tests.cpp | 15 ++++++++ .../DeployerSelector.cs | 1 + test/IIS.FunctionalTests/DeployerSelector.cs | 13 +++++++ .../DeployerSelector.cs | 1 + 7 files changed, 66 insertions(+), 14 deletions(-) rename test/{IIS.Shared.FunctionalTests => IIS.BackwardsCompatibility.FunctionalTests}/DeployerSelector.cs (86%) create mode 100644 test/IIS.FunctionalTests/DeployerSelector.cs diff --git a/src/AspNetCoreModuleV2/CommonLib/ServerErrorHandler.h b/src/AspNetCoreModuleV2/CommonLib/ServerErrorHandler.h index f6745768a7..69aa0d440a 100644 --- a/src/AspNetCoreModuleV2/CommonLib/ServerErrorHandler.h +++ b/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(LockResource(rcData))); + THROW_LAST_ERROR_IF_NULL(pTempData = static_cast(LockResource(rcData))); + data = std::string(pTempData, size); auto additionalErrorLink = Environment::GetEnvironmentVariableValue(L"ANCM_ADDITIONAL_ERROR_PAGE_LINK"); std::string additionalHtml; diff --git a/src/AspNetCoreModuleV2/CommonLib/StringHelpers.h b/src/AspNetCoreModuleV2/CommonLib/StringHelpers.h index 40b3b821c7..3adc8863a4 100644 --- a/src/AspNetCoreModuleV2/CommonLib/StringHelpers.h +++ b/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); - if (swprintf(result.data(), result.size(), format.c_str(), args ... ) == -1) + result.resize(size); + 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); - if (snprintf(result.data(), result.size(), format.c_str(), args ... ) == -1) + result.resize(size); + if (snprintf(result.data(), result.size() + 1, format.c_str(), args ... ) == -1) { throw std::system_error(std::error_code(errno, std::system_category())); } diff --git a/test/Common.FunctionalTests/Inprocess/ErrorPagesTests.cs b/test/Common.FunctionalTests/Inprocess/ErrorPagesTests.cs index 6b0e0b93b2..936ad852ac 100644 --- a/test/Common.FunctionalTests/Inprocess/ErrorPagesTests.cs +++ b/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("\r\n", responseString); + } + } + private static async Task AssertLink(HttpResponseMessage response) { Assert.Contains(" http://example and ", await response.Content.ReadAsStringAsync()); diff --git a/test/CommonLibTests/utility_tests.cpp b/test/CommonLibTests/utility_tests.cpp index 567e47762b..ee69d79054 100644 --- a/test/CommonLibTests/utility_tests.cpp +++ b/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()); +} diff --git a/test/IIS.Shared.FunctionalTests/DeployerSelector.cs b/test/IIS.BackwardsCompatibility.FunctionalTests/DeployerSelector.cs similarity index 86% rename from test/IIS.Shared.FunctionalTests/DeployerSelector.cs rename to test/IIS.BackwardsCompatibility.FunctionalTests/DeployerSelector.cs index 2032b716b9..b6dc197e8a 100644 --- a/test/IIS.Shared.FunctionalTests/DeployerSelector.cs +++ b/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; } } diff --git a/test/IIS.FunctionalTests/DeployerSelector.cs b/test/IIS.FunctionalTests/DeployerSelector.cs new file mode 100644 index 0000000000..2359de7226 --- /dev/null +++ b/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; + } +} diff --git a/test/IISExpress.FunctionalTests/DeployerSelector.cs b/test/IISExpress.FunctionalTests/DeployerSelector.cs index 5344cbacf0..c21900e0fb 100644 --- a/test/IISExpress.FunctionalTests/DeployerSelector.cs +++ b/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; } }