[FastDeploy2] fix adb push argument quoting on Windows

src/Xamarin.Android.Build.Debugging.Tasks/Tasks/FastDeploy2.Adb.cs

@@ -1,7 +1,6 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
-using System.Text;
 using System.Threading.Tasks;
 
 namespace Xamarin.Android.Tasks
@@ -180,25 +179,6 @@ static string GetInstallErrorCode (InstallResultKind kind)
 				_ => "ADB0010",
 			};
 		}
-
-		/// <summary>Quotes an argument for <see cref="System.Diagnostics.ProcessStartInfo.Arguments"/>.</summary>
-		static string QuoteProcessArgument (string argument)
-		{
-			if (argument == null) {
-				return "\"\"";
-			}
-			var sb = new StringBuilder ();
-			sb.Append ('"');
-			// The .NET process class only supports quoted arguments with escaped quotes/backslashes.
-			foreach (char c in argument) {
-				if (c == '"' || c == '\\') {
-					sb.Append ('\\');
-				}
-				sb.Append (c);
-			}
-			sb.Append ('"');
-			return sb.ToString ();
-		}
 	}
 
 	/// <summary>

src/Xamarin.Android.Build.Debugging.Tasks/Tasks/FastDeploy2.cs

@@ -10,6 +10,7 @@
 using Microsoft.Android.Build.Tasks;
 using Microsoft.Build.Framework;
 using Xamarin.Android.Build.Debugging.Tasks.Properties;
+using Xamarin.Android.Tools;
 
 namespace Xamarin.Android.Tasks
 {
@@ -732,70 +733,21 @@ async Task<AdbCommandResult> RunAdbCommand (string [] arguments, Dictionary<stri
 			}
 			adbArguments.AddRange (arguments);
 
-			var stdout = new StringBuilder ();
-			var stderr = new StringBuilder ();
-			using var stdoutCompleted = new ManualResetEvent (false);
-			using var stderrCompleted = new ManualResetEvent (false);
-			var psi = new ProcessStartInfo {
-				FileName = adb,
-				Arguments = string.Join (" ", adbArguments.Select (QuoteProcessArgument)),
-				UseShellExecute = false,
-				RedirectStandardOutput = true,
-				RedirectStandardError = true,
-				CreateNoWindow = true,
-				WindowStyle = ProcessWindowStyle.Hidden,
-			};
-			if (environmentVariables != null) {
-				foreach (var kvp in environmentVariables) {
-					psi.EnvironmentVariables [kvp.Key] = kvp.Value;
-				}
-			}
+			var psi = ProcessUtils.CreateProcessStartInfo (adb, adbArguments.ToArray ());
+			psi.WindowStyle = ProcessWindowStyle.Hidden;
 
-			LogDiagnostic ($"adb command: {psi.FileName} {psi.Arguments}");
-			using (var process = new Process ()) {
-				process.StartInfo = psi;
-				process.OutputDataReceived += (sender, e) => {
-					if (e.Data != null) {
-						lock (stdout) {
-							stdout.AppendLine (e.Data);
-						}
-					} else {
-						stdoutCompleted.Set ();
-					}
-				};
-				process.ErrorDataReceived += (sender, e) => {
-					if (e.Data != null) {
-						lock (stderr) {
-							stderr.AppendLine (e.Data);
-						}
-					} else {
-						stderrCompleted.Set ();
-					}
-				};
+			LogDiagnostic ($"adb command: {psi.FileName} {string.Join (" ", adbArguments)}");
 
-				process.Start ();
-				process.BeginOutputReadLine ();
-				process.BeginErrorReadLine ();
-				using (CancellationToken.Register (() => {
-					try {
-						if (!process.HasExited) {
-							process.Kill ();
-						}
-					} catch (InvalidOperationException) {
-					}
-				})) {
-					await Task.Run (() => process.WaitForExit (), CancellationToken);
-				}
-				stdoutCompleted.WaitOne (TimeSpan.FromSeconds (30));
-				stderrCompleted.WaitOne (TimeSpan.FromSeconds (30));
-				var result = new AdbCommandResult {
-					ExitCode = process.ExitCode,
-					StandardOutput = stdout.ToString ().Trim (),
-					StandardError = stderr.ToString ().Trim (),
-				};
-				LogAdbCommandResult (result);
-				return result;
-			}
+			using var stdout = new StringWriter ();
+			using var stderr = new StringWriter ();
+			int exitCode = await ProcessUtils.StartProcess (psi, stdout, stderr, CancellationToken, environmentVariables);
+			var result = new AdbCommandResult {
+				ExitCode = exitCode,
+				StandardOutput = stdout.ToString ().Trim (),
+				StandardError = stderr.ToString ().Trim (),
+			};
+			LogAdbCommandResult (result);
+			return result;
 		}
 
 		void LogAdbCommandResult (AdbCommandResult result)

src/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs

@@ -186,7 +186,8 @@ internal static void Exec (ProcessStartInfo processStartInfo, DataReceivedEventH
 		/// <summary>
 		/// Creates a <see cref="ProcessStartInfo"/> with the given filename and arguments.
 		/// On .NET 5+ uses <see cref="ProcessStartInfo.ArgumentList"/> to avoid shell-escaping issues;
-		/// on older frameworks falls back to a single <see cref="ProcessStartInfo.Arguments"/> string.
+		/// on older frameworks falls back to a single <see cref="ProcessStartInfo.Arguments"/> string
+		/// built by <see cref="JoinArguments"/>.
 		/// </summary>
 		public static ProcessStartInfo CreateProcessStartInfo (string fileName, params string[] args)
 		{
@@ -204,18 +205,66 @@ public static ProcessStartInfo CreateProcessStartInfo (string fileName, params s
 			return psi;
 		}
 
-#if !NET5_0_OR_GREATER
-		static string JoinArguments (string[] args)
+		static readonly char [] CharsRequiringQuotes = { ' ', '\t', '"', '\n', '\v' };
+
+		/// <summary>
+		/// Joins <paramref name="args"/> into a single command line suitable for
+		/// <see cref="ProcessStartInfo.Arguments"/>.
+		/// </summary>
+		/// <remarks>
+		/// Implements the quoting rules understood by <c>CommandLineToArgvW</c>, which are also
+		/// the rules .NET uses when it parses <see cref="ProcessStartInfo.Arguments"/> on Unix.
+		/// A run of backslashes is only an escape sequence when it is immediately followed by a
+		/// quote, so backslashes must *not* be doubled unconditionally: doing so turns
+		/// <c>C:\dir\file.dll</c> into <c>C:\\dir\\file.dll</c>, which some tools (notably
+		/// <c>adb push</c>) reject.
+		/// </remarks>
+		internal static string JoinArguments (params string?[] args)
 		{
 			var sb = new StringBuilder ();
 			for (int i = 0; i < args.Length; i++) {
 				if (i > 0)
 					sb.Append (' ');
-				sb.Append ('"').Append (args [i]).Append ('"');
+				AppendArgument (sb, args [i]);
 			}
 			return sb.ToString ();
 		}
-#endif
+
+		static void AppendArgument (StringBuilder sb, string? argument)
+		{
+			if (argument is null || argument.Length == 0) {
+				sb.Append ("\"\"");
+				return;
+			}
+
+			if (argument.IndexOfAny (CharsRequiringQuotes) < 0) {
+				sb.Append (argument);
+				return;
+			}
+
+			sb.Append ('"');
+			for (int i = 0; i < argument.Length; i++) {
+				int backslashes = 0;
+				while (i < argument.Length && argument [i] == '\\') {
+					backslashes++;
+					i++;
+				}
+
+				if (i == argument.Length) {
+					// Trailing backslashes precede the closing quote, so they must be doubled
+					// to avoid escaping it.
+					sb.Append ('\\', backslashes * 2);
+					break;
+				}
+
+				if (argument [i] == '"') {
+					sb.Append ('\\', backslashes * 2 + 1).Append ('"');
+				} else {
+					sb.Append ('\\', backslashes).Append (argument [i]);
+				}
+			}
+			sb.Append ('"');
+		}
 
 		/// <summary>
 		/// Throws <see cref="InvalidOperationException"/> when <paramref name="exitCode"/> is non-zero.

tests/Xamarin.Android.Tools.AndroidSdk-Tests/ProcessUtilsTests.cs

@@ -1,7 +1,9 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Collections.Generic;
 using System.Diagnostics;
+using System.Text;
 
 using NUnit.Framework;
 
@@ -66,5 +68,169 @@ public void IsElevated_DoesNotThrow ()
 			bool result = ProcessUtils.IsElevated ();
 			Assert.That (result, Is.TypeOf<bool> ());
 		}
+
+		[Test]
+		public void JoinArguments_WindowsPathIsNotDoubleEscaped ()
+		{
+			// Regression test: escaping every backslash produced `C:\\dir\\file.dll`, which
+			// made `adb push` fail with "failed to read all of ...: Invalid argument".
+			Assert.AreEqual (@"C:\Users\me\obj\a.dll", ProcessUtils.JoinArguments (@"C:\Users\me\obj\a.dll"));
+		}
+
+		[Test]
+		public void JoinArguments_PathWithSpacesIsQuotedButNotDoubleEscaped ()
+		{
+			Assert.AreEqual ("\"C:\\path with spaces\\a.dll\"", ProcessUtils.JoinArguments (@"C:\path with spaces\a.dll"));
+		}
+
+		[Test]
+		public void JoinArguments_EmbeddedQuoteIsEscaped ()
+		{
+			Assert.AreEqual ("\"he said \\\"hi\\\"\"", ProcessUtils.JoinArguments ("he said \"hi\""));
+		}
+
+		[Test]
+		public void JoinArguments_BackslashBeforeQuoteIsDoubled ()
+		{
+			Assert.AreEqual ("\"a\\\\\\\"b\"", ProcessUtils.JoinArguments ("a\\\"b"));
+		}
+
+		[Test]
+		public void JoinArguments_TrailingBackslashWithSpacesIsDoubled ()
+		{
+			// The trailing backslash precedes the closing quote, so it must be doubled.
+			Assert.AreEqual ("\"C:\\a b\\\\\"", ProcessUtils.JoinArguments (@"C:\a b\"));
+		}
+
+		[Test]
+		public void JoinArguments_TrailingBackslashWithoutSpacesNeedsNoQuoting ()
+		{
+			Assert.AreEqual (@"C:\dir\", ProcessUtils.JoinArguments (@"C:\dir\"));
+		}
+
+		[Test]
+		public void JoinArguments_EmptyArgument ()
+		{
+			Assert.AreEqual ("\"\"", ProcessUtils.JoinArguments (""));
+		}
+
+		[Test]
+		public void JoinArguments_NullArgument ()
+		{
+			Assert.AreEqual ("\"\"", ProcessUtils.JoinArguments (default (string)));
+		}
+
+		[Test]
+		public void JoinArguments_NoArguments ()
+		{
+			Assert.AreEqual ("", ProcessUtils.JoinArguments ());
+		}
+
+		[Test]
+		public void JoinArguments_MultipleArgumentsAreSpaceSeparated ()
+		{
+			Assert.AreEqual (
+				"push -z any \"C:\\a b\\x.dll\" C:\\y.dll /data/local/tmp",
+				ProcessUtils.JoinArguments ("push", "-z", "any", @"C:\a b\x.dll", @"C:\y.dll", "/data/local/tmp"));
+		}
+
+		[Test]
+		public void JoinArguments_RoundTripsThroughArgumentParsing ()
+		{
+			var args = new [] {
+				@"C:\Users\me\obj\Debug\net11.0-android\a.dll",
+				@"C:\path with spaces\b.dll",
+				"he said \"hi\"",
+				@"C:\dir\",
+				@"C:\a b\",
+				"plain",
+			};
+			var parsed = SplitCommandLine (ProcessUtils.JoinArguments (args));
+			CollectionAssert.AreEqual (args, parsed);
+		}
+
+		/// <summary>
+		/// Arguments that contain no whitespace or quotes are now emitted bare rather than
+		/// wrapped in quotes. That is transparent to the child process (the quotes were always
+		/// stripped by argument parsing), but <see cref="AdbRunner"/> passes many such arguments,
+		/// so assert the shapes it uses still arrive unchanged.
+		/// </summary>
+		[TestCase ("devices")]
+		[TestCase ("-l")]
+		[TestCase ("-s")]
+		[TestCase ("58230DLCR0013R")]
+		[TestCase ("emulator-5554")]
+		[TestCase ("tcp:5555")]
+		[TestCase ("localabstract:org.example_debug")]
+		[TestCase ("--remove-all")]
+		[TestCase ("ro.product.cpu.abilist")]
+		[TestCase ("getprop")]
+		public void JoinArguments_AdbArgumentShapesRoundTrip (string argument)
+		{
+			CollectionAssert.AreEqual (new [] { argument }, SplitCommandLine (ProcessUtils.JoinArguments (argument)));
+		}
+
+		[Test]
+		public void JoinArguments_AdbShellCommandRoundTrips ()
+		{
+			// `AdbRunner.RunShellCommandAsync` passes an entire shell command as one argument.
+			var args = new [] { "-s", "58230DLCR0013R", "shell", "echo \"remote=$(cat /data/local/tmp/x)\"" };
+			CollectionAssert.AreEqual (args, SplitCommandLine (ProcessUtils.JoinArguments (args)));
+		}
+
+		/// <summary>
+		/// Minimal implementation of the <c>CommandLineToArgvW</c> parsing rules, used to verify
+		/// that <see cref="ProcessUtils.JoinArguments"/> round-trips.
+		/// </summary>
+		static List<string> SplitCommandLine (string commandLine)
+		{
+			var results = new List<string> ();
+			var current = new StringBuilder ();
+			bool inQuotes = false, hasArgument = false;
+
+			for (int i = 0; i < commandLine.Length; i++) {
+				char c = commandLine [i];
+				if (c == '\\') {
+					int backslashes = 0;
+					while (i < commandLine.Length && commandLine [i] == '\\') {
+						backslashes++;
+						i++;
+					}
+					if (i < commandLine.Length && commandLine [i] == '"') {
+						current.Append ('\\', backslashes / 2);
+						if (backslashes % 2 == 0) {
+							inQuotes = !inQuotes;
+						} else {
+							current.Append ('"');
+						}
+						hasArgument = true;
+					} else {
+						current.Append ('\\', backslashes);
+						i--;
+					}
+					continue;
+				}
+				if (c == '"') {
+					inQuotes = !inQuotes;
+					hasArgument = true;
+					continue;
+				}
+				if (!inQuotes && (c == ' ' || c == '\t')) {
+					if (hasArgument || current.Length > 0) {
+						results.Add (current.ToString ());
+						current.Clear ();
+						hasArgument = false;
+					}
+					continue;
+				}
+				current.Append (c);
+				hasArgument = true;
+			}
+
+			if (hasArgument || current.Length > 0) {
+				results.Add (current.ToString ());
+			}
+			return results;
+		}
 	}
 }