Fix for invalid values in %PATH% or %PATHEXT%

[jonathanpeppers]

Fixes: https://developercommunity.visualstudio.com/t/illegal-character-exception-in-xamarinandroid-afte/1363149

In 16.9 we are getting some reports of:

(_ResolveSdks target) ->
    error XARSD7004: System.ArgumentException: Illegal characters in path.
    error XARSD7004:    at System.IO.Path.CheckInvalidPathChars(String path, Boolean checkAdditional)
    error XARSD7004:    at System.IO.Path.Combine(String path1, String path2)
    error XARSD7004:    at Xamarin.Android.Tools.ProcessUtils.<FindExecutablesInDirectory>d__9.MoveNext() in /Users/builder/azdo/_work/278/s/xamarin-android/external/xamarin-android-tools/src/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs:line 177
    error XARSD7004:    at Xamarin.Android.Tools.ProcessUtils.<FindExecutablesInPath>d__8.MoveNext() in /Users/builder/azdo/_work/278/s/xamarin-android/external/xamarin-android-tools/src/Xamarin.Android.Tools.AndroidSdk/ProcessUtils.cs:line 168
    error XARSD7004:    at Xamarin.Android.Tools.AndroidSdkBase.<GetAllAvailableAndroidNdks>d__73.MoveNext() in /Users/builder/azdo/_work/278/s/xamarin-android/external/xamarin-android-tools/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs:line 153
    error XARSD7004:    at Xamarin.Android.Tools.AndroidSdkWindows.<GetAllAvailableAndroidNdks>d__43.MoveNext() in /Users/builder/azdo/_work/278/s/xamarin-android/external/xamarin-android-tools/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs:line 257
    error XARSD7004:    at Xamarin.Android.Tools.AndroidSdkBase.GetValidNdkPath(String ctorParam) in /Users/builder/azdo/_work/278/s/xamarin-android/external/xamarin-android-tools/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs:line 128
    error XARSD7004:    at Xamarin.Android.Tools.AndroidSdkBase.Initialize(String androidSdkPath, String androidNdkPath, String javaSdkPath) in /Users/builder/azdo/_work/278/s/xamarin-android/external/xamarin-android-tools/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs:line 71
    error XARSD7004:    at Xamarin.Android.Tools.AndroidSdkWindows.Initialize(String androidSdkPath, String androidNdkPath, String javaSdkPath) in /Users/builder/azdo/_work/278/s/xamarin-android/external/xamarin-android-tools/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs:line 310
    error XARSD7004:    at Xamarin.Android.Tools.AndroidSdkInfo..ctor(Action`2 logger, String androidSdkPath, String androidNdkPath, String javaSdkPath) in /Users/builder/azdo/_work/278/s/xamarin-android/external/xamarin-android-tools/src/Xamarin.Android.Tools.AndroidSdk/AndroidSdkInfo.cs:line 18
    error XARSD7004:    at Xamarin.Android.Tasks.MonoAndroidHelper.RefreshAndroidSdk(String sdkPath, String ndkPath, String javaPath, TaskLoggingHelper logHelper)
    error XARSD7004:    at Xamarin.Android.Tasks.ResolveSdks.RunTask()
    error XARSD7004:    at Xamarin.Android.Tasks.AndroidTask.Execute()

It appears this is simply a call like this failing:

Path.Combine ("foo", "\"")
Path.Combine ("\"", "foo")

I could reproduce this in a test by setting %PATH% or %PATHEXT% to
invalid names.

For fixing %PATH%, I could simply add a Directory.Exists() check
in the place that makes sense. However, I think a try-catch of
ArgumentException is the only way to handle %PATHEXT%? I had to
put this in two places where the new test found an issue.

I am not exactly sure how this problem would have been introduced in
16.9, as this problem looks like it would have always been here.

Possibly 9d8924d? Maybe the new call to
GetSdkFromEnvironmentVariables() is triggering this issue?

[jonpryor]

@jonathanpeppers wrote:

Possibly 9d8924d? Maybe the new call to
GetSdkFromEnvironmentVariables() is triggering this issue?

I think that's not a bad avenue of thinking. What happens if e.g. %ANDROID_HOME% contains quotes?

set ANDROID_HOME="C:\Path\To\My\Android\Sdk"

It's not always obvious that set doesn't require quoting the value, as quoting the value is common/typical on Unix. (I think I did this with %PATH% once, and some things were "weird"…though I forget what.)

That said, I can kinda provoke Windows to hit GetSdkFromEnvironmentVariables() -- by #ifing out lots of code -- I don't see it break:

diff --git a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs
index cd6c984..7e9864b 100644
--- a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs
+++ b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkBase.cs
@@ -117,6 +117,7 @@ namespace Xamarin.Android.Tools
 		{
 			if (ValidateAndroidNdkLocation (ctorParam))
 				return ctorParam;
+				#if false
 			if (AndroidSdkPath != null) {
 				string bundle = FindBestNDK (AndroidSdkPath);
 				if (Directory.Exists (bundle) && ValidateAndroidNdkLocation (bundle))
@@ -125,6 +126,7 @@ namespace Xamarin.Android.Tools
 			ctorParam = PreferedAndroidNdkPath;
 			if (ValidateAndroidNdkLocation (ctorParam))
 				return ctorParam;
+				#endif
 			foreach (var path in GetAllAvailableAndroidNdks ()) {
 				if (ValidateAndroidNdkLocation (path))
 					return path;
diff --git a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs
index 2f3b2f9..f6dccef 100644
--- a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs
+++ b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkUnix.cs
@@ -90,9 +90,11 @@ namespace Xamarin.Android.Tools
 
 		protected override IEnumerable<string> GetAllAvailableAndroidSdks ()
 		{
+			#if false
 			var preferedSdkPath = PreferedAndroidSdkPath;
 			if (!string.IsNullOrEmpty (preferedSdkPath))
 				yield return preferedSdkPath!;
+				#endif
 
 			foreach (string dir in GetSdkFromEnvironmentVariables ()) {
 				yield return dir;
diff --git a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs
index 7391999..a5c58ca 100644
--- a/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs
+++ b/src/Xamarin.Android.Tools.AndroidSdk/Sdks/AndroidSdkWindows.cs
@@ -218,7 +218,7 @@ namespace Xamarin.Android.Tools
 
 		protected override IEnumerable<string> GetAllAvailableAndroidNdks ()
 		{
-
+#if false
 			var roots = new[] { RegistryEx.CurrentUser, RegistryEx.LocalMachine };
 			var wow = RegistryEx.Wow64.Key32;
 			var regKey = GetMDRegistryKey ();
@@ -229,7 +229,7 @@ namespace Xamarin.Android.Tools
 			foreach (var root in roots)
 				if (CheckRegistryKeyForExecutable (root, regKey, MDREG_ANDROID_NDK, wow, ".", NdkStack))
 					yield return RegistryEx.GetValueString (root, regKey, MDREG_ANDROID_NDK, wow) ?? "";
-
+#endif
 			foreach (string dir in GetSdkFromEnvironmentVariables ()) {
 				yield return dir;
 			}

results in:

set ANDROID_HOME="C:\Program Files (x86)\Android\android-sdk"
…run my test app…
[Verbose] ValidateAndroidNdkLocation: `"C:\Program Files (x86)\Android\android-sdk"`, result=False

So we see double-quotes in the above path, but nothing breaks; there's no exception. (The directory also isn't used -- result=False -- which can make for some "unexpected behaviors.)

---

I wonder if part of what we should do, particularly around environment variables, is check the value, and if the first character is a " or ' and the last character is the same quote, trim off the quotes. Would this be at all reasonable?

It would allow people (like me) who quote ~everything to observe "reasonable" behavior, even when File.Exists() disagrees…

[jonpryor]

I wonder if part of what we should do, particularly around environment variables, is check the value…

Actually, I wonder if we should just remove all Path.GetInvalidPathChars() values within environment variables? Except on Unix, " isn't an invalid character, so maybe add " and '?