From 54fd476416448fa5aaf8e39fb21025dae5a42754 Mon Sep 17 00:00:00 2001 From: Elinor Fung <47805090+elinor-fung@users.noreply.github.com> Date: Wed, 17 Apr 2019 23:12:50 -0700 Subject: [PATCH] Fail early on duplicate runtime properties (dotnet/core-setup#5898) Commit migrated from https://github.com/dotnet/core-setup/commit/859ee7d256a043b0a9bfe3e9cc6c8483eb23efd5 --- src/installer/corehost/cli/hostpolicy/coreclr.cpp | 77 ++++++++++++++-------- src/installer/corehost/cli/hostpolicy/coreclr.h | 18 +++-- .../corehost/cli/hostpolicy/hostpolicy_context.cpp | 41 ++++++++++-- src/installer/corehost/error_codes.h | 7 +- .../test/HostActivationTests/RuntimeProperties.cs | 3 +- 5 files changed, 102 insertions(+), 44 deletions(-) diff --git a/src/installer/corehost/cli/hostpolicy/coreclr.cpp b/src/installer/corehost/cli/hostpolicy/coreclr.cpp index d9282e5..db90019 100644 --- a/src/installer/corehost/cli/hostpolicy/coreclr.cpp +++ b/src/installer/corehost/cli/hostpolicy/coreclr.cpp @@ -99,12 +99,14 @@ pal::hresult_t coreclr_t::create( std::vector keys(propertyCount); std::vector> values_strs(propertyCount); std::vector values(propertyCount); - std::function callback = [&] (int index, const pal::string_t& key, const pal::string_t& value) + int index = 0; + std::function callback = [&] (const pal::string_t& key, const pal::string_t& value) { pal::pal_clrstring(key, &keys_strs[index]); keys[index] = keys_strs[index].data(); pal::pal_clrstring(value, &values_strs[index]); values[index] = values_strs[index].data(); + ++index; }; properties.enumerate(callback); @@ -208,30 +210,46 @@ namespace static_assert((sizeof(PropertyNameMapping) / sizeof(*PropertyNameMapping)) == static_cast(common_property::Last), "Invalid property count"); } +const pal::char_t* coreclr_property_bag_t::common_property_to_string(common_property key) +{ + int idx = static_cast(key); + assert(0 <= idx && idx < static_cast(common_property::Last)); + + return PropertyNameMapping[idx]; +} + coreclr_property_bag_t::coreclr_property_bag_t() { // Optimize the bag for at least twice as many common properties. const size_t init_size = 2 * static_cast(common_property::Last); - _keys.reserve(init_size); - _values.reserve(init_size); + _properties.reserve(init_size); } -void coreclr_property_bag_t::add(common_property key, const pal::char_t *value) +bool coreclr_property_bag_t::add(common_property key, const pal::char_t *value) { int idx = static_cast(key); assert(0 <= idx && idx < static_cast(common_property::Last)); - add(PropertyNameMapping[idx], value); + return add(PropertyNameMapping[idx], value); } -void coreclr_property_bag_t::add(const pal::char_t *key, const pal::char_t *value) +bool coreclr_property_bag_t::add(const pal::char_t *key, const pal::char_t *value) { if (key == nullptr || value == nullptr) - return; + return false; - assert(_keys.size() == _values.size()); - _keys.push_back(key); - _values.push_back(value); + auto iter = _properties.find(key); + if (iter == _properties.cend()) + { + _properties.emplace(key, value); + return true; + } + else + { + trace::verbose(_X("Overwriting property %s. New value: '%s'. Old value: '%s'."), key, value, (*iter).second.c_str()); + _properties[key] = value; + return false; + } } bool coreclr_property_bag_t::try_get(common_property key, const pal::char_t **value) @@ -245,32 +263,39 @@ bool coreclr_property_bag_t::try_get(common_property key, const pal::char_t **va bool coreclr_property_bag_t::try_get(const pal::char_t *key, const pal::char_t **value) { assert(key != nullptr && value != nullptr); - for (int i = 0; i < count(); ++i) - { - if (0 == pal::strcasecmp(_keys[i].c_str(), key)) - { - *value = _values[i].c_str(); - return true; - } - } + auto iter = _properties.find(key); + if (iter == _properties.cend()) + return false; + + *value = (*iter).second.c_str(); + return true; +} + +void coreclr_property_bag_t::remove(const pal::char_t *key) +{ + if (key == nullptr) + return; + + auto iter = _properties.find(key); + if (iter == _properties.cend()) + return; - return false; + _properties.erase(iter); } void coreclr_property_bag_t::log_properties() const { - for (int i = 0; i < count(); ++i) - trace::verbose(_X("Property %s = %s"), _keys[i].c_str(), _values[i].c_str()); + for (auto &kv : _properties) + trace::verbose(_X("Property %s = %s"), kv.first.c_str(), kv.second.c_str()); } int coreclr_property_bag_t::count() const { - assert(_keys.size() == _values.size()); - return static_cast(_keys.size()); + return static_cast(_properties.size()); } -void coreclr_property_bag_t::enumerate(std::function &callback) const +void coreclr_property_bag_t::enumerate(std::function &callback) const { - for (int i = 0; i < count(); ++i) - callback(i, _keys[i], _values[i]); + for (auto &kv : _properties) + callback(kv.first, kv.second); } \ No newline at end of file diff --git a/src/installer/corehost/cli/hostpolicy/coreclr.h b/src/installer/corehost/cli/hostpolicy/coreclr.h index 06cce20..7227eac 100644 --- a/src/installer/corehost/cli/hostpolicy/coreclr.h +++ b/src/installer/corehost/cli/hostpolicy/coreclr.h @@ -75,26 +75,30 @@ enum class common_property class coreclr_property_bag_t { +public: // static + static const pal::char_t* common_property_to_string(common_property key); + public: coreclr_property_bag_t(); - void add(common_property key, const pal::char_t *value); - - void add(const pal::char_t *key, const pal::char_t *value); + // Add a property to the property bag. If the property already exists, it is overwritten. + // Returns true if the property was newly added, false if it already existed or could not be added. + bool add(common_property key, const pal::char_t *value); + bool add(const pal::char_t *key, const pal::char_t *value); bool try_get(common_property key, const pal::char_t **value); - bool try_get(const pal::char_t *key, const pal::char_t **value); + void remove(const pal::char_t *key); + void log_properties() const; int count() const; - void enumerate(std::function &callback) const; + void enumerate(std::function &callback) const; private: - std::vector _keys; - std::vector _values; + std::unordered_map _properties; }; #endif // _COREHOST_CLI_CORECLR_H_ diff --git a/src/installer/corehost/cli/hostpolicy/hostpolicy_context.cpp b/src/installer/corehost/cli/hostpolicy/hostpolicy_context.cpp index 15dbba9..aa933d7 100644 --- a/src/installer/corehost/cli/hostpolicy/hostpolicy_context.cpp +++ b/src/installer/corehost/cli/hostpolicy/hostpolicy_context.cpp @@ -8,6 +8,15 @@ #include #include +namespace +{ + void log_duplicate_property_error(const pal::char_t *property_key) + { + trace::error(_X("Duplicate runtime property found: %s"), property_key); + trace::error(_X("It is invalid to specify values for properties populated by the hosting layer in the the application's .runtimeconfig.json")); + } +} + int hostpolicy_context_t::initialize(hostpolicy_init_t &hostpolicy_init, const arguments_t &args, bool enable_breadcrumbs) { application = args.managed_application; @@ -57,7 +66,7 @@ int hostpolicy_context_t::initialize(hostpolicy_init_t &hostpolicy_init, const a clr_path = probe_paths.coreclr; if (clr_path.empty() || !pal::realpath(&clr_path)) { - trace::error(_X("Could not resolve CoreCLR path. For more details, enable tracing by setting COREHOST_TRACE environment variable to 1"));; + trace::error(_X("Could not resolve CoreCLR path. For more details, enable tracing by setting COREHOST_TRACE environment variable to 1")); return StatusCode::CoreClrResolveFailure; } @@ -146,12 +155,17 @@ int hostpolicy_context_t::initialize(hostpolicy_init_t &hostpolicy_init, const a for (int i = 0; i < hostpolicy_init.cfg_keys.size(); ++i) { // Provide opt-in compatible behavior by using the switch to set APP_PATHS - if (pal::strcasecmp(hostpolicy_init.cfg_keys[i].c_str(), _X("Microsoft.NETCore.DotNetHostPolicy.SetAppPaths")) == 0) + const pal::char_t *key = hostpolicy_init.cfg_keys[i].c_str(); + if (pal::strcasecmp(key, _X("Microsoft.NETCore.DotNetHostPolicy.SetAppPaths")) == 0) { set_app_paths = (pal::strcasecmp(hostpolicy_init.cfg_values[i].data(), _X("true")) == 0); } - coreclr_properties.add(hostpolicy_init.cfg_keys[i].c_str(), hostpolicy_init.cfg_values[i].c_str()); + if (!coreclr_properties.add(key, hostpolicy_init.cfg_values[i].c_str())) + { + log_duplicate_property_error(key); + return StatusCode::LibHostDuplicateProperty; + } } // App paths and App NI paths. @@ -159,14 +173,29 @@ int hostpolicy_context_t::initialize(hostpolicy_init_t &hostpolicy_init, const a // and that could indicate the app paths shouldn't be set. if (set_app_paths) { - coreclr_properties.add(common_property::AppPaths, app_base.c_str()); - coreclr_properties.add(common_property::AppNIPaths, app_base.c_str()); + if (!coreclr_properties.add(common_property::AppPaths, app_base.c_str())) + { + log_duplicate_property_error(coreclr_property_bag_t::common_property_to_string(common_property::AppPaths)); + return StatusCode::LibHostDuplicateProperty; + } + + if (!coreclr_properties.add(common_property::AppNIPaths, app_base.c_str())) + { + log_duplicate_property_error(coreclr_property_bag_t::common_property_to_string(common_property::AppNIPaths)); + return StatusCode::LibHostDuplicateProperty; + } } // Startup hooks pal::string_t startup_hooks; if (pal::getenv(_X("DOTNET_STARTUP_HOOKS"), &startup_hooks)) - coreclr_properties.add(common_property::StartUpHooks, startup_hooks.c_str()); + { + if (!coreclr_properties.add(common_property::StartUpHooks, startup_hooks.c_str())) + { + log_duplicate_property_error(coreclr_property_bag_t::common_property_to_string(common_property::StartUpHooks)); + return StatusCode::LibHostDuplicateProperty; + } + } return StatusCode::Success; } \ No newline at end of file diff --git a/src/installer/corehost/error_codes.h b/src/installer/corehost/error_codes.h index fb85187..e86e651 100644 --- a/src/installer/corehost/error_codes.h +++ b/src/installer/corehost/error_codes.h @@ -36,8 +36,9 @@ enum StatusCode SdkResolverResolveFailure = 0x8000809b, FrameworkCompatFailure = 0x8000809c, FrameworkCompatRetry = 0x8000809d, - AppHostExeNotBundle = 0x8000809e, - BundleExtractionFailure = 0x8000809f, - BundleExtractionIOError = 0x800080a0 + AppHostExeNotBundle = 0x8000809e, + BundleExtractionFailure = 0x8000809f, + BundleExtractionIOError = 0x800080a0, + LibHostDuplicateProperty = 0x800080a1, }; #endif // __ERROR_CODES_H__ diff --git a/src/installer/test/HostActivationTests/RuntimeProperties.cs b/src/installer/test/HostActivationTests/RuntimeProperties.cs index 15c0443..6010aa2 100644 --- a/src/installer/test/HostActivationTests/RuntimeProperties.cs +++ b/src/installer/test/HostActivationTests/RuntimeProperties.cs @@ -115,8 +115,7 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation .CaptureStdOut() .Execute() .Should().Fail() - // Update for: https://github.com/dotnet/core-setup/issues/5529 - .And.HaveStdErrContaining("Failed to create CoreCLR"); + .And.HaveStdErrContaining($"Duplicate runtime property found: {name}"); } public class SharedTestState : IDisposable -- 2.7.4