From 3e12d44a707e6e1daeff3f7f67729a9b7c932fb6 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Wed, 7 Feb 2018 10:22:51 -0600 Subject: [PATCH] Make error messages consistent across platforms (dotnet/core-setup#3667) Commit migrated from https://github.com/dotnet/core-setup/commit/d8cb2dc6e25340a365be84763e12e74e6eb92a7c --- src/installer/corehost/cli/fxr/fx_muxer.cpp | 79 ++++++++++------- src/installer/corehost/cli/fxr/fx_muxer.h | 1 - src/installer/corehost/common/pal.windows.cpp | 86 +++++++++---------- src/installer/corehost/common/utils.cpp | 2 +- ...enThatICareAboutDotnetArgValidationScenarios.cs | 98 ++++++++++++++++++++++ 5 files changed, 185 insertions(+), 81 deletions(-) create mode 100644 src/installer/test/HostActivationTests/GivenThatICareAboutDotnetArgValidationScenarios.cs diff --git a/src/installer/corehost/cli/fxr/fx_muxer.cpp b/src/installer/corehost/cli/fxr/fx_muxer.cpp index e24fed4..55e19a8 100644 --- a/src/installer/corehost/cli/fxr/fx_muxer.cpp +++ b/src/installer/corehost/cli/fxr/fx_muxer.cpp @@ -1092,17 +1092,21 @@ void append_probe_realpath(const pal::string_t& path, std::vector { pal::string_t probe_path = path; - if (pal::realpath(&probe_path) && pal::directory_exists(probe_path)) + if (pal::realpath(&probe_path)) { realpaths->push_back(probe_path); } else { - //Check if we can extrapolate |arch||tfm| for probing stores - pal::string_t placeholder = _X("|arch|"); - placeholder.push_back(DIR_SEPARATOR); - placeholder.append(_X("|tfm|")); + // Check if we can extrapolate |arch||tfm| for probing stores + // Check for for both forward and back slashes + pal::string_t placeholder = _X("|arch|\\|tfm|"); auto pos_placeholder = probe_path.find(placeholder); + if (pos_placeholder == pal::string_t::npos) + { + placeholder = _X("|arch|/|tfm|"); + pos_placeholder = probe_path.find(placeholder); + } if (pos_placeholder != pal::string_t::npos) { @@ -1111,7 +1115,7 @@ void append_probe_realpath(const pal::string_t& path, std::vector segment.append(tfm); probe_path.replace(pos_placeholder, placeholder.length(), segment); - if (pal::directory_exists(probe_path)) + if (pal::realpath(&probe_path)) { realpaths->push_back(probe_path); } @@ -1146,6 +1150,7 @@ std::vector fx_muxer_t::get_known_opts(bool exec_mode, host_mode_t return known_opts; } +// Returns '0' on success, 'AppArgNotRunnable' if should be routed to CLI, otherwise error code. int fx_muxer_t::parse_args( const pal::string_t& own_dir, const pal::string_t& own_dll, @@ -1154,13 +1159,10 @@ int fx_muxer_t::parse_args( const pal::char_t* argv[], bool exec_mode, host_mode_t mode, - bool* is_an_app, int* new_argoff, pal::string_t& app_candidate, opt_map_t& opts) { - *is_an_app = true; - std::vector known_opts = get_known_opts(exec_mode, mode); // Parse the known arguments if any. @@ -1177,7 +1179,12 @@ int fx_muxer_t::parse_args( app_candidate = own_dll; *new_argoff = argoff + num_parsed; - if (mode != host_mode_t::standalone) + bool doesAppExist = false; + if (mode == host_mode_t::standalone) + { + doesAppExist = pal::realpath(&app_candidate); + } + else { trace::verbose(_X("Detected a non-standalone application, expecting app.dll to execute.")); if (*new_argoff >= argc) @@ -1186,29 +1193,39 @@ int fx_muxer_t::parse_args( } app_candidate = argv[*new_argoff]; - bool is_app_managed = (ends_with(app_candidate, _X(".dll"), false) || ends_with(app_candidate, _X(".exe"), false)) && pal::realpath(&app_candidate); + bool is_app_managed = ends_with(app_candidate, _X(".dll"), false) || ends_with(app_candidate, _X(".exe"), false); if (!is_app_managed) { trace::verbose(_X("Application '%s' is not a managed executable."), app_candidate.c_str()); + if (!exec_mode) + { + // Route to CLI. + return AppArgNotRunnable; + } + } - *is_an_app = false; - - if (exec_mode) + doesAppExist = pal::realpath(&app_candidate); + if (!doesAppExist) + { + trace::verbose(_X("Application '%s' does not exist."), app_candidate.c_str()); + if (!exec_mode) { - trace::error(_X("dotnet exec needs a managed .dll or .exe extension. The application specified was '%s'"), app_candidate.c_str()); - return InvalidArgFailure; + // Route to CLI. + return AppArgNotRunnable; } + } - // Route to CLI. - return AppArgNotRunnable; + if (!is_app_managed && doesAppExist) + { + assert(exec_mode == true); + trace::error(_X("dotnet exec needs a managed .dll or .exe extension. The application specified was '%s'"), app_candidate.c_str()); + return InvalidArgFailure; } } // App is managed executable. - trace::verbose(_X("Treating application '%s' as a managed executable."), app_candidate.c_str()); - - if (!pal::file_exists(app_candidate)) + if (!doesAppExist) { trace::error(_X("The application to execute does not exist: '%s'"), app_candidate.c_str()); return InvalidArgFailure; @@ -1223,7 +1240,7 @@ int read_config( pal::string_t& runtime_config ) { - if (!runtime_config.empty() && (!pal::realpath(&runtime_config) || !pal::file_exists(runtime_config))) + if (!runtime_config.empty() && !pal::realpath(&runtime_config)) { trace::error(_X("The specified runtimeconfig.json [%s] does not exist"), runtime_config.c_str()); return StatusCode::InvalidConfigFile; @@ -1278,7 +1295,7 @@ int fx_muxer_t::read_config_and_execute( pal::string_t runtime_config = get_last_known_arg(opts, opts_runtime_config, _X("")); std::vector spec_probe_paths = opts.count(opts_probe_path) ? opts.find(opts_probe_path)->second : std::vector(); - if (!deps_file.empty() && (!pal::realpath(&deps_file) || !pal::file_exists(deps_file))) + if (!deps_file.empty() && !pal::realpath(&deps_file)) { trace::error(_X("The specified deps.json [%s] does not exist"), deps_file.c_str()); return StatusCode::InvalidArgFailure; @@ -1460,7 +1477,6 @@ int fx_muxer_t::execute( // Detect invocation mode host_mode_t mode = detect_operating_mode(own_dir, own_dll, own_name); - bool is_an_app = true; // Indicates activation of an app instead of an invocation of the SDK. int new_argoff; pal::string_t app_candidate; opt_map_t opts; @@ -1469,13 +1485,13 @@ int fx_muxer_t::execute( { // Invoked as corehost trace::verbose(_X("--- Executing in split/FX mode...")); - result = parse_args(own_dir, own_dll, 1, argc, argv, false, mode, &is_an_app, &new_argoff, app_candidate, opts); + result = parse_args(own_dir, own_dll, 1, argc, argv, false, mode, &new_argoff, app_candidate, opts); } else if (mode == host_mode_t::standalone) { // Invoked from the application base. trace::verbose(_X("--- Executing in standalone mode...")); - result = parse_args(own_dir, own_dll, 1, argc, argv, false, mode, &is_an_app, &new_argoff, app_candidate, opts); + result = parse_args(own_dir, own_dll, 1, argc, argv, false, mode, &new_argoff, app_candidate, opts); } else { @@ -1490,20 +1506,20 @@ int fx_muxer_t::execute( if (pal::strcasecmp(_X("exec"), argv[1]) == 0) { - result = parse_args(own_dir, own_dll, 2, argc, argv, true, mode, &is_an_app, &new_argoff, app_candidate, opts); // arg offset 2 for dotnet, exec + result = parse_args(own_dir, own_dll, 2, argc, argv, true, mode, &new_argoff, app_candidate, opts); // arg offset 2 for dotnet, exec } else { - result = parse_args(own_dir, own_dll, 1, argc, argv, false, mode, &is_an_app, &new_argoff, app_candidate, opts); // arg offset 1 for dotnet + result = parse_args(own_dir, own_dll, 1, argc, argv, false, mode, &new_argoff, app_candidate, opts); // arg offset 1 for dotnet - if (!is_an_app) + if (result == AppArgNotRunnable) { return handle_cli(own_dir, own_dll, argc, argv); } } } - if (!result && is_an_app) + if (!result) { // Transform dotnet [exec] [--additionalprobingpath path] [--depsfile file] [dll] [args] -> dotnet [dll] [args] result = handle_exec_host_command(host_command, own_dir, app_candidate, opts, argc, argv, new_argoff, mode, result_buffer, buffer_size, required_buffer_size); @@ -1610,12 +1626,11 @@ int fx_muxer_t::handle_cli( trace::verbose(_X("Using dotnet SDK dll=[%s]"), sdk_dotnet.c_str()); - bool is_an_app; int new_argoff; pal::string_t app_candidate; opt_map_t opts; - int result = parse_args(own_dir, own_dll, 1, new_argv.size(), new_argv.data(), false, host_mode_t::muxer, &is_an_app, &new_argoff, app_candidate, opts); // arg offset 1 for dotnet + int result = parse_args(own_dir, own_dll, 1, new_argv.size(), new_argv.data(), false, host_mode_t::muxer, &new_argoff, app_candidate, opts); // arg offset 1 for dotnet if (!result) { // Transform dotnet [exec] [--additionalprobingpath path] [--depsfile file] [dll] [args] -> dotnet [dll] [args] diff --git a/src/installer/corehost/cli/fxr/fx_muxer.h b/src/installer/corehost/cli/fxr/fx_muxer.h index 1429d80..26114ba 100644 --- a/src/installer/corehost/cli/fxr/fx_muxer.h +++ b/src/installer/corehost/cli/fxr/fx_muxer.h @@ -47,7 +47,6 @@ public: const pal::char_t* argv[], bool exec_mode, host_mode_t mode, - bool* is_an_app, int* new_argoff, pal::string_t& app_candidate, opt_map_t& opts); diff --git a/src/installer/corehost/common/pal.windows.cpp b/src/installer/corehost/common/pal.windows.cpp index 82a58b6..6e2c3a2 100644 --- a/src/installer/corehost/common/pal.windows.cpp +++ b/src/installer/corehost/common/pal.windows.cpp @@ -362,56 +362,66 @@ bool pal::clr_palstring(const char* cstr, pal::string_t* out) return wchar_convert_helper(CP_UTF8, cstr, ::strlen(cstr), out); } +// Return if path is valid and file exists, return true and adjust path as appropriate. bool pal::realpath(string_t* path) { - - if (LongFile::IsNormalized(*path)) + if (LongFile::IsNormalized(path->c_str())) { - return true; + WIN32_FILE_ATTRIBUTE_DATA data; + if (GetFileAttributesExW(path->c_str(), GetFileExInfoStandard, &data) != 0) + { + return true; + } } char_t buf[MAX_PATH]; auto size = ::GetFullPathNameW(path->c_str(), MAX_PATH, buf, nullptr); - if (size == 0) { trace::error(_X("Error resolving full path [%s]"), path->c_str()); return false; } + string_t str; if (size < MAX_PATH) { - path->assign(buf); - return true; + str.assign(buf); } + else + { + str.resize(size + LongFile::UNCExtendedPathPrefix.length(), 0); - string_t str; - str.resize(size + LongFile::UNCExtendedPathPrefix.length(), 0); + size = ::GetFullPathNameW(path->c_str(), size, (LPWSTR)str.data(), nullptr); + assert(size <= str.size()); - size = ::GetFullPathNameW(path->c_str() , size, (LPWSTR)str.data() , nullptr); - assert(size <= str.size()); - - if (size == 0) - { - trace::error(_X("Error resolving full path [%s]"), path->c_str()); - return false; + if (size == 0) + { + trace::error(_X("Error resolving full path [%s]"), path->c_str()); + return false; + } + + const string_t* prefix = &LongFile::ExtendedPrefix; + //Check if the resolved path is a UNC. By default we assume relative path to resolve to disk + if (str.compare(0, LongFile::UNCPathPrefix.length(), LongFile::UNCPathPrefix) == 0) + { + prefix = &LongFile::UNCExtendedPathPrefix; + str.erase(0, LongFile::UNCPathPrefix.length()); + size = size - LongFile::UNCPathPrefix.length(); + } + + str.insert(0, *prefix); + str.resize(size + prefix->length()); + str.shrink_to_fit(); } - const string_t* prefix = &LongFile::ExtendedPrefix; - //Check if the resolved path is a UNC. By default we assume relative path to resolve to disk - if (str.compare(0, LongFile::UNCPathPrefix.length(), LongFile::UNCPathPrefix) == 0) + WIN32_FILE_ATTRIBUTE_DATA data; + if (GetFileAttributesExW(str.c_str(), GetFileExInfoStandard, &data) != 0) { - prefix = &LongFile::UNCExtendedPathPrefix; - str.erase(0, LongFile::UNCPathPrefix.length()); - size = size - LongFile::UNCPathPrefix.length(); + *path = str; + return true; } - str.insert(0, *prefix); - str.resize(size + prefix->length()); - str.shrink_to_fit(); - *path = str; - - return true; + return false; } bool pal::file_exists(const string_t& path) @@ -421,26 +431,8 @@ bool pal::file_exists(const string_t& path) return false; } - auto pathstring = path.c_str(); - string_t normalized_path; - if (LongFile::ShouldNormalize(path)) - { - normalized_path = path; - if (!pal::realpath(&normalized_path)) - { - return false; - } - pathstring = normalized_path.c_str(); - } - - // We will attempt to fetch attributes for the file or folder in question that are - // returned only if they exist. - WIN32_FILE_ATTRIBUTE_DATA data; - if (GetFileAttributesExW(pathstring, GetFileExInfoStandard, &data) != 0) { - return true; - } - - return false; + string_t tmp(path); + return pal::realpath(&tmp); } static void readdir(const pal::string_t& path, const pal::string_t& pattern, bool onlydirectories, std::vector* list) diff --git a/src/installer/corehost/common/utils.cpp b/src/installer/corehost/common/utils.cpp index 09417678..471bc75 100644 --- a/src/installer/corehost/common/utils.cpp +++ b/src/installer/corehost/common/utils.cpp @@ -329,7 +329,7 @@ bool get_path_from_argv(pal::string_t *path) // the wrong location when filename is ends up being found in %PATH% and not the current directory. if (path->find(DIR_SEPARATOR) != pal::string_t::npos) { - return (pal::realpath(path) && pal::file_exists(*path)); + return pal::realpath(path); } return false; diff --git a/src/installer/test/HostActivationTests/GivenThatICareAboutDotnetArgValidationScenarios.cs b/src/installer/test/HostActivationTests/GivenThatICareAboutDotnetArgValidationScenarios.cs new file mode 100644 index 0000000..c129abc --- /dev/null +++ b/src/installer/test/HostActivationTests/GivenThatICareAboutDotnetArgValidationScenarios.cs @@ -0,0 +1,98 @@ +using FluentAssertions; +using Microsoft.DotNet.CoreSetup.Test; +using Microsoft.DotNet.Cli.Build.Framework; +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; +using System.IO; +using System.Runtime.InteropServices; +using Xunit; + +namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.ArgValidation +{ + public class GivenThatICareAboutDotnetArgValidationScenarios + { + private RepoDirectoriesProvider RepoDirectories { get; set; } + private TestProjectFixture PreviouslyBuiltAndRestoredPortableTestProjectFixture { get; set; } + + public GivenThatICareAboutDotnetArgValidationScenarios() + { + RepoDirectories = new RepoDirectoriesProvider(); + + PreviouslyBuiltAndRestoredPortableTestProjectFixture = new TestProjectFixture("PortableApp", RepoDirectories) + .EnsureRestored(RepoDirectories.CorehostPackages) + .BuildProject(); + } + + [Fact] + public void Muxer_Exec_With_Missing_App_Assembly_Fails() + { + var fixture = PreviouslyBuiltAndRestoredPortableTestProjectFixture + .Copy(); + + var dotnet = fixture.BuiltDotnet; + + string assemblyName = Path.Combine(GetNonexistentAndUnnormalizedPath(), "foo.dll"); + + dotnet.Exec("exec", assemblyName) + .CaptureStdOut() + .CaptureStdErr() + .Execute() + .Should() + .Fail() + .And + .HaveStdErrContaining($"The application to execute does not exist: '{assemblyName}'"); + } + + [Fact] + public void Muxer_Exec_With_Missing_App_Assembly_And_Bad_Extension_Fails() + { + var fixture = PreviouslyBuiltAndRestoredPortableTestProjectFixture + .Copy(); + + var dotnet = fixture.BuiltDotnet; + + string assemblyName = Path.Combine(GetNonexistentAndUnnormalizedPath(), "foo.xzy"); + + dotnet.Exec("exec", assemblyName) + .CaptureStdOut() + .CaptureStdErr() + .Execute() + .Should() + .Fail() + .And + .HaveStdErrContaining($"The application to execute does not exist: '{assemblyName}'"); + } + + [Fact] + public void Muxer_Exec_With_Bad_Extension_Fails() + { + var fixture = PreviouslyBuiltAndRestoredPortableTestProjectFixture + .Copy(); + + var dotnet = fixture.BuiltDotnet; + + // Get a valid file name, but not exe or dll + string fxDir = Path.Combine(fixture.SdkDotnet.BinPath, "shared", "Microsoft.NETCore.App"); + fxDir = new DirectoryInfo(fxDir).GetDirectories()[0].FullName; + string assemblyName = Path.Combine(fxDir, "Microsoft.NETCore.App.deps.json"); + + dotnet.Exec("exec", assemblyName) + .CaptureStdOut() + .CaptureStdErr() + .Execute() + .Should() + .Fail() + .And + .HaveStdErrContaining($"dotnet exec needs a managed .dll or .exe extension. The application specified was '{assemblyName}'"); + } + + + // Return a non-exisitent path that contains a mix of / and \ + private string GetNonexistentAndUnnormalizedPath() + { + return Path.Combine(PreviouslyBuiltAndRestoredPortableTestProjectFixture.SdkDotnet.BinPath, @"x\y/"); + } + } +} -- 2.7.4