feat: Add logics to escape MSBuildArgument text representation when contains MSBuild special chars

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<Argument>
+    public abstract class Argument : IEquatable<Argument>
     {
         [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;
+        }
+
+        /// <summary>
+        /// Gets the MSBuild argument that is used for build script.
+        /// </summary>
+        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}\"
+                    """;
+        }
     }
-}
+}

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<MsBuildArgument>();
 
-            return string.Join(" ", msBuildArguments.Select(arg => arg.TextRepresentation));
+            return string.Join(" ", msBuildArguments.Select(arg => arg.GetEscapedTextRepresentation()));
         }
 
         private static IEnumerable<string> GetNuGetAddPackageCommands(BenchmarkCase benchmarkCase, IResolver resolver)

tests/BenchmarkDotNet.IntegrationTests.ManualRunning/BenchmarkDotNet.IntegrationTests.ManualRunning.csproj

@@ -15,7 +15,15 @@
   <PropertyGroup Condition="'$(CustomProp)'=='true'">
     <DefineConstants>$(DefineConstants);CUSTOM_PROP</DefineConstants>
   </PropertyGroup>
-
+
+  <!-- Set AssemblyMetadataAttribute that is used by MsBuildArgumentTests-->
+  <ItemGroup Condition="'$(CustomProp)' != ''">
+    <AssemblyAttribute Include="System.Reflection.AssemblyMetadataAttribute">
+      <_Parameter1>CustomProp</_Parameter1>
+      <_Parameter2>$(CustomProp)</_Parameter2>
+    </AssemblyAttribute>
+  </ItemGroup>
+
   <ItemGroup>
     <Compile Include="..\BenchmarkDotNet.IntegrationTests\BenchmarkTestExecutor.cs" Link="BenchmarkTestExecutor.cs" />
     <Compile Include="..\BenchmarkDotNet.IntegrationTests\Xunit\MisconfiguredEnvironmentException.cs" Link="MisconfiguredEnvironmentException.cs" />
@@ -41,4 +49,5 @@
       <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
     </PackageReference>
   </ItemGroup>
+
 </Project>

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<ValidateEscapedValueBenchmark>(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<AssemblyMetadataAttribute>().Single(x => x.Key == "CustomProp").Value;
+
+                if (result != expected)
+                    throw new Exception($"Failed to run benchmark. Expected:{expected} Actual : {result}");
+            }
+        }
     }
-}
+}

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);
+        }
+    }
+}