Fail early on duplicate runtime properties (dotnet/core-setup#5898)
authorElinor Fung <47805090+elinor-fung@users.noreply.github.com>
Thu, 18 Apr 2019 06:12:50 +0000 (23:12 -0700)
committerGitHub <noreply@github.com>
Thu, 18 Apr 2019 06:12:50 +0000 (23:12 -0700)
Commit migrated from https://github.com/dotnet/core-setup/commit/859ee7d256a043b0a9bfe3e9cc6c8483eb23efd5

src/installer/corehost/cli/hostpolicy/coreclr.cpp
src/installer/corehost/cli/hostpolicy/coreclr.h
src/installer/corehost/cli/hostpolicy/hostpolicy_context.cpp
src/installer/corehost/error_codes.h
src/installer/test/HostActivationTests/RuntimeProperties.cs

index d9282e5..db90019 100644 (file)
@@ -99,12 +99,14 @@ pal::hresult_t coreclr_t::create(
     std::vector<const char*> keys(propertyCount);
     std::vector<std::vector<char>> values_strs(propertyCount);
     std::vector<const char*> values(propertyCount);
-    std::function<void (int,const pal::string_t &,const pal::string_t &)> callback = [&] (int index, const pal::string_t& key, const pal::string_t& value)
+    int index = 0;
+    std::function<void (const pal::string_t &,const pal::string_t &)> 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<size_t>(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<int>(key);
+    assert(0 <= idx && idx < static_cast<int>(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<size_t>(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<int>(key);
     assert(0 <= idx && idx < static_cast<int>(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<int>(_keys.size());
+    return static_cast<int>(_properties.size());
 }
 
-void coreclr_property_bag_t::enumerate(std::function<void(int, const pal::string_t&, const pal::string_t&)> &callback) const
+void coreclr_property_bag_t::enumerate(std::function<void(const pal::string_t&, const pal::string_t&)> &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
index 06cce20..7227eac 100644 (file)
@@ -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<void(int, const pal::string_t&, const pal::string_t&)> &callback) const;
+    void enumerate(std::function<void(const pal::string_t&, const pal::string_t&)> &callback) const;
 
 private:
-    std::vector<pal::string_t> _keys;
-    std::vector<pal::string_t> _values;
+    std::unordered_map<pal::string_t, pal::string_t> _properties;
 };
 
 #endif // _COREHOST_CLI_CORECLR_H_
index 15dbba9..aa933d7 100644 (file)
@@ -8,6 +8,15 @@
 #include <error_codes.h>
 #include <trace.h>
 
+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
index fb85187..e86e651 100644 (file)
@@ -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__
index 15c0443..6010aa2 100644 (file)
@@ -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