From 773f65f9b57463fa553b8624d91c961546bc7652 Mon Sep 17 00:00:00 2001 From: Swaroop Sridhar Date: Wed, 2 Sep 2020 23:17:22 -0700 Subject: [PATCH 1/4] SingleFile: Enable hammer servicing This change enables hammer servicing to work on single-file apps. The runtime resolves assemblies in the bundle even before attempting to resolve using the TPA. This masks any entries in the TPA that correspond to hammer-serviced DLLs. This change fixes this problem: If a file is resolved to the servicing directory, it is disabled in the bundle directory. Testing: Added automated tests on Unix systems, manually tested on Windows. Fixes #36031 --- .../corehost/cli/bundle/file_entry.h | 6 ++ src/installer/corehost/cli/bundle/runner.cpp | 17 ++- src/installer/corehost/cli/bundle/runner.h | 5 +- src/installer/corehost/cli/deps_entry.cpp | 43 +++++--- src/installer/corehost/cli/deps_entry.h | 9 +- .../corehost/cli/hostpolicy/deps_resolver.cpp | 34 +++--- .../corehost/cli/hostpolicy/deps_resolver.h | 2 +- .../HammerServiceApp/HammerServiceApp.csproj | 15 +++ .../HammerServiceApp/Location/Class.cs | 9 ++ .../HammerServiceApp/Location/Location.csproj | 7 ++ .../TestProjects/HammerServiceApp/Program.cs | 17 +++ .../TestProjects/HammerServiceHelper/Class.cs | 9 ++ .../HammerServiceHelper.csproj | 9 ++ .../AppHost.Bundle.Tests/HammerServiceTest.cs | 100 ++++++++++++++++++ .../Helpers/BundleHelper.cs | 6 ++ 15 files changed, 250 insertions(+), 38 deletions(-) create mode 100644 src/installer/tests/Assets/TestProjects/HammerServiceApp/HammerServiceApp.csproj create mode 100644 src/installer/tests/Assets/TestProjects/HammerServiceApp/Location/Class.cs create mode 100644 src/installer/tests/Assets/TestProjects/HammerServiceApp/Location/Location.csproj create mode 100644 src/installer/tests/Assets/TestProjects/HammerServiceApp/Program.cs create mode 100644 src/installer/tests/Assets/TestProjects/HammerServiceHelper/Class.cs create mode 100644 src/installer/tests/Assets/TestProjects/HammerServiceHelper/HammerServiceHelper.csproj create mode 100644 src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/HammerServiceTest.cs diff --git a/src/installer/corehost/cli/bundle/file_entry.h b/src/installer/corehost/cli/bundle/file_entry.h index afdfcfef08a913..60631bc93f9686 100644 --- a/src/installer/corehost/cli/bundle/file_entry.h +++ b/src/installer/corehost/cli/bundle/file_entry.h @@ -37,11 +37,13 @@ namespace bundle , m_size(0) , m_type(file_type_t::__last) , m_relative_path() + , m_disabled(false) { } file_entry_t(const file_entry_fixed_t *fixed_data) :m_relative_path() + , m_disabled(false) { // File_entries in the bundle-manifest are expected to be used // beyond startup (for loading files directly from bundle, lazy extraction, etc.). @@ -57,7 +59,10 @@ namespace bundle int64_t offset() const { return m_offset; } int64_t size() const { return m_size; } file_type_t type() const { return m_type; } + void disable() { m_disabled = true; } + bool is_disabled() const { return m_disabled; } bool needs_extraction() const; + bool matches(const pal::string_t& path) const { return (pal::pathcmp(relative_path(), path) == 0) && !is_disabled(); } static file_entry_t read(reader_t &reader); @@ -66,6 +71,7 @@ namespace bundle int64_t m_size; file_type_t m_type; pal::string_t m_relative_path; // Path of an embedded file, relative to the extraction directory. + bool m_disabled; // Whether this entry is disabled, because an overriding hammer servicing entry exists. bool is_valid() const; }; } diff --git a/src/installer/corehost/cli/bundle/runner.cpp b/src/installer/corehost/cli/bundle/runner.cpp index bc0e1df2aadc39..91f0434597e80f 100644 --- a/src/installer/corehost/cli/bundle/runner.cpp +++ b/src/installer/corehost/cli/bundle/runner.cpp @@ -51,7 +51,7 @@ const file_entry_t* runner_t::probe(const pal::string_t &relative_path) const { for (const file_entry_t& entry : m_manifest.files) { - if (pal::pathcmp(entry.relative_path(), relative_path) == 0) + if (entry.matches(relative_path)) { return &entry; } @@ -74,7 +74,6 @@ bool runner_t::probe(const pal::string_t& relative_path, int64_t* offset, int64_ *offset = entry->offset(); *size = entry->size(); - return true; } @@ -96,3 +95,17 @@ bool runner_t::locate(const pal::string_t& relative_path, pal::string_t& full_pa return true; } +bool runner_t::disable(const pal::string_t& relative_path) +{ + for (file_entry_t& entry : m_manifest.files) + { + if (entry.matches(relative_path)) + { + entry.disable(); + return true; + } + } + + return false; +} + diff --git a/src/installer/corehost/cli/bundle/runner.h b/src/installer/corehost/cli/bundle/runner.h index 3aeddc4209a8e7..1cc6d3b03991ab 100644 --- a/src/installer/corehost/cli/bundle/runner.h +++ b/src/installer/corehost/cli/bundle/runner.h @@ -25,6 +25,7 @@ namespace bundle : info_t(bundle_path, app_path, header_offset) {} const pal::string_t& extraction_path() const { return m_extraction_path; } + bool has_base(const pal::string_t& base) const { return base.compare(base_path()) == 0; } bool probe(const pal::string_t& relative_path, int64_t* offset, int64_t* size) const; const file_entry_t* probe(const pal::string_t& relative_path) const; @@ -34,13 +35,15 @@ namespace bundle bool extracted_to_disk; return locate(relative_path, full_path, extracted_to_disk); } + bool disable(const pal::string_t& relative_path); static StatusCode process_manifest_and_extract() { - return ((runner_t*) the_app)->extract(); + return mutable_app()->extract(); } static const runner_t* app() { return (const runner_t*)the_app; } + static runner_t* mutable_app() { return (runner_t*)the_app; } private: diff --git a/src/installer/corehost/cli/deps_entry.cpp b/src/installer/corehost/cli/deps_entry.cpp index 305e999c76bfa7..87715ea2c51ed7 100644 --- a/src/installer/corehost/cli/deps_entry.cpp +++ b/src/installer/corehost/cli/deps_entry.cpp @@ -37,12 +37,12 @@ static pal::string_t normalize_dir_separator(const pal::string_t& path) // Returns: // If the file exists in the path relative to the "base" directory within the // single-file or on disk. -bool deps_entry_t::to_path(const pal::string_t& base, const pal::string_t& ietf_dir, bool look_in_base, bool look_in_bundle, pal::string_t* str, bool &loaded_from_bundle) const +bool deps_entry_t::to_path(const pal::string_t& base, const pal::string_t& ietf_dir, bool look_in_base, bool look_in_bundle, bool is_servicing, pal::string_t* str, bool &found_in_bundle) const { pal::string_t& candidate = *str; candidate.clear(); - loaded_from_bundle = false; + found_in_bundle = false; // Base directory must be present to obtain full path if (base.empty()) @@ -59,18 +59,19 @@ bool deps_entry_t::to_path(const pal::string_t& base, const pal::string_t& ietf_ pal::string_t sub_path = ietf_dir; append_path(&sub_path, file_path.c_str()); - if (look_in_bundle && bundle::info_t::is_single_file_bundle()) + // Don't look_in_bundle while probing a servicing location, because hammer servicing takes highest priority. + if (!is_servicing && look_in_bundle && bundle::info_t::is_single_file_bundle()) { const bundle::runner_t* app = bundle::runner_t::app(); - if (base.compare(app->base_path()) == 0) + if (app->has_base(base)) { // If sub_path is found in the single-file bundle, // app::locate() will set candidate to the full-path to the assembly extracted out to disk. bool extracted_to_disk = false; if (app->locate(sub_path, candidate, extracted_to_disk)) { - loaded_from_bundle = !extracted_to_disk; + found_in_bundle = !extracted_to_disk; trace::verbose(_X(" %s found in bundle [%s] %s"), sub_path.c_str(), candidate.c_str(), extracted_to_disk ? _X("(extracted)") : _X("")); return true; } @@ -99,6 +100,22 @@ bool deps_entry_t::to_path(const pal::string_t& base, const pal::string_t& ietf_ else { trace::verbose(_X(" %s path query exists %s"), query_type, candidate.c_str()); + + // If a file is resolved to the servicing directory, disable it in the bundle directory. + // This step is necessary because runtime resolves assemblies in the bundle even before + // attempting to resolve using the TPA. Disabling the file's entry in the bundle + // ensures that the servicing entry in the TPA gets priority. + if (is_servicing && bundle::info_t::is_single_file_bundle()) + { + bundle::runner_t* app = bundle::runner_t::mutable_app(); + assert(!app->has_base(base)); + assert(!found_in_bundle); + + if (app->disable(sub_path)) + { + trace::verbose(_X(" %s disabled in bundle because of servicing override %s"), sub_path.c_str(), candidate.c_str()); + } + } } return exists; @@ -114,7 +131,7 @@ bool deps_entry_t::to_path(const pal::string_t& base, const pal::string_t& ietf_ // Returns: // If the file exists in the path relative to the "base" directory. // -bool deps_entry_t::to_dir_path(const pal::string_t& base, bool look_in_bundle, pal::string_t* str, bool& loaded_from_bundle) const +bool deps_entry_t::to_dir_path(const pal::string_t& base, bool look_in_bundle, bool is_servicing, pal::string_t* str, bool& found_in_bundle) const { pal::string_t ietf_dir; @@ -136,7 +153,7 @@ bool deps_entry_t::to_dir_path(const pal::string_t& base, bool look_in_bundle, p base.c_str(), ietf_dir.c_str(), asset.name.c_str()); } - return to_path(base, ietf_dir, true, look_in_bundle, str, loaded_from_bundle); + return to_path(base, ietf_dir, true, look_in_bundle, is_servicing, str, found_in_bundle); } // ----------------------------------------------------------------------------- @@ -150,11 +167,11 @@ bool deps_entry_t::to_dir_path(const pal::string_t& base, bool look_in_bundle, p // Returns: // If the file exists in the path relative to the "base" directory. // -bool deps_entry_t::to_rel_path(const pal::string_t& base, bool look_in_bundle, pal::string_t* str) const +bool deps_entry_t::to_rel_path(const pal::string_t& base, bool look_in_bundle, bool is_servicing, pal::string_t* str) const { - bool loaded_from_bundle; - bool result = to_path(base, _X(""), false, look_in_bundle, str, loaded_from_bundle); - assert(!loaded_from_bundle); + bool found_in_bundle; + bool result = to_path(base, _X(""), false, look_in_bundle, is_servicing, str, found_in_bundle); + assert(!found_in_bundle); return result; } @@ -169,7 +186,7 @@ bool deps_entry_t::to_rel_path(const pal::string_t& base, bool look_in_bundle, p // Returns: // If the file exists in the path relative to the "base" directory. // -bool deps_entry_t::to_full_path(const pal::string_t& base, pal::string_t* str) const +bool deps_entry_t::to_full_path(const pal::string_t& base, bool is_servicing, pal::string_t* str) const { str->clear(); @@ -191,5 +208,5 @@ bool deps_entry_t::to_full_path(const pal::string_t& base, pal::string_t* str) c append_path(&new_base, library_path.c_str()); } - return to_rel_path(new_base, false, str); + return to_rel_path(new_base, false, is_servicing, str); } diff --git a/src/installer/corehost/cli/deps_entry.h b/src/installer/corehost/cli/deps_entry.h index 16bf84580c0b95..5c0d298f97b738 100644 --- a/src/installer/corehost/cli/deps_entry.h +++ b/src/installer/corehost/cli/deps_entry.h @@ -51,19 +51,20 @@ struct deps_entry_t bool is_serviceable; bool is_rid_specific; + // Given a "base" dir, yield the file path within this directory or single-file bundle. - bool to_dir_path(const pal::string_t& base, bool look_in_bundle, pal::string_t* str, bool& loaded_from_bundle) const; + bool to_dir_path(const pal::string_t& base, bool look_in_bundle, bool is_servicing, pal::string_t* str, bool& found_in_bundle) const; // Given a "base" dir, yield the relative path in the package layout. - bool to_rel_path(const pal::string_t& base, bool look_in_bundle, pal::string_t* str) const; + bool to_rel_path(const pal::string_t& base, bool look_in_bundle, bool is_servicing, pal::string_t* str) const; // Given a "base" dir, yield the relative path with package name, version in the package layout. - bool to_full_path(const pal::string_t& root, pal::string_t* str) const; + bool to_full_path(const pal::string_t& root, bool is_servicing, pal::string_t* str) const; private: // Given a "base" dir, yield the filepath within this directory or relative to this directory based on "look_in_base" // Returns a path within the single-file bundle, or a file on disk, - bool to_path(const pal::string_t& base, const pal::string_t& ietf_code, bool look_in_base, bool look_in_bundle, pal::string_t* str, bool & loaded_from_bundle) const; + bool to_path(const pal::string_t& base, const pal::string_t& ietf_code, bool look_in_base, bool look_in_bundle, bool is_servicing, pal::string_t* str, bool & found_in_bundle) const; }; #endif // __DEPS_ENTRY_H_ diff --git a/src/installer/corehost/cli/hostpolicy/deps_resolver.cpp b/src/installer/corehost/cli/hostpolicy/deps_resolver.cpp index 21c06a57205302..9c187d652ca1d8 100644 --- a/src/installer/corehost/cli/hostpolicy/deps_resolver.cpp +++ b/src/installer/corehost/cli/hostpolicy/deps_resolver.cpp @@ -281,12 +281,12 @@ void deps_resolver_t::setup_additional_probes(const std::vector& * -- When a deps json based probe is performed, the deps entry's package name and version must match. * -- When looking into a published dir, for rid specific assets lookup rid split folders; for non-rid assets lookup the layout dir. * The path to the resolved file is returned in candidate out parameter - * If the candidate is embedded within the single-file bundle (rather than an actual file on disk), loaded_from_bundle will be set to true. + * If the candidate is embedded within the single-file bundle (rather than an actual file on disk), found_in_bundle will be set to true. */ -bool deps_resolver_t::probe_deps_entry(const deps_entry_t& entry, const pal::string_t& deps_dir, int fx_level, pal::string_t* candidate, bool & loaded_from_bundle) +bool deps_resolver_t::probe_deps_entry(const deps_entry_t& entry, const pal::string_t& deps_dir, int fx_level, pal::string_t* candidate, bool & found_in_bundle) { candidate->clear(); - loaded_from_bundle = false; + found_in_bundle = false; for (const auto& config : m_probes) { @@ -318,9 +318,9 @@ bool deps_resolver_t::probe_deps_entry(const deps_entry_t& entry, const pal::str // If the deps json has the package name and version, then someone has already done rid selection and // put the right asset in the dir. So checking just package name and version would suffice. // No need to check further for the exact asset relative sub path. - if (config.probe_deps_json->has_package(entry.library_name, entry.library_version) && entry.to_dir_path(probe_dir, false, candidate, loaded_from_bundle)) + if (config.probe_deps_json->has_package(entry.library_name, entry.library_version) && entry.to_dir_path(probe_dir, false, config.only_serviceable_assets, candidate, found_in_bundle)) { - assert(!loaded_from_bundle); + assert(!found_in_bundle); trace::verbose(_X(" Probed deps json and matched '%s'"), candidate->c_str()); return true; } @@ -337,7 +337,7 @@ bool deps_resolver_t::probe_deps_entry(const deps_entry_t& entry, const pal::str { if (entry.is_rid_specific) { - if (entry.to_rel_path(deps_dir, true, candidate)) + if (entry.to_rel_path(deps_dir, true, config.only_serviceable_assets, candidate)) { trace::verbose(_X(" Probed deps dir and matched '%s'"), candidate->c_str()); return true; @@ -346,7 +346,7 @@ bool deps_resolver_t::probe_deps_entry(const deps_entry_t& entry, const pal::str else { // Non-rid assets, lookup in the published dir. - if (entry.to_dir_path(deps_dir, true, candidate, loaded_from_bundle)) + if (entry.to_dir_path(deps_dir, true, config.only_serviceable_assets, candidate, found_in_bundle)) { trace::verbose(_X(" Probed deps dir and matched '%s'"), candidate->c_str()); return true; @@ -356,7 +356,7 @@ bool deps_resolver_t::probe_deps_entry(const deps_entry_t& entry, const pal::str trace::verbose(_X(" Skipping... not found in deps dir '%s'"), deps_dir.c_str()); } - else if (entry.to_full_path(probe_dir, candidate)) + else if (entry.to_full_path(probe_dir, config.only_serviceable_assets, candidate)) { trace::verbose(_X(" Probed package dir and matched '%s'"), candidate->c_str()); return true; @@ -440,12 +440,12 @@ bool deps_resolver_t::resolve_tpa_list( name_to_resolved_asset_map_t::iterator existing = items.find(entry.asset.name); if (existing == items.end()) { - bool loaded_from_bundle = false; - if (probe_deps_entry(entry, deps_dir, fx_level, &resolved_path, loaded_from_bundle)) + bool found_in_bundle = false; + if (probe_deps_entry(entry, deps_dir, fx_level, &resolved_path, found_in_bundle)) { // Assemblies loaded directly from the bundle are not added to the TPA list. // The runtime directly probes the bundle-manifest using a host-callback. - if (!loaded_from_bundle) + if (!found_in_bundle) { deps_resolved_asset_t resolved_asset(entry.asset, resolved_path); add_tpa_asset(resolved_asset, &items); @@ -478,8 +478,8 @@ bool deps_resolver_t::resolve_tpa_list( if (entry.asset.assembly_version > existing_entry->asset.assembly_version || (entry.asset.assembly_version == existing_entry->asset.assembly_version && entry.asset.file_version >= existing_entry->asset.file_version)) { - bool loaded_from_bundle = false; - if (probe_deps_entry(entry, deps_dir, fx_level, &resolved_path, loaded_from_bundle)) + bool found_in_bundle = false; + if (probe_deps_entry(entry, deps_dir, fx_level, &resolved_path, found_in_bundle)) { // If the path is the same, then no need to replace if (resolved_path != existing_entry->resolved_path) @@ -491,7 +491,7 @@ bool deps_resolver_t::resolve_tpa_list( existing_entry = nullptr; items.erase(existing); - if (!loaded_from_bundle) + if (!found_in_bundle) { deps_asset_t asset(entry.asset.name, entry.asset.relative_path, entry.asset.assembly_version, entry.asset.file_version); deps_resolved_asset_t resolved_asset(asset, resolved_path); @@ -805,10 +805,10 @@ bool deps_resolver_t::resolve_probe_dirs( trace::verbose(_X("Processing native/culture for deps entry [%s, %s, %s]"), entry.library_name.c_str(), entry.library_version.c_str(), entry.asset.relative_path.c_str()); - bool loaded_from_bundle = false; - if (probe_deps_entry(entry, deps_dir, fx_level, &candidate, loaded_from_bundle)) + bool found_in_bundle = false; + if (probe_deps_entry(entry, deps_dir, fx_level, &candidate, found_in_bundle)) { - if (!loaded_from_bundle) + if (!found_in_bundle) { init_known_entry_path(entry, candidate); add_unique_path(asset_type, action(candidate), &items, output, &non_serviced, core_servicing); diff --git a/src/installer/corehost/cli/hostpolicy/deps_resolver.h b/src/installer/corehost/cli/hostpolicy/deps_resolver.h index 6798a50fba605e..6ec332e807f465 100644 --- a/src/installer/corehost/cli/hostpolicy/deps_resolver.h +++ b/src/installer/corehost/cli/hostpolicy/deps_resolver.h @@ -228,7 +228,7 @@ class deps_resolver_t const pal::string_t& deps_dir, int fx_level, pal::string_t* candidate, - bool &loaded_from_bundle); + bool &found_in_bundle); fx_definition_vector_t& m_fx_definitions; diff --git a/src/installer/tests/Assets/TestProjects/HammerServiceApp/HammerServiceApp.csproj b/src/installer/tests/Assets/TestProjects/HammerServiceApp/HammerServiceApp.csproj new file mode 100644 index 00000000000000..94dc6c8359f7c9 --- /dev/null +++ b/src/installer/tests/Assets/TestProjects/HammerServiceApp/HammerServiceApp.csproj @@ -0,0 +1,15 @@ + + + + $(NetCoreAppCurrent) + Exe + $(TestTargetRid) + $(MNAVersion) + + + + + + + + diff --git a/src/installer/tests/Assets/TestProjects/HammerServiceApp/Location/Class.cs b/src/installer/tests/Assets/TestProjects/HammerServiceApp/Location/Class.cs new file mode 100644 index 00000000000000..1b784651c1adaf --- /dev/null +++ b/src/installer/tests/Assets/TestProjects/HammerServiceApp/Location/Class.cs @@ -0,0 +1,9 @@ +using System; + +namespace GPS +{ + public class Location + { + public static string City => "Bellevue"; + } +} diff --git a/src/installer/tests/Assets/TestProjects/HammerServiceApp/Location/Location.csproj b/src/installer/tests/Assets/TestProjects/HammerServiceApp/Location/Location.csproj new file mode 100644 index 00000000000000..f208d303c9811f --- /dev/null +++ b/src/installer/tests/Assets/TestProjects/HammerServiceApp/Location/Location.csproj @@ -0,0 +1,7 @@ + + + + net5.0 + + + diff --git a/src/installer/tests/Assets/TestProjects/HammerServiceApp/Program.cs b/src/installer/tests/Assets/TestProjects/HammerServiceApp/Program.cs new file mode 100644 index 00000000000000..92697d2c95b04f --- /dev/null +++ b/src/installer/tests/Assets/TestProjects/HammerServiceApp/Program.cs @@ -0,0 +1,17 @@ +using System; +using System.Reflection; + +namespace hammer +{ + class Program + { + static void Main(string[] args) + { + var asm = Assembly.Load("Location, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null"); + var location = asm.GetType("GPS.Location"); + var city = location.GetProperty("City"); + var cityName = city.GetValue(null); + Console.WriteLine($"Hi {cityName}!"); + } + } +} diff --git a/src/installer/tests/Assets/TestProjects/HammerServiceHelper/Class.cs b/src/installer/tests/Assets/TestProjects/HammerServiceHelper/Class.cs new file mode 100644 index 00000000000000..2d8ded5990be59 --- /dev/null +++ b/src/installer/tests/Assets/TestProjects/HammerServiceHelper/Class.cs @@ -0,0 +1,9 @@ +using System; + +namespace GPS +{ + public class Location + { + public static string City => "Bengaluru"; + } +} diff --git a/src/installer/tests/Assets/TestProjects/HammerServiceHelper/HammerServiceHelper.csproj b/src/installer/tests/Assets/TestProjects/HammerServiceHelper/HammerServiceHelper.csproj new file mode 100644 index 00000000000000..5abe74a4512f01 --- /dev/null +++ b/src/installer/tests/Assets/TestProjects/HammerServiceHelper/HammerServiceHelper.csproj @@ -0,0 +1,9 @@ + + + + $(NetCoreAppCurrent) + $(MNAVersion) + Location + + + diff --git a/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/HammerServiceTest.cs b/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/HammerServiceTest.cs new file mode 100644 index 00000000000000..44cbe061da8d79 --- /dev/null +++ b/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/HammerServiceTest.cs @@ -0,0 +1,100 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using BundleTests.Helpers; +using Microsoft.DotNet.Cli.Build.Framework; +using Microsoft.DotNet.CoreSetup.Test; +using System; +using System.IO; +using System.Runtime.InteropServices; +using Xunit; + +namespace AppHost.Bundle.Tests +{ + public class HammerServiceTest : IClassFixture + { + private SharedTestState sharedTestState; + + public HammerServiceTest(SharedTestState fixture) + { + sharedTestState = fixture; + } + + [Fact] + private void SingleFile_Apps_Are_Serviced() + { + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + // On Windows, the hammer servicing location is %ProgramFiles%\coreservicing. + // Since writing to this location requires administrative privilege, we do not run the test on Windows. + // On Unix systems, the servicing location is obtained from the environment variable $CORE_SERVICING. + + return; + } + + var fixture = sharedTestState.TestFixture.Copy(); + var servicer = sharedTestState.ServiceFixture.Copy(); + + // Annotate the app as servicible, and then publish to a single-file. + string depsjson = BundleHelper.GetDepsJsonPath(fixture); + File.WriteAllText(depsjson, File.ReadAllText(depsjson).Replace("\"serviceable\": false", "\"serviceable\": true")); + var singleFile = BundleHelper.BundleApp(fixture); + + // Create the servicing directory, and copy the servived DLL from service fixture to the servicing directory. + var serviceBasePath = Path.Combine(fixture.TestProject.ProjectDirectory, "coreservicing"); + var servicePath = Path.Combine(serviceBasePath, "pkgs", BundleHelper.GetAppBaseName(servicer), "1.0.0"); + Directory.CreateDirectory(servicePath); + File.Copy(BundleHelper.GetAppPath(servicer), Path.Combine(servicePath, BundleHelper.GetAppName(servicer))); + + // Verify that the test DLL is loaded from the bundle when not being serviced + Command.Create(singleFile) + .CaptureStdErr() + .CaptureStdOut() + .Execute() + .Should() + .Pass() + .And + .HaveStdOutContaining("Hi Bellevue!"); + + // Verify that the test DLL is loaded from the servicing location when being serviced + Command.Create(singleFile) + .CaptureStdErr() + .CaptureStdOut() + .EnvironmentVariable(BundleHelper.CoreServicingEnvVariable, serviceBasePath) + .Execute() + .Should() + .Pass() + .And + .HaveStdOutContaining("Hi Bengaluru!"); + } + + public class SharedTestState : IDisposable + { + public TestProjectFixture TestFixture { get; set; } + public TestProjectFixture ServiceFixture { get; set; } + public RepoDirectoriesProvider RepoDirectories { get; set; } + + public SharedTestState() + { + RepoDirectories = new RepoDirectoriesProvider(); + TestFixture = new TestProjectFixture("HammerServiceApp", RepoDirectories); + TestFixture + .EnsureRestoredForRid(TestFixture.CurrentRid, RepoDirectories.CorehostPackages) + .PublishProject(runtime: TestFixture.CurrentRid, + outputDirectory: BundleHelper.GetPublishPath(TestFixture)); + + ServiceFixture = new TestProjectFixture("HammerServiceHelper", RepoDirectories, assemblyName: "Location"); + ServiceFixture + .EnsureRestored(RepoDirectories.CorehostPackages) + .PublishProject(outputDirectory: BundleHelper.GetPublishPath(ServiceFixture)); + + } + + public void Dispose() + { + TestFixture.Dispose(); + ServiceFixture.Dispose(); + } + } + } +} diff --git a/src/installer/tests/Microsoft.NET.HostModel.Tests/Helpers/BundleHelper.cs b/src/installer/tests/Microsoft.NET.HostModel.Tests/Helpers/BundleHelper.cs index e147a8351b03a4..5f57edab365e8e 100644 --- a/src/installer/tests/Microsoft.NET.HostModel.Tests/Helpers/BundleHelper.cs +++ b/src/installer/tests/Microsoft.NET.HostModel.Tests/Helpers/BundleHelper.cs @@ -13,6 +13,7 @@ namespace BundleTests.Helpers public static class BundleHelper { public const string DotnetBundleExtractBaseEnvVariable = "DOTNET_BUNDLE_EXTRACT_BASE_DIR"; + public const string CoreServicingEnvVariable = "CORE_SERVICING"; public static string GetHostPath(TestProjectFixture fixture) { @@ -24,6 +25,11 @@ public static string GetAppPath(TestProjectFixture fixture) return Path.Combine(GetPublishPath(fixture), GetAppName(fixture)); } + public static string GetDepsJsonPath(TestProjectFixture fixture) + { + return Path.Combine(GetPublishPath(fixture), $"{GetAppBaseName(fixture)}.deps.json"); + } + public static string GetPublishedSingleFilePath(TestProjectFixture fixture) { return GetHostPath(fixture); From c4909c9a6925b6ffafcbd49bb937d2721a4be248 Mon Sep 17 00:00:00 2001 From: Swaroop Sridhar Date: Fri, 4 Sep 2020 11:28:20 -0700 Subject: [PATCH 2/4] Update comments in src/installer/corehost/cli/bundle/file_entry.h Co-authored-by: Vitek Karas --- src/installer/corehost/cli/bundle/file_entry.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/installer/corehost/cli/bundle/file_entry.h b/src/installer/corehost/cli/bundle/file_entry.h index 60631bc93f9686..9d45eaecb317fd 100644 --- a/src/installer/corehost/cli/bundle/file_entry.h +++ b/src/installer/corehost/cli/bundle/file_entry.h @@ -71,7 +71,11 @@ namespace bundle int64_t m_size; file_type_t m_type; pal::string_t m_relative_path; // Path of an embedded file, relative to the extraction directory. - bool m_disabled; // Whether this entry is disabled, because an overriding hammer servicing entry exists. + // If the file represented by this entry is also found in a servicing location, the servicing location must take precedence. + // But in general, bundle will take precedence over on-disk locations everywhere. + // So in order to make sure that the servicing location is used, the file entry in the bundle is marked as "disabled" + // in such case, and the lookup logic will behave as if the file is not present in the bundle. + bool m_disabled; bool is_valid() const; }; } From 1fc7054ca891eee92b5972a9e07285f12df823e5 Mon Sep 17 00:00:00 2001 From: Swaroop Sridhar Date: Fri, 4 Sep 2020 11:28:44 -0700 Subject: [PATCH 3/4] Update comments in src/installer/corehost/cli/deps_entry.cpp Co-authored-by: Vitek Karas --- src/installer/corehost/cli/deps_entry.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/installer/corehost/cli/deps_entry.cpp b/src/installer/corehost/cli/deps_entry.cpp index 87715ea2c51ed7..cd84ffb30852c1 100644 --- a/src/installer/corehost/cli/deps_entry.cpp +++ b/src/installer/corehost/cli/deps_entry.cpp @@ -101,9 +101,10 @@ bool deps_entry_t::to_path(const pal::string_t& base, const pal::string_t& ietf_ { trace::verbose(_X(" %s path query exists %s"), query_type, candidate.c_str()); - // If a file is resolved to the servicing directory, disable it in the bundle directory. - // This step is necessary because runtime resolves assemblies in the bundle even before - // attempting to resolve using the TPA. Disabling the file's entry in the bundle + // If a file is resolved to the servicing directory, mark it as disabled in the bundle. + // This step is necessary because runtime will try to resolve assemblies from the bundle + // before it uses the TPA. So putting the servicing entry into TPA is not enough, since runtime would + // resolve it from the bundle first anyway. Disabling the file's entry in the bundle // ensures that the servicing entry in the TPA gets priority. if (is_servicing && bundle::info_t::is_single_file_bundle()) { From 4cc9b150528d74d76f19d3c707c1fc9fd5963189 Mon Sep 17 00:00:00 2001 From: Swaroop Sridhar Date: Fri, 4 Sep 2020 13:18:48 -0700 Subject: [PATCH 4/4] Address Code Review Feedback --- src/installer/corehost/cli/bundle/runner.cpp | 4 ++++ src/installer/corehost/cli/deps_entry.cpp | 20 +++++++++++++------ src/installer/corehost/cli/deps_entry.h | 10 +++++----- .../corehost/cli/hostpolicy/deps_resolver.cpp | 6 +++--- .../Class.cs | 0 .../ServicedLocation.csproj} | 0 .../AppHost.Bundle.Tests/HammerServiceTest.cs | 2 +- 7 files changed, 27 insertions(+), 15 deletions(-) rename src/installer/tests/Assets/TestProjects/{HammerServiceHelper => ServicedLocation}/Class.cs (100%) rename src/installer/tests/Assets/TestProjects/{HammerServiceHelper/HammerServiceHelper.csproj => ServicedLocation/ServicedLocation.csproj} (100%) diff --git a/src/installer/corehost/cli/bundle/runner.cpp b/src/installer/corehost/cli/bundle/runner.cpp index 91f0434597e80f..012274cc9ed0e6 100644 --- a/src/installer/corehost/cli/bundle/runner.cpp +++ b/src/installer/corehost/cli/bundle/runner.cpp @@ -53,6 +53,7 @@ const file_entry_t* runner_t::probe(const pal::string_t &relative_path) const { if (entry.matches(relative_path)) { + assert(!entry.is_disabled()); return &entry; } } @@ -69,6 +70,7 @@ bool runner_t::probe(const pal::string_t& relative_path, int64_t* offset, int64_ return false; } + assert(!entry->is_disabled()); assert(entry->offset() != 0); *offset = entry->offset(); @@ -87,6 +89,8 @@ bool runner_t::locate(const pal::string_t& relative_path, pal::string_t& full_pa return false; } + assert(!entry->is_disabled()); + extracted_to_disk = entry->needs_extraction(); full_path.assign(extracted_to_disk ? extraction_path() : base_path()); diff --git a/src/installer/corehost/cli/deps_entry.cpp b/src/installer/corehost/cli/deps_entry.cpp index cd84ffb30852c1..58c23a3f0d8321 100644 --- a/src/installer/corehost/cli/deps_entry.cpp +++ b/src/installer/corehost/cli/deps_entry.cpp @@ -32,11 +32,14 @@ static pal::string_t normalize_dir_separator(const pal::string_t& path) // ietf_dir - If this is a resource asset, the IETF intermediate directory // look_in_base - Whether to search as a relative path // look_in_bundle - Whether to look within the single-file bundle -// str - If the method returns true, contains the file path for this deps entry +// is_servicing - Whether the base directory is the core-servicing directory +// str - (out parameter) If the method returns true, contains the file path for this deps entry +// found_in_bundle - (out parameter) True if the candidate is located within the single-file bundle. // // Returns: // If the file exists in the path relative to the "base" directory within the // single-file or on disk. + bool deps_entry_t::to_path(const pal::string_t& base, const pal::string_t& ietf_dir, bool look_in_base, bool look_in_bundle, bool is_servicing, pal::string_t* str, bool &found_in_bundle) const { pal::string_t& candidate = *str; @@ -59,8 +62,9 @@ bool deps_entry_t::to_path(const pal::string_t& base, const pal::string_t& ietf_ pal::string_t sub_path = ietf_dir; append_path(&sub_path, file_path.c_str()); - // Don't look_in_bundle while probing a servicing location, because hammer servicing takes highest priority. - if (!is_servicing && look_in_bundle && bundle::info_t::is_single_file_bundle()) + assert(!is_servicing || !look_in_bundle); + + if (look_in_bundle && bundle::info_t::is_single_file_bundle()) { const bundle::runner_t* app = bundle::runner_t::app(); @@ -128,11 +132,12 @@ bool deps_entry_t::to_path(const pal::string_t& base, const pal::string_t& ietf_ // Parameters: // base - The base directory to look for the relative path of this entry // str - If the method returns true, contains the file path for this deps entry +// look_in_bundle - Whether to look within the single-file bundle // // Returns: // If the file exists in the path relative to the "base" directory. // -bool deps_entry_t::to_dir_path(const pal::string_t& base, bool look_in_bundle, bool is_servicing, pal::string_t* str, bool& found_in_bundle) const +bool deps_entry_t::to_dir_path(const pal::string_t& base, bool look_in_bundle, pal::string_t* str, bool& found_in_bundle) const { pal::string_t ietf_dir; @@ -154,16 +159,18 @@ bool deps_entry_t::to_dir_path(const pal::string_t& base, bool look_in_bundle, b base.c_str(), ietf_dir.c_str(), asset.name.c_str()); } - return to_path(base, ietf_dir, true, look_in_bundle, is_servicing, str, found_in_bundle); + return to_path(base, ietf_dir, true, look_in_bundle, false, str, found_in_bundle); } // ----------------------------------------------------------------------------- // Given a "base" directory, yield the relative path of this file in the package -// layout. +// layout or servicing location. // // Parameters: // base - The base directory to look for the relative path of this entry // str - If the method returns true, contains the file path for this deps entry +// look_in_bundle - Whether to look within the single-file bundle +// is_servicing - Whether the base directory is the core-servicing directory // // Returns: // If the file exists in the path relative to the "base" directory. @@ -183,6 +190,7 @@ bool deps_entry_t::to_rel_path(const pal::string_t& base, bool look_in_bundle, b // Parameters: // base - The base directory to look for the relative path of this entry // str - If the method returns true, contains the file path for this deps entry +// is_servicing - Whether the base directory is the core-servicing directory // // Returns: // If the file exists in the path relative to the "base" directory. diff --git a/src/installer/corehost/cli/deps_entry.h b/src/installer/corehost/cli/deps_entry.h index 5c0d298f97b738..db2a32e27d278d 100644 --- a/src/installer/corehost/cli/deps_entry.h +++ b/src/installer/corehost/cli/deps_entry.h @@ -51,20 +51,20 @@ struct deps_entry_t bool is_serviceable; bool is_rid_specific; - // Given a "base" dir, yield the file path within this directory or single-file bundle. - bool to_dir_path(const pal::string_t& base, bool look_in_bundle, bool is_servicing, pal::string_t* str, bool& found_in_bundle) const; + bool to_dir_path(const pal::string_t& base, bool look_in_bundle, pal::string_t* str, bool& found_in_bundle) const; - // Given a "base" dir, yield the relative path in the package layout. + // Given a "base" dir, yield the relative path in the package layout or servicing directory. bool to_rel_path(const pal::string_t& base, bool look_in_bundle, bool is_servicing, pal::string_t* str) const; - // Given a "base" dir, yield the relative path with package name, version in the package layout. - bool to_full_path(const pal::string_t& root, bool is_servicing, pal::string_t* str) const; + // Given a "base" dir, yield the relative path with package name/version in the package layout or servicing location. + bool to_full_path(const pal::string_t& base, bool is_servicing, pal::string_t* str) const; private: // Given a "base" dir, yield the filepath within this directory or relative to this directory based on "look_in_base" // Returns a path within the single-file bundle, or a file on disk, bool to_path(const pal::string_t& base, const pal::string_t& ietf_code, bool look_in_base, bool look_in_bundle, bool is_servicing, pal::string_t* str, bool & found_in_bundle) const; + }; #endif // __DEPS_ENTRY_H_ diff --git a/src/installer/corehost/cli/hostpolicy/deps_resolver.cpp b/src/installer/corehost/cli/hostpolicy/deps_resolver.cpp index 9c187d652ca1d8..dfe77ce54e3fd2 100644 --- a/src/installer/corehost/cli/hostpolicy/deps_resolver.cpp +++ b/src/installer/corehost/cli/hostpolicy/deps_resolver.cpp @@ -318,7 +318,7 @@ bool deps_resolver_t::probe_deps_entry(const deps_entry_t& entry, const pal::str // If the deps json has the package name and version, then someone has already done rid selection and // put the right asset in the dir. So checking just package name and version would suffice. // No need to check further for the exact asset relative sub path. - if (config.probe_deps_json->has_package(entry.library_name, entry.library_version) && entry.to_dir_path(probe_dir, false, config.only_serviceable_assets, candidate, found_in_bundle)) + if (config.probe_deps_json->has_package(entry.library_name, entry.library_version) && entry.to_dir_path(probe_dir, false, candidate, found_in_bundle)) { assert(!found_in_bundle); trace::verbose(_X(" Probed deps json and matched '%s'"), candidate->c_str()); @@ -337,7 +337,7 @@ bool deps_resolver_t::probe_deps_entry(const deps_entry_t& entry, const pal::str { if (entry.is_rid_specific) { - if (entry.to_rel_path(deps_dir, true, config.only_serviceable_assets, candidate)) + if (entry.to_rel_path(deps_dir, true, false, candidate)) { trace::verbose(_X(" Probed deps dir and matched '%s'"), candidate->c_str()); return true; @@ -346,7 +346,7 @@ bool deps_resolver_t::probe_deps_entry(const deps_entry_t& entry, const pal::str else { // Non-rid assets, lookup in the published dir. - if (entry.to_dir_path(deps_dir, true, config.only_serviceable_assets, candidate, found_in_bundle)) + if (entry.to_dir_path(deps_dir, true, candidate, found_in_bundle)) { trace::verbose(_X(" Probed deps dir and matched '%s'"), candidate->c_str()); return true; diff --git a/src/installer/tests/Assets/TestProjects/HammerServiceHelper/Class.cs b/src/installer/tests/Assets/TestProjects/ServicedLocation/Class.cs similarity index 100% rename from src/installer/tests/Assets/TestProjects/HammerServiceHelper/Class.cs rename to src/installer/tests/Assets/TestProjects/ServicedLocation/Class.cs diff --git a/src/installer/tests/Assets/TestProjects/HammerServiceHelper/HammerServiceHelper.csproj b/src/installer/tests/Assets/TestProjects/ServicedLocation/ServicedLocation.csproj similarity index 100% rename from src/installer/tests/Assets/TestProjects/HammerServiceHelper/HammerServiceHelper.csproj rename to src/installer/tests/Assets/TestProjects/ServicedLocation/ServicedLocation.csproj diff --git a/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/HammerServiceTest.cs b/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/HammerServiceTest.cs index 44cbe061da8d79..5c0206573c47da 100644 --- a/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/HammerServiceTest.cs +++ b/src/installer/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/HammerServiceTest.cs @@ -83,7 +83,7 @@ public SharedTestState() .PublishProject(runtime: TestFixture.CurrentRid, outputDirectory: BundleHelper.GetPublishPath(TestFixture)); - ServiceFixture = new TestProjectFixture("HammerServiceHelper", RepoDirectories, assemblyName: "Location"); + ServiceFixture = new TestProjectFixture("ServicedLocation", RepoDirectories, assemblyName: "Location"); ServiceFixture .EnsureRestored(RepoDirectories.CorehostPackages) .PublishProject(outputDirectory: BundleHelper.GetPublishPath(ServiceFixture));