From 491004075002814a2f903fb1be94fae7e4fe2f07 Mon Sep 17 00:00:00 2001 From: filzrev <103790468+filzrev@users.noreply.github.com> Date: Thu, 15 May 2025 19:15:39 +0900 Subject: [PATCH 1/2] feat: escape msbuild argument when contains special chars --- src/BenchmarkDotNet/Jobs/Argument.cs | 67 ++++++++++++++++++- .../Toolchains/DotNetCli/DotNetCliCommand.cs | 2 +- .../Jobs/MsBuildArgumentTests.cs | 55 +++++++++++++++ 3 files changed, 122 insertions(+), 2 deletions(-) create mode 100644 tests/BenchmarkDotNet.Tests/Jobs/MsBuildArgumentTests.cs diff --git a/src/BenchmarkDotNet/Jobs/Argument.cs b/src/BenchmarkDotNet/Jobs/Argument.cs index c86c83906a..be329a4f40 100644 --- a/src/BenchmarkDotNet/Jobs/Argument.cs +++ b/src/BenchmarkDotNet/Jobs/Argument.cs @@ -47,6 +47,71 @@ public MonoArgument(string value) : base(value) [PublicAPI] public class MsBuildArgument : Argument { + // Characters that need to be escaped. + // 1. Space + // 2. Comma (Special char that is used for separater for value of `-property:{name}={value}` and `-restoreProperty:{name}={value}`) + // 3. Other MSBuild special chars (https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-special-characters?view=vs-2022) + private static readonly char[] MSBuildCharsToEscape = [' ', ',', '%', '$', '@', '\'', '(', ')', ';', '?', '*']; + public MsBuildArgument(string value) : base(value) { } + + /// + /// Gets the MSBuild argument that is used for build script. + /// + internal string GetEscapedTextRepresentation() + { + var originalArgument = TextRepresentation; + + // If entire argument surrounded with double quote, returns original argument. + // In this case. MSBuild special chars must be escaped by user. https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-special-characters + if (originalArgument.StartsWith("\"")) + return originalArgument; + + // Process MSBuildArgument that contains '=' char. (e.g. `--property:{key}={value}` and `-restoreProperty:{key}={value}`) + // See: https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-command-line-reference?view=vs-2022 + var values = originalArgument.Split(['='], 2); + if (values.Length != 2) + return originalArgument; + + var key = values[0]; + var value = values[1]; + + // If value starts with `\` char. It is expected that the escaped value is specified by the user. + if (value.StartsWith("\\")) + return originalArgument; + + // If value don't contains special chars. return original value. + if (value.IndexOfAny(MSBuildCharsToEscape) < 0) + return originalArgument; + + return $"{key}={GetEscapedValue(value)}"; + } + + private static string GetEscapedValue(string value) + { + // If value starts with double quote. Trim leading/trailing double quote + if (value.StartsWith("\"")) + value = value.Trim(['"']); + + bool isWindows = true; +#if NET + isWindows = OperatingSystem.IsWindows(); +#endif + if (isWindows) + { + // On Windows environment. + // Returns double-quoted value. (Command line execution and `.bat` file requires escape double quote with `\`) + return $""" + \"{value}\" + """; + } + + // On non-Windows environment. + // Returns value that surround with `'"` and `"'`. See: https://github.com/dotnet/sdk/issues/8792#issuecomment-393756980 + // It requires escape with `\` when running command with `.sh` file. ) + return $""" + \'\"{value}\"\' + """; + } } -} \ No newline at end of file +} diff --git a/src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliCommand.cs b/src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliCommand.cs index 81f48b4413..dc6134be3f 100644 --- a/src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliCommand.cs +++ b/src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliCommand.cs @@ -201,7 +201,7 @@ private static string GetCustomMsBuildArguments(BenchmarkCase benchmarkCase, IRe var msBuildArguments = benchmarkCase.Job.ResolveValue(InfrastructureMode.ArgumentsCharacteristic, resolver).OfType(); - return string.Join(" ", msBuildArguments.Select(arg => arg.TextRepresentation)); + return string.Join(" ", msBuildArguments.Select(arg => arg.GetEscapedTextRepresentation())); } private static IEnumerable GetNuGetAddPackageCommands(BenchmarkCase benchmarkCase, IResolver resolver) diff --git a/tests/BenchmarkDotNet.Tests/Jobs/MsBuildArgumentTests.cs b/tests/BenchmarkDotNet.Tests/Jobs/MsBuildArgumentTests.cs new file mode 100644 index 0000000000..e6df2f8ed4 --- /dev/null +++ b/tests/BenchmarkDotNet.Tests/Jobs/MsBuildArgumentTests.cs @@ -0,0 +1,55 @@ +using BenchmarkDotNet.Jobs; +using System; +using Xunit; + +namespace BenchmarkDotNet.Tests +{ + public class MsBuildArgumentTests + { + [Theory] + [InlineData("/p:CustomProperty=100%", "100%")] // Contains percentage + [InlineData("/p:CustomProperty=AAA;BBB", "AAA;BBB")] // Contains semicolon + [InlineData("/p:CustomProperty=AAA,BBB", "AAA,BBB")] // Contains comma + [InlineData("/p:CustomProperty=AAA BBB", "AAA BBB")] // Contains space + [InlineData("/p:CustomProperty=AAA%3BBBB", "AAA%3BBBB")] // Contains escaped semicolon + [InlineData("/p:CustomProperty=\"AAA%3BBBB\"", "AAA%3BBBB")] // Contains escaped semicolon. and surrounded with non-escaped double quote + [InlineData("/p:NoWarn=1591;1573;3001", "1591;1573;3001")] // NoWarn property that contains semicolon + public void MSBuildArgument_ContainsSpecialChars(string argument, string expected) + { + var result = new MsBuildArgument(argument).GetEscapedTextRepresentation(); + AssertEscapedValue(result, expected); + } + + [Theory] + [InlineData("/p:CustomProperty=AAA_BBB")] // Argument that don't contains special chars + [InlineData("\"/p:CustomProperty=AAA%3BBBB\"")] // Argument is surrounded with double quote and value is escaped + [InlineData("/p:CustomProperty=\\\"100%3B200\\\"")] // Value is surrounded with double quote and value is escaped (For Windows environment) + [InlineData("/p:CustomProperty=\\\'\\\"100%3B200\\\"\\\'")] // Value is surrounded with double quote and value is escaped (For non-Windows environment) + [InlineData("/noWarn:1591;1573;3001")] // Other argument should not be escaped + public void MSBuildArgument_ShouldNotBeEscaped(string originalArg) + { + var result = new MsBuildArgument(originalArg).GetEscapedTextRepresentation(); + Assert.Equal(expected: originalArg, result); + } + + private static void AssertEscapedValue(string escapedArgument, string expectedValue) + { + var values = escapedArgument.Split(['='], 2); + Assert.Equal(2, values.Length); + + var key = values[0]; + var value = values[1]; + +#if NET + // On non-Windows environment value should be surrounded with escaped `'"` and `"'` + if (!OperatingSystem.IsWindows()) + { + Assert.Equal("\\\'\\\"" + expectedValue + "\\\"\\\'", value); + return; + } +#endif + // On Windows environment. Value should be quote with escaped `\"` + Assert.Equal("\\\"" + expectedValue + "\\\"", value); + } + } +} From 526699461cbd885ec2827cbe961fd900eebbbe16 Mon Sep 17 00:00:00 2001 From: filzrev <103790468+filzrev@users.noreply.github.com> Date: Tue, 20 May 2025 23:55:37 +0900 Subject: [PATCH 2/2] chore: modify escape logics and update tests --- src/BenchmarkDotNet/Jobs/Argument.cs | 69 ++++++++-------- ...tNet.IntegrationTests.ManualRunning.csproj | 11 ++- .../MsBuildArgumentTests.cs | 79 ++++++++++++++++++- .../Jobs/MsBuildArgumentTests.cs | 74 +++++++++-------- 4 files changed, 159 insertions(+), 74 deletions(-) diff --git a/src/BenchmarkDotNet/Jobs/Argument.cs b/src/BenchmarkDotNet/Jobs/Argument.cs index be329a4f40..26982c8924 100644 --- a/src/BenchmarkDotNet/Jobs/Argument.cs +++ b/src/BenchmarkDotNet/Jobs/Argument.cs @@ -1,9 +1,9 @@ -using System; -using JetBrains.Annotations; +using JetBrains.Annotations; +using System; namespace BenchmarkDotNet.Jobs { - public abstract class Argument: IEquatable + public abstract class Argument : IEquatable { [PublicAPI] public string TextRepresentation { get; } @@ -47,13 +47,17 @@ public MonoArgument(string value) : base(value) [PublicAPI] public class MsBuildArgument : Argument { - // Characters that need to be escaped. - // 1. Space - // 2. Comma (Special char that is used for separater for value of `-property:{name}={value}` and `-restoreProperty:{name}={value}`) - // 3. Other MSBuild special chars (https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-special-characters?view=vs-2022) - private static readonly char[] MSBuildCharsToEscape = [' ', ',', '%', '$', '@', '\'', '(', ')', ';', '?', '*']; + // Specisal chars that need to be wrapped with `\"`. + // 1. Comma char (It's used for separater char for `-property:{name}={value}` and `-restoreProperty:{name}={ value}`) + // 2. MSBuild special chars (https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-special-characters?view=vs-2022) + private static readonly char[] MSBuildSpecialChars = [',', '%', '$', '@', '\'', '(', ')', ';', '?', '*']; - public MsBuildArgument(string value) : base(value) { } + private readonly bool escapeArgument; + + public MsBuildArgument(string value, bool escape = false) : base(value) + { + escapeArgument = escape; + } /// /// Gets the MSBuild argument that is used for build script. @@ -62,6 +66,9 @@ internal string GetEscapedTextRepresentation() { var originalArgument = TextRepresentation; + if (!escapeArgument) + return originalArgument; + // If entire argument surrounded with double quote, returns original argument. // In this case. MSBuild special chars must be escaped by user. https://learn.microsoft.com/en-us/visualstudio/msbuild/msbuild-special-characters if (originalArgument.StartsWith("\"")) @@ -76,41 +83,27 @@ internal string GetEscapedTextRepresentation() var key = values[0]; var value = values[1]; - // If value starts with `\` char. It is expected that the escaped value is specified by the user. - if (value.StartsWith("\\")) + // If value starts with `\"`. + // It is expected that the escaped value is specified by the user. + if (value.StartsWith("\\\"")) return originalArgument; - // If value don't contains special chars. return original value. - if (value.IndexOfAny(MSBuildCharsToEscape) < 0) - return originalArgument; + // If value is wrapped with double quote. Trim leading/trailing double quote. + if (value.StartsWith("\"") && value.EndsWith("\"")) + value = value.Trim(['"']); - return $"{key}={GetEscapedValue(value)}"; - } + // Escape chars that need to escaped when wrapped with escaped double quote (`\"`) + value = value.Replace(" ", "%20") // Space + .Replace("\"", "%22") // Double Quote + .Replace("\\", "%5C"); // BackSlash - private static string GetEscapedValue(string value) - { - // If value starts with double quote. Trim leading/trailing double quote - if (value.StartsWith("\"")) - value = value.Trim(['"']); + // If escaped value doesn't contains MSBuild special char, return original argument. + if (value.IndexOfAny(MSBuildSpecialChars) < 0) + return originalArgument; - bool isWindows = true; -#if NET - isWindows = OperatingSystem.IsWindows(); -#endif - if (isWindows) - { - // On Windows environment. - // Returns double-quoted value. (Command line execution and `.bat` file requires escape double quote with `\`) - return $""" - \"{value}\" - """; - } - - // On non-Windows environment. - // Returns value that surround with `'"` and `"'`. See: https://github.com/dotnet/sdk/issues/8792#issuecomment-393756980 - // It requires escape with `\` when running command with `.sh` file. ) + // Return escaped value that is wrapped with escaped double quote (`\"`) return $""" - \'\"{value}\"\' + {key}=\"{value}\" """; } } diff --git a/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/BenchmarkDotNet.IntegrationTests.ManualRunning.csproj b/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/BenchmarkDotNet.IntegrationTests.ManualRunning.csproj index 4d8d86600f..4dbec52d5f 100755 --- a/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/BenchmarkDotNet.IntegrationTests.ManualRunning.csproj +++ b/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/BenchmarkDotNet.IntegrationTests.ManualRunning.csproj @@ -15,7 +15,15 @@ $(DefineConstants);CUSTOM_PROP - + + + + + <_Parameter1>CustomProp + <_Parameter2>$(CustomProp) + + + @@ -41,4 +49,5 @@ runtime; build; native; contentfiles; analyzers; buildtransitive + \ No newline at end of file diff --git a/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs b/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs index f4619040cc..088476cca5 100644 --- a/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs +++ b/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs @@ -1,8 +1,11 @@ -using System; -using BenchmarkDotNet.Attributes; +using BenchmarkDotNet.Attributes; using BenchmarkDotNet.Configs; +using BenchmarkDotNet.Engines; using BenchmarkDotNet.Environments; using BenchmarkDotNet.Jobs; +using System; +using System.Linq; +using System.Reflection; using Xunit; namespace BenchmarkDotNet.IntegrationTests.ManualRunning @@ -61,5 +64,75 @@ public void ThrowWhenWrong() } } } + + private const string AsciiChars = " !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~"; + + [Theory] + [InlineData("AAA;BBB")] // Contains separator char (`;`) + [InlineData(AsciiChars)] // Validate all ASCII char patterns. + // Following tests are commented out by default. Because it takes time to execute. + //[InlineData("AAA;BBB,CCC")] // Validate argument that contains semicolon/comma separators. + //[InlineData("AAA BBB")] // Contains space char + //[InlineData("AAA\"BBB")] // Contains double quote char + //[InlineData("AAA\\BBB")] // Contains backslash char + //[InlineData("\"AAA")] // StartsWith `"` but not ends with `"` + //[InlineData("AAA\"")] // EndsWith `"` but not ends with `"` + //[InlineData("\"AAA;BBB\"", "AAA;BBB")] // Surrounded with `"` + //[InlineData("\\\"AAA%3BBBB\\\"", "AAA;BBB")] // Surrounded with `\"` and escaped `;` + public void ValidateEscapedMsBuildArgument(string propertyValue, string? expectedValue = null) + { + // Arrange + expectedValue ??= propertyValue; + var config = ManualConfig.CreateEmpty() + .WithOption(ConfigOptions.DisableOptimizationsValidator, true) + .AddJob(Job.Dry + .WithStrategy(RunStrategy.Monitoring) + .WithArguments([new MsBuildArgument($"/p:CustomProp={propertyValue}", escape: true)]) + .WithEnvironmentVariable(CustomPropEnvVarName, expectedValue) + ); + + // Act + var summary = CanExecute(config, fullValidation: false); + + // Assert + Assert.False(summary.HasCriticalValidationErrors); + Assert.True(summary.Reports.Any()); + foreach (var report in summary.Reports) + { + if (!report.GenerateResult.IsGenerateSuccess) + { + var message = report.GenerateResult.GenerateException?.ToString(); + Assert.Fail($"Failed to generate benchmark project:{Environment.NewLine}{message}"); + } + + if (!report.BuildResult.IsBuildSuccess) + Assert.Fail($"Failed to build benchmark project:{Environment.NewLine}{report.BuildResult.ErrorMessage}"); + + foreach (var executeResult in report.ExecuteResults) + { + if (!executeResult.IsSuccess) + { + string message = string.Join(Environment.NewLine, executeResult.Results).Trim(); + Assert.Fail($"Failed to run benchmark({report.BenchmarkCase.Descriptor.DisplayInfo}):{Environment.NewLine}{message}"); + } + } + } + } + + public class ValidateEscapedValueBenchmark + { + [Benchmark] + public void Validate() + { + // Gets expected value from environment variable. + var expected = Environment.GetEnvironmentVariable(CustomPropEnvVarName); + + // Gets MSBuild property from AssemblyMetadataAttribute (This attribute is set by csproj) + var result = typeof(MsBuildArgumentTests).Assembly.GetCustomAttributes().Single(x => x.Key == "CustomProp").Value; + + if (result != expected) + throw new Exception($"Failed to run benchmark. Expected:{expected} Actual : {result}"); + } + } } -} \ No newline at end of file +} diff --git a/tests/BenchmarkDotNet.Tests/Jobs/MsBuildArgumentTests.cs b/tests/BenchmarkDotNet.Tests/Jobs/MsBuildArgumentTests.cs index e6df2f8ed4..90a47df1d5 100644 --- a/tests/BenchmarkDotNet.Tests/Jobs/MsBuildArgumentTests.cs +++ b/tests/BenchmarkDotNet.Tests/Jobs/MsBuildArgumentTests.cs @@ -10,46 +10,56 @@ public class MsBuildArgumentTests [InlineData("/p:CustomProperty=100%", "100%")] // Contains percentage [InlineData("/p:CustomProperty=AAA;BBB", "AAA;BBB")] // Contains semicolon [InlineData("/p:CustomProperty=AAA,BBB", "AAA,BBB")] // Contains comma - [InlineData("/p:CustomProperty=AAA BBB", "AAA BBB")] // Contains space - [InlineData("/p:CustomProperty=AAA%3BBBB", "AAA%3BBBB")] // Contains escaped semicolon - [InlineData("/p:CustomProperty=\"AAA%3BBBB\"", "AAA%3BBBB")] // Contains escaped semicolon. and surrounded with non-escaped double quote + [InlineData("/p:CustomProperty=AAA BBB", "AAA%20BBB")] // Contains space (It's escaped to `%20`) + [InlineData("/p:CustomProperty=AAA\"BBB", "AAA%22BBB")] // Contains double quote (It's escaped to `%22B`) + [InlineData("/p:CustomProperty=AAA\\BBB", "AAA%5CBBB")] // Contains backslash (It's escaped to `%5C`) + [InlineData("/p:CustomProperty=AAA%3BBBB", "AAA%3BBBB")] // Contains escaped semicolon (`%3B`) + [InlineData("/p:CustomProperty=\"AAA%3BBBB\"", "AAA%3BBBB")] // Contains escaped semicolon (`%3B`), and surrounded with double quote [InlineData("/p:NoWarn=1591;1573;3001", "1591;1573;3001")] // NoWarn property that contains semicolon - public void MSBuildArgument_ContainsSpecialChars(string argument, string expected) + public void MSBuildArgument_ContainsMSBuildSpecialChars(string argument, string expected) { - var result = new MsBuildArgument(argument).GetEscapedTextRepresentation(); - AssertEscapedValue(result, expected); - } + // Arrange + var msbuildArgument = new MsBuildArgument(argument, escape: true); - [Theory] - [InlineData("/p:CustomProperty=AAA_BBB")] // Argument that don't contains special chars - [InlineData("\"/p:CustomProperty=AAA%3BBBB\"")] // Argument is surrounded with double quote and value is escaped - [InlineData("/p:CustomProperty=\\\"100%3B200\\\"")] // Value is surrounded with double quote and value is escaped (For Windows environment) - [InlineData("/p:CustomProperty=\\\'\\\"100%3B200\\\"\\\'")] // Value is surrounded with double quote and value is escaped (For non-Windows environment) - [InlineData("/noWarn:1591;1573;3001")] // Other argument should not be escaped - public void MSBuildArgument_ShouldNotBeEscaped(string originalArg) - { - var result = new MsBuildArgument(originalArg).GetEscapedTextRepresentation(); - Assert.Equal(expected: originalArg, result); + // Act + var result = msbuildArgument.GetEscapedTextRepresentation(); + + // Assert + AssertEscapedValue(expected, result); + + // Helper method + static void AssertEscapedValue(string expectedValue, string argument) + { + var values = argument.Split(['='], 2); + Assert.Equal(2, values.Length); + + var key = values[0]; + var value = values[1]; + + // Assert value is wrapped with `\"` + Assert.StartsWith("\\\"", value); + Assert.EndsWith("\\\"", value); + + value = value.Substring(2, value.Length - 4); + Assert.Equal(expectedValue, value); + } } - private static void AssertEscapedValue(string escapedArgument, string expectedValue) + [Theory] + [InlineData("/p:CustomProperty=AAA_BBB")] // Argument that don't contains special chars + [InlineData("\"/p:CustomProperty=AAA%3BBBB\"")] // Entire argument is surrounded with double quote and value is escaped + [InlineData("/p:CustomProperty=\\\"100%3B200\\\"")] // Value is surrounded with escaped double quote and value is escaped + [InlineData("/noWarn:1591;1573;3001")] // Other argument that don't contains `=` should not be escaped + public void MSBuildArgument_ShouldNotBeEscaped(string argument) { - var values = escapedArgument.Split(['='], 2); - Assert.Equal(2, values.Length); + // Arrange + var msbuildArgument = new MsBuildArgument(argument, escape: true); - var key = values[0]; - var value = values[1]; + // Act + var result = msbuildArgument.GetEscapedTextRepresentation(); -#if NET - // On non-Windows environment value should be surrounded with escaped `'"` and `"'` - if (!OperatingSystem.IsWindows()) - { - Assert.Equal("\\\'\\\"" + expectedValue + "\\\"\\\'", value); - return; - } -#endif - // On Windows environment. Value should be quote with escaped `\"` - Assert.Equal("\\\"" + expectedValue + "\\\"", value); + // Assert + Assert.Equal(argument, result); } } }