From f61f23526898805d958b1edcf6fcc03cc3cf817a Mon Sep 17 00:00:00 2001 From: Gal Date: Wed, 14 May 2025 10:29:46 +0300 Subject: [PATCH 1/3] Fix: escape MSBuild special characters in MsBuildArgument with unit test --- src/BenchmarkDotNet/Jobs/Argument.cs | 40 ++++++++++++++++++- .../MsBuildArgumentTests.cs | 7 ++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/src/BenchmarkDotNet/Jobs/Argument.cs b/src/BenchmarkDotNet/Jobs/Argument.cs index c86c83906a..22098a3b05 100644 --- a/src/BenchmarkDotNet/Jobs/Argument.cs +++ b/src/BenchmarkDotNet/Jobs/Argument.cs @@ -1,9 +1,11 @@ using System; +using System.Collections.Generic; +using System.Text; using JetBrains.Annotations; namespace BenchmarkDotNet.Jobs { - public abstract class Argument: IEquatable + public abstract class Argument : IEquatable { [PublicAPI] public string TextRepresentation { get; } @@ -47,6 +49,40 @@ public MonoArgument(string value) : base(value) [PublicAPI] public class MsBuildArgument : Argument { - public MsBuildArgument(string value) : base(value) { } + private static readonly Dictionary MsBuildEscapes = new () + { + { '%', "%25" }, + { '$', "%24" }, + { '@', "%40" }, + { '\'', "%27" }, + { '(', "%28" }, + { ')', "%29" }, + { ';', "%3B" }, + { '?', "%3F" }, + { '*', "%2A" } + }; + + private static string EscapeMsBuildSpecialChars(string value) + { + if (string.IsNullOrEmpty(value)) + return value; + var sb = new StringBuilder(value.Length); + + foreach (char c in value) + { + if (MsBuildEscapes.TryGetValue(c, out string escaped)) + { + sb.Append(escaped); + } + else + { + sb.Append(c); + } + } + + return sb.ToString(); + } + + public MsBuildArgument(string value) : base(EscapeMsBuildSpecialChars(value)) { } } } \ No newline at end of file diff --git a/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs b/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs index f4619040cc..0ab2ee9e45 100644 --- a/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs +++ b/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs @@ -43,6 +43,13 @@ public void MultipleProcessesAreBuiltWithCorrectProperties() CanExecute(config); } + [Fact] + public void EscapesSemicolonInDefineConstants() + { + var arg = new MsBuildArgument("/p:DefineConstants=TEST1;TEST2"); + Assert.Equal("/p:DefineConstants=TEST1%3BTEST2", arg.ToString()); + } + public class PropertyDefine { private const bool customPropWasSet = From 37a02caab529b08550c761603c71690d69628268 Mon Sep 17 00:00:00 2001 From: Gal Date: Wed, 14 May 2025 12:43:24 +0300 Subject: [PATCH 2/3] Add test for percent escaping and avoid double-escape for known patterns --- src/BenchmarkDotNet/Jobs/Argument.cs | 27 +++++++++++++++++-- .../MsBuildArgumentTests.cs | 14 ++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/BenchmarkDotNet/Jobs/Argument.cs b/src/BenchmarkDotNet/Jobs/Argument.cs index 22098a3b05..e3eede2914 100644 --- a/src/BenchmarkDotNet/Jobs/Argument.cs +++ b/src/BenchmarkDotNet/Jobs/Argument.cs @@ -62,15 +62,36 @@ public class MsBuildArgument : Argument { '*', "%2A" } }; + // This lets us check if a %xx is a known escape + private static readonly HashSet KnownEscapeValues = [.. MsBuildEscapes.Values]; + private static string EscapeMsBuildSpecialChars(string value) { if (string.IsNullOrEmpty(value)) return value; + var sb = new StringBuilder(value.Length); + int i = 0; - foreach (char c in value) + while (i < value.Length) { - if (MsBuildEscapes.TryGetValue(c, out string escaped)) + char c = value[i]; + + // Check for a known %xx escape + if (c == '%' && i + 2 < value.Length) + { + string candidate = value.Substring(i, 3); // e.g., "%3B" + + if (KnownEscapeValues.Contains(candidate)) + { + sb.Append(candidate); + i += 3; + continue; + } + } + + // Escape only if it's a special MSBuild character + if (MsBuildEscapes.TryGetValue(c, out var escaped)) { sb.Append(escaped); } @@ -78,6 +99,8 @@ private static string EscapeMsBuildSpecialChars(string value) { sb.Append(c); } + + i++; } return sb.ToString(); diff --git a/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs b/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs index 0ab2ee9e45..0afeaa78bf 100644 --- a/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs +++ b/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs @@ -50,6 +50,20 @@ public void EscapesSemicolonInDefineConstants() Assert.Equal("/p:DefineConstants=TEST1%3BTEST2", arg.ToString()); } + [Fact] + public void EscapesPercentSign() + { + var arg = new MsBuildArgument("/p:SomeValue=100%"); + Assert.Equal("/p:SomeValue=100%25", arg.ToString()); + } + + [Fact] + public void DoesNotDoubleEscapeAlreadyEscapedPercent() + { + var arg = new MsBuildArgument("/p:SomeValue=100%25"); + Assert.Equal("/p:SomeValue=100%25", arg.ToString()); + } + public class PropertyDefine { private const bool customPropWasSet = From 5c6f4e3fb43b91ff74d46e222e29205fc4072501 Mon Sep 17 00:00:00 2001 From: Gal Date: Wed, 14 May 2025 23:00:32 +0300 Subject: [PATCH 3/3] Feature: add escapeSpecialChars param to MsBuildArgument constructor --- src/BenchmarkDotNet/Jobs/Argument.cs | 40 +++++-------------- .../MsBuildArgumentTests.cs | 2 +- 2 files changed, 11 insertions(+), 31 deletions(-) diff --git a/src/BenchmarkDotNet/Jobs/Argument.cs b/src/BenchmarkDotNet/Jobs/Argument.cs index e3eede2914..c451f4d6d3 100644 --- a/src/BenchmarkDotNet/Jobs/Argument.cs +++ b/src/BenchmarkDotNet/Jobs/Argument.cs @@ -62,50 +62,30 @@ public class MsBuildArgument : Argument { '*', "%2A" } }; - // This lets us check if a %xx is a known escape - private static readonly HashSet KnownEscapeValues = [.. MsBuildEscapes.Values]; - private static string EscapeMsBuildSpecialChars(string value) { if (string.IsNullOrEmpty(value)) return value; var sb = new StringBuilder(value.Length); - int i = 0; - - while (i < value.Length) + foreach (char c in value) { - char c = value[i]; - - // Check for a known %xx escape - if (c == '%' && i + 2 < value.Length) - { - string candidate = value.Substring(i, 3); // e.g., "%3B" - - if (KnownEscapeValues.Contains(candidate)) - { - sb.Append(candidate); - i += 3; - continue; - } - } - - // Escape only if it's a special MSBuild character if (MsBuildEscapes.TryGetValue(c, out var escaped)) - { sb.Append(escaped); - } else - { sb.Append(c); - } - - i++; } return sb.ToString(); } - - public MsBuildArgument(string value) : base(EscapeMsBuildSpecialChars(value)) { } + /// + /// Represents an MSBuild command-line argument. + /// The raw or escaped argument value (e.g., "/p:DefineConstants=TEST1;TEST2"). + /// + /// If true (default), special MSBuild characters like %, ;, *, etc. will be escaped. + /// If false, the value is used as-is — use this only if you're passing a fully pre-escaped string. + /// + /// + public MsBuildArgument(string value, bool escapeSpecialChars = true) : base(escapeSpecialChars ? EscapeMsBuildSpecialChars(value) : value) { } } } \ No newline at end of file diff --git a/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs b/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs index 0afeaa78bf..a9f333d84b 100644 --- a/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs +++ b/tests/BenchmarkDotNet.IntegrationTests.ManualRunning/MsBuildArgumentTests.cs @@ -60,7 +60,7 @@ public void EscapesPercentSign() [Fact] public void DoesNotDoubleEscapeAlreadyEscapedPercent() { - var arg = new MsBuildArgument("/p:SomeValue=100%25"); + var arg = new MsBuildArgument("/p:SomeValue=100%25", false); Assert.Equal("/p:SomeValue=100%25", arg.ToString()); }