diff --git a/src/BenchmarkDotNet/Jobs/Argument.cs b/src/BenchmarkDotNet/Jobs/Argument.cs index c86c83906a..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,6 +47,64 @@ public MonoArgument(string value) : base(value) [PublicAPI] public class MsBuildArgument : Argument { - public MsBuildArgument(string value) : base(value) { } + // 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 = [',', '%', '$', '@', '\'', '(', ')', ';', '?', '*']; + + 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. + /// + 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("\"")) + 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 `\"`. + // It is expected that the escaped value is specified by the user. + if (value.StartsWith("\\\"")) + return originalArgument; + + // If value is wrapped with double quote. Trim leading/trailing double quote. + if (value.StartsWith("\"") && value.EndsWith("\"")) + value = value.Trim(['"']); + + // Escape chars that need to escaped when wrapped with escaped double quote (`\"`) + value = value.Replace(" ", "%20") // Space + .Replace("\"", "%22") // Double Quote + .Replace("\\", "%5C"); // BackSlash + + // If escaped value doesn't contains MSBuild special char, return original argument. + if (value.IndexOfAny(MSBuildSpecialChars) < 0) + return originalArgument; + + // Return escaped value that is wrapped with escaped double quote (`\"`) + return $""" + {key}=\"{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.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 new file mode 100644 index 0000000000..90a47df1d5 --- /dev/null +++ b/tests/BenchmarkDotNet.Tests/Jobs/MsBuildArgumentTests.cs @@ -0,0 +1,65 @@ +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%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_ContainsMSBuildSpecialChars(string argument, string expected) + { + // Arrange + var msbuildArgument = new MsBuildArgument(argument, escape: true); + + // 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); + } + } + + [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) + { + // Arrange + var msbuildArgument = new MsBuildArgument(argument, escape: true); + + // Act + var result = msbuildArgument.GetEscapedTextRepresentation(); + + // Assert + Assert.Equal(argument, result); + } + } +}