diff --git a/src/Xamarin.Android.Build.Debugging.Tasks/Tasks/FastDeploy2.Adb.cs b/src/Xamarin.Android.Build.Debugging.Tasks/Tasks/FastDeploy2.Adb.cs index 338ab6cf324..dc6170e2400 100644 --- a/src/Xamarin.Android.Build.Debugging.Tasks/Tasks/FastDeploy2.Adb.cs +++ b/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", }; } - - /// Quotes an argument for . - 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 (); - } } /// diff --git a/src/Xamarin.Android.Build.Debugging.Tasks/Tasks/FastDeploy2.cs b/src/Xamarin.Android.Build.Debugging.Tasks/Tasks/FastDeploy2.cs index c655728f46a..4c9cebb38fb 100644 --- a/src/Xamarin.Android.Build.Debugging.Tasks/Tasks/FastDeploy2.cs +++ b/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,26 @@ async Task RunAdbCommand (string [] arguments, Dictionary $"[{a}]")); + LogDiagnostic ($"adb command: {psi.FileName} {commandLine}"); + + 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 (), }; - if (environmentVariables != null) { - foreach (var kvp in environmentVariables) { - psi.EnvironmentVariables [kvp.Key] = kvp.Value; - } - } - - 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 (); - } - }; - - 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; - } + LogAdbCommandResult (result); + return result; } void LogAdbCommandResult (AdbCommandResult result) diff --git a/src/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs b/src/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs index b5bf8c1f8dc..45eaeb3edf7 100644 --- a/src/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs +++ b/src/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs @@ -186,7 +186,8 @@ internal static void Exec (ProcessStartInfo processStartInfo, DataReceivedEventH /// /// Creates a with the given filename and arguments. /// On .NET 5+ uses to avoid shell-escaping issues; - /// on older frameworks falls back to a single string. + /// on older frameworks falls back to a single string + /// built by . /// public static ProcessStartInfo CreateProcessStartInfo (string fileName, params string[] args) { @@ -204,18 +205,74 @@ public static ProcessStartInfo CreateProcessStartInfo (string fileName, params s return psi; } -#if !NET5_0_OR_GREATER - static string JoinArguments (string[] args) + /// + /// Joins into a single command line suitable for + /// . + /// + /// + /// Implements the quoting rules understood by CommandLineToArgvW, which are also + /// the rules .NET uses when it parses 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:\dir\file.dll into C:\\dir\\file.dll, which some tools (notably + /// adb push) reject. + /// + 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 (ContainsNoWhitespaceOrQuotes (argument)) { + 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 ('"'); + } + + static bool ContainsNoWhitespaceOrQuotes (string s) + { + for (int i = 0; i < s.Length; i++) { + char c = s [i]; + if (char.IsWhiteSpace (c) || c == '"') + return false; + } + return true; + } /// /// Throws when is non-zero. diff --git a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/ProcessUtilsTests.cs b/tests/Xamarin.Android.Tools.AndroidSdk-Tests/ProcessUtilsTests.cs index a9e7be31d8a..b44b0153174 100644 --- a/tests/Xamarin.Android.Tools.AndroidSdk-Tests/ProcessUtilsTests.cs +++ b/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,186 @@ public void IsElevated_DoesNotThrow () bool result = ProcessUtils.IsElevated (); Assert.That (result, Is.TypeOf ()); } + + [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); + } + + /// + /// 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 passes many such arguments, + /// so assert the shapes it uses still arrive unchanged. + /// + [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))); + } + + /// + /// Whitespace detection matches the BCL's PasteArguments, which uses + /// rather than just space and tab. Quoting an + /// argument is always safe, so erring towards quoting keeps the two implementations + /// in agreement. + /// + [TestCase ("a\rb")] + [TestCase ("a\fb")] + [TestCase ("a\nb")] + [TestCase ("a\vb")] + [TestCase ("a\u00a0b")] + public void JoinArguments_AllWhitespaceIsQuoted (string argument) + { + var joined = ProcessUtils.JoinArguments (argument); + Assert.AreEqual ($"\"{argument}\"", joined); + } + + /// + /// Minimal implementation of the CommandLineToArgvW parsing rules, used to verify + /// that round-trips. + /// + static List SplitCommandLine (string commandLine) + { + var results = new List (); + 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; + } } }