Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit 232f488

Browse files
hughbeatsushikan
hughbe
authored and
atsushikan
committed
Correctly handle empty strings in params of RemoteExecutorTestBase (#21196)
* Implement argument pasting in RemoteInvoke * Add tests for RemoteInvoke fixes * Update RemoteInvoke tests now that quoting is unecessary * Fix netfx tests The extra parameter is empty in netfx so was pasted to "" * Address PR feedback
1 parent 26abbf0 commit 232f488

File tree

15 files changed

+209
-113
lines changed

15 files changed

+209
-113
lines changed

‎src/Common/src/System/PasteArguments.cs‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System.Collections.Generic;
6+
using System.Text;
7+
8+
namespace System
9+
{
10+
internal static class PasteArguments
11+
{
12+
/// <summary>
13+
/// Repastes a set of arguments into a linear string that parses back into the originals under pre- or post-2008 VC parsing rules.
14+
/// The rules for parsing the executable name (argv[0]) are special, so you must indicate whether the first argument actually is argv[0].
15+
/// </summary>
16+
public static string Paste(IEnumerable<string> arguments, bool pasteFirstArgumentUsingArgV0Rules)
17+
{
18+
var stringBuilder = new StringBuilder();
19+
20+
foreach (string argument in arguments)
21+
{
22+
if (pasteFirstArgumentUsingArgV0Rules)
23+
{
24+
pasteFirstArgumentUsingArgV0Rules = false;
25+
26+
// Special rules for argv[0]
27+
// - Backslash is a normal character.
28+
// - Quotes used to include whitespace characters.
29+
// - Parsing ends at first whitespace outside quoted region.
30+
// - No way to get a literal quote past the parser.
31+
32+
bool hasWhitespace = false;
33+
foreach (char c in argument)
34+
{
35+
if (c == Quote)
36+
{
37+
throw new ApplicationException("The argv[0] argument cannot include a double quote.");
38+
}
39+
if (char.IsWhiteSpace(c))
40+
{
41+
hasWhitespace = true;
42+
}
43+
}
44+
if (argument.Length == 0 || hasWhitespace)
45+
{
46+
stringBuilder.Append(Quote);
47+
stringBuilder.Append(argument);
48+
stringBuilder.Append(Quote);
49+
}
50+
else
51+
{
52+
stringBuilder.Append(argument);
53+
}
54+
}
55+
else
56+
{
57+
if (stringBuilder.Length != 0)
58+
{
59+
stringBuilder.Append(' ');
60+
}
61+
62+
// Parsing rules for non-argv[0] arguments:
63+
// - Backslash is a normal character except followed by a quote.
64+
// - 2N backslashes followed by a quote ==> N literal backslashes followed by unescaped quote
65+
// - 2N+1 backslashes followed by a quote ==> N literal backslashes followed by a literal quote
66+
// - Parsing stops at first whitespace outside of quoted region.
67+
// - (post 2008 rule): A closing quote followed by another quote ==> literal quote, and parsing remains in quoting mode.
68+
if (argument.Length != 0 && ContainsNoWhitespaceOrQuotes(argument))
69+
{
70+
// Simple case - no quoting or changes needed.
71+
stringBuilder.Append(argument);
72+
}
73+
else
74+
{
75+
stringBuilder.Append(Quote);
76+
int idx = 0;
77+
while (idx < argument.Length)
78+
{
79+
char c = argument[idx++];
80+
if (c == Backslash)
81+
{
82+
int numBackSlash = 1;
83+
while (idx < argument.Length && argument[idx] == Backslash)
84+
{
85+
idx++;
86+
numBackSlash++;
87+
}
88+
if (idx == argument.Length)
89+
{
90+
// We'll emit an end quote after this so must double the number of backslashes.
91+
stringBuilder.Append(Backslash, numBackSlash * 2);
92+
}
93+
else if (argument[idx] == Quote)
94+
{
95+
// Backslashes will be followed by a quote. Must double the number of backslashes.
96+
stringBuilder.Append(Backslash, numBackSlash * 2 + 1);
97+
stringBuilder.Append(Quote);
98+
idx++;
99+
}
100+
else
101+
{
102+
// Backslash will not be followed by a quote, so emit as normal characters.
103+
stringBuilder.Append(Backslash, numBackSlash);
104+
}
105+
continue;
106+
}
107+
if (c == Quote)
108+
{
109+
// Escape the quote so it appears as a literal. This also guarantees that we won't end up generating a closing quote followed
110+
// by another quote (which parses differently pre-2008 vs. post-2008.)
111+
stringBuilder.Append(Backslash);
112+
stringBuilder.Append(Quote);
113+
continue;
114+
}
115+
stringBuilder.Append(c);
116+
}
117+
stringBuilder.Append(Quote);
118+
}
119+
}
120+
}
121+
122+
return stringBuilder.ToString();
123+
}
124+
125+
private static bool ContainsNoWhitespaceOrQuotes(string s)
126+
{
127+
for (int i = 0; i < s.Length; i++)
128+
{
129+
char c = s[i];
130+
if (char.IsWhiteSpace(c) || c == Quote)
131+
{
132+
return false;
133+
}
134+
}
135+
136+
return true;
137+
}
138+
139+
private const char Quote = '\"';
140+
private const char Backslash = '\\';
141+
}
142+
}

‎src/CoreFx.Private.TestUtilities/src/CoreFx.Private.TestUtilities.csproj‎

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,8 @@
66
<ProjectGuid>{5B7EAEC-93CB-40DF-BE40-A60BC189B737}</ProjectGuid>
77
<RuntimeProjectFile>$(ProjectDir)\external\test-runtime\XUnit.Runtime.depproj</RuntimeProjectFile>
88
<ClsCompliant>false</ClsCompliant>
9-
<ShouldWriteSigningRequired>false</ShouldWriteSigningRequired>
9+
<ShouldWriteSigningRequired>false</ShouldWriteSigningRequired>
1010
</PropertyGroup>
11-
1211
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Unix-Debug|AnyCPU'" />
1312
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Unix-Release|AnyCPU'" />
1413
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netcoreapp-Windows_NT-Debug|AnyCPU'" />
@@ -19,17 +18,18 @@
1918
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'uapaot-Windows_NT-Release|AnyCPU'" />
2019
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netfx-Windows_NT-Debug|AnyCPU'" />
2120
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'netfx-Windows_NT-Release|AnyCPU'" />
22-
2321
<ItemGroup>
2422
<Compile Include="System\IO\FileCleanupTestBase.cs" />
2523
<Compile Include="System\Diagnostics\RemoteExecutorTestBase.cs" />
26-
<Compile Condition="'$(TargetGroup)' == 'netcoreapp'" Include="System\Diagnostics\RemoteExecutorTestBase.netcore.cs" />
27-
<Compile Condition="'$(TargetGroup)' == 'netfx'" Include="System\Diagnostics\RemoteExecutorTestBase.netfx.cs" />
28-
<Compile Condition="'$(TargetGroup)' != 'uap'" Include="System\Diagnostics\RemoteExecutorTestBase.Process.cs" />
29-
<Compile Condition="'$(TargetGroup)' == 'uap'" Include="System\Diagnostics\RemoteExecutorTestBase.uap.cs" />
30-
<Compile Condition="'$(TargetGroup)' == 'uapaot'" Include="System\Diagnostics\RemoteExecutorTestBase.uapaot.cs" />
24+
<Compile Condition="'$(TargetGroup)' == 'netcoreapp'" Include="System\Diagnostics\RemoteExecutorTestBase.netcore.cs" />
25+
<Compile Condition="'$(TargetGroup)' == 'netfx'" Include="System\Diagnostics\RemoteExecutorTestBase.netfx.cs" />
26+
<Compile Condition="'$(TargetGroup)' != 'uap'" Include="System\Diagnostics\RemoteExecutorTestBase.Process.cs" />
27+
<Compile Condition="'$(TargetGroup)' == 'uap'" Include="System\Diagnostics\RemoteExecutorTestBase.uap.cs" />
28+
<Compile Condition="'$(TargetGroup)' == 'uapaot'" Include="System\Diagnostics\RemoteExecutorTestBase.uapaot.cs" />
29+
<Compile Include="$(CommonPath)\System\PasteArguments.cs">
30+
<Link>Common\System\PasteArguments.cs</Link>
31+
</Compile>
3132
</ItemGroup>
32-
3333
<ItemGroup>
3434
<Reference Include="System.Runtime" />
3535
<Reference Include="System.IO.FileSystem" />
@@ -38,22 +38,18 @@
3838
<Reference Include="System.Diagnostics.Process" />
3939
<Reference Include="System.ComponentModel.Primitives" />
4040
<Reference Include="System.Runtime.InteropServices.RuntimeInformation" />
41-
4241
<ReferenceFromRuntime Include="xunit.core" />
4342
<ReferenceFromRuntime Include="Xunit.NetCore.Extensions" />
4443
<ReferenceFromRuntime Include="xunit.assert" />
4544
</ItemGroup>
46-
4745
<ItemGroup Condition="'$(TargetGroup)' == 'uap'">
4846
<Reference Include="Windows" />
4947
<Reference Include="mscorlib" />
5048
<Reference Include="System.Runtime.WindowsRuntime" />
5149
</ItemGroup>
52-
5350
<ItemGroup Condition="'$(TargetGroup)' == 'netfx'">
5451
<Reference Include="mscorlib" />
5552
<Reference Include="System" />
5653
</ItemGroup>
57-
5854
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
5955
</Project>

‎src/CoreFx.Private.TestUtilities/src/System/Diagnostics/RemoteExecutorTestBase.Process.cs‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,9 @@
33
// See the LICENSE file in the project root for more information.
44

55
using System.IO;
6+
using System.Linq;
67
using System.Reflection;
7-
using System.Runtime.InteropServices;
8+
using System.Text;
89
using System.Threading.Tasks;
910
using Xunit;
1011

@@ -18,7 +19,8 @@ public abstract partial class RemoteExecutorTestBase : FileCleanupTestBase
1819
/// <param name="args">The arguments to pass to the method.</param>
1920
/// <param name="start">true if this function should Start the Process; false if that responsibility is left up to the caller.</param>
2021
/// <param name="psi">The ProcessStartInfo to use, or null for a default.</param>
21-
private static RemoteInvokeHandle RemoteInvoke(MethodInfo method, string[] args, RemoteInvokeOptions options)
22+
/// <param name="pasteArguments">true if this function should paste the arguments (e.g. surrounding with quotes); false if that reponsibility is left up to the caller.</param>
23+
private static RemoteInvokeHandle RemoteInvoke(MethodInfo method, string[] args, RemoteInvokeOptions options, bool pasteArguments = true)
2224
{
2325
options = options ?? new RemoteInvokeOptions();
2426

@@ -49,13 +51,15 @@ private static RemoteInvokeHandle RemoteInvoke(MethodInfo method, string[] args,
4951
}
5052

5153
// If we need the host (if it exists), use it, otherwise target the console app directly.
52-
string testConsoleAppArgs = "\"" + a.FullName + "\" " + t.FullName + " " + method.Name + " " + string.Join(" ", args);
54+
string metadataArgs = PasteArguments.Paste(new string[] { a.FullName, t.FullName, method.Name }, pasteFirstArgumentUsingArgV0Rules: false);
55+
string passedArgs = pasteArguments ? PasteArguments.Paste(args, pasteFirstArgumentUsingArgV0Rules: false) : string.Join(" ", args);
56+
string testConsoleAppArgs = ExtraParameter + " " + metadataArgs + " " + passedArgs;
5357

5458
if (!File.Exists(TestConsoleApp))
5559
throw new IOException("RemoteExecutorConsoleApp test app isn't present in the test runtime directory.");
5660

5761
psi.FileName = HostRunner;
58-
psi.Arguments = ExtraParameter + testConsoleAppArgs;
62+
psi.Arguments = testConsoleAppArgs;
5963

6064
// Return the handle to the process, which may or not be started
6165
return new RemoteInvokeHandle(options.Start ?

‎src/CoreFx.Private.TestUtilities/src/System/Diagnostics/RemoteExecutorTestBase.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -113,14 +113,14 @@ public static RemoteInvokeHandle RemoteInvoke(
113113
return RemoteInvoke(GetMethodInfo(method), new[] { arg1, arg2, arg3, arg4, arg5 }, options);
114114
}
115115

116-
/// <summary>Invokes the method from this assembly in another process using the specified arguments.</summary>
116+
/// <summary>Invokes the method from this assembly in another process using the specified arguments without performing any modifications to the arguments.</summary>
117117
/// <param name="method">The method to invoke.</param>
118118
/// <param name="args">The arguments to pass to the method.</param>
119119
/// <param name="options">Options to use for the invocation.</param>
120120
public static RemoteInvokeHandle RemoteInvokeRaw(Delegate method, string unparsedArg,
121121
RemoteInvokeOptions options = null)
122122
{
123-
return RemoteInvoke(GetMethodInfo(method), new[] { unparsedArg }, options);
123+
return RemoteInvoke(GetMethodInfo(method), new[] { unparsedArg }, options, pasteArguments: false);
124124
}
125125

126126
private static MethodInfo GetMethodInfo(Delegate d)
@@ -193,7 +193,7 @@ public sealed class RemoteInvokeOptions
193193
public bool Start { get; set; } = true;
194194
public ProcessStartInfo StartInfo { get; set; } = new ProcessStartInfo();
195195
public bool EnableProfiling { get; set; } = true;
196-
public bool CheckExitCode {get; set; } = true;
196+
public bool CheckExitCode { get; set; } = true;
197197

198198
public int TimeOut {get; set; } = RemoteExecutorTestBase.FailWaitTimeoutMilliseconds;
199199
}

‎src/CoreFx.Private.TestUtilities/src/System/Diagnostics/RemoteExecutorTestBase.netcore.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,6 @@ public abstract partial class RemoteExecutorTestBase : FileCleanupTestBase
1313
protected static readonly string HostRunnerName = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "dotnet.exe" : "dotnet";
1414
protected static readonly string HostRunner = Process.GetCurrentProcess().MainModule.FileName;
1515

16-
private static readonly string ExtraParameter = TestConsoleApp + " " ;
16+
private static readonly string ExtraParameter = TestConsoleApp;
1717
}
1818
}

‎src/CoreFx.Private.TestUtilities/src/System/Diagnostics/RemoteExecutorTestBase.uap.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ public abstract partial class RemoteExecutorTestBase : FileCleanupTestBase
2323
/// <param name="args">The arguments to pass to the method.</param>
2424
/// <param name="start">true if this function should Start the Process; false if that responsibility is left up to the caller.</param>
2525
/// <param name="psi">The ProcessStartInfo to use, or null for a default.</param>
26-
private static RemoteInvokeHandle RemoteInvoke(MethodInfo method, string[] args, RemoteInvokeOptions options)
26+
/// <param name="pasteArguments">Unused in UAP.</param>
27+
private static RemoteInvokeHandle RemoteInvoke(MethodInfo method, string[] args, RemoteInvokeOptions options, bool pasteArguments = false)
2728
{
2829
options = options ?? new RemoteInvokeOptions();
2930

‎src/System.Data.SqlClient/tests/FunctionalTests/DiagnosticTest.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@ namespace System.Data.SqlClient.Tests
2121
public class DiagnosticTest : RemoteExecutorTestBase
2222
{
2323
private const string BadConnectionString = "data source = bad; initial catalog = bad; uid = bad; password = bad; connection timeout = 1;";
24-
private static readonly string s_tcpConnStr = $"\"{Environment.GetEnvironmentVariable("TEST_TCP_CONN_STR")}\"";
24+
private static readonly string s_tcpConnStr = Environment.GetEnvironmentVariable("TEST_TCP_CONN_STR") ?? string.Empty;
2525

26-
public static bool IsConnectionStringConfigured() => s_tcpConnStr != "\"\"";
26+
public static bool IsConnectionStringConfigured() => s_tcpConnStr != string.Empty;
2727

2828
[Fact]
2929
[ActiveIssue("dotnet/corefx #17925", TargetFrameworkMonikers.NetFramework)]

‎src/System.Diagnostics.Process/tests/ProcessTests.cs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -966,9 +966,12 @@ public void StartInfo_GetOnRunningProcess_ThrowsInvalidOperationException()
966966
[InlineData("b d \\\"\\\"a\\\"\\\"", "b,d,\"\"a\"\"")]
967967
public void TestArgumentParsing(string inputArguments, string expectedArgv)
968968
{
969-
using (var handle = RemoteInvokeRaw((Func<string, string, string, int>)ConcatThreeArguments,
970-
inputArguments,
971-
new RemoteInvokeOptions { Start = true, StartInfo = new ProcessStartInfo { RedirectStandardOutput = true } }))
969+
var options = new RemoteInvokeOptions
970+
{
971+
Start = true,
972+
StartInfo = new ProcessStartInfo { RedirectStandardOutput = true }
973+
};
974+
using (RemoteInvokeHandle handle = RemoteInvokeRaw((Func<string, string, string, int>)ConcatThreeArguments, inputArguments, options))
972975
{
973976
Assert.Equal(expectedArgv, handle.Process.StandardOutput.ReadToEnd());
974977
}

‎src/System.Globalization/tests/NumberFormatInfo/NumberFormatInfoCurrentInfo.cs‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,11 @@ public void CurrentInfo_CustomCulture(CultureInfo newCurrentCulture)
2626

2727
RemoteInvoke((cultureName) =>
2828
{
29-
if (cultureName.Equals("EmptyString"))
30-
cultureName = string.Empty;
3129
CultureInfo newCulture = new CultureInfo(cultureName);
3230
CultureInfo.CurrentCulture = newCulture;
3331
Assert.Same(newCulture.NumberFormat, NumberFormatInfo.CurrentInfo);
3432
return SuccessExitCode;
35-
}, newCurrentCulture.Name.Length > 0 ? newCurrentCulture.Name : "EmptyString").Dispose();
33+
}, newCurrentCulture.Name).Dispose();
3634
}
3735

3836
[Fact]

0 commit comments

Comments
 (0)