From 09cf838757859e156ce13863d7bb14324bb292aa Mon Sep 17 00:00:00 2001 From: Mateo Torres Ruiz Date: Thu, 29 Oct 2020 17:48:21 -0700 Subject: [PATCH 1/8] Introduce feature switch for startup hook --- docs/design/features/host-startup-hook.md | 4 ++++ docs/workflow/trimming/feature-switches.md | 1 + .../src/System/StartupHookProvider.cs | 9 +++++++++ .../src/ILLink/ILLink.Substitutions.Shared.xml | 3 +++ 4 files changed, 17 insertions(+) diff --git a/docs/design/features/host-startup-hook.md b/docs/design/features/host-startup-hook.md index e2b4623fab038b..21ff3eb86418ff 100644 --- a/docs/design/features/host-startup-hook.md +++ b/docs/design/features/host-startup-hook.md @@ -253,3 +253,7 @@ be defined either in the app or within the first hook that uses it: The type should be made `internal` to prevent exposing it as API surface to any managed code that happens to have access to the startup hook dll. However, the feature will also work if the type is `public`. + +### Linker unfriendly + +The usage of startup hooks on a trimmed app is potentially dangerous since these could make use of assemblies, types or members that were removed by the linker, causing the app to crash. \ No newline at end of file diff --git a/docs/workflow/trimming/feature-switches.md b/docs/workflow/trimming/feature-switches.md index 40d3a2299f474b..02647aa2339825 100644 --- a/docs/workflow/trimming/feature-switches.md +++ b/docs/workflow/trimming/feature-switches.md @@ -15,6 +15,7 @@ configurations but their defaults might vary as any SDK can set the defaults dif | InvariantGlobalization | System.Globalization.Invariant | All globalization specific code and data is trimmed when set to true | | UseSystemResourceKeys | System.Resources.UseSystemResourceKeys | Any localizable resources for system assemblies is trimmed when set to true | | HttpActivityPropagationSupport | System.Net.Http.EnableActivityPropagation | Any dependency related to diagnostics support for System.Net.Http is trimmed when set to false | +| StartupHookSupport | System.StartupHookProvider.IsSupported | Any StartupHook related logic is trimmed when set to false | Any feature-switch which defines property can be set in csproj file or on the command line as any other MSBuild property. Those without predefined property name diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs b/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs index ffdf388e43e335..365041ffc083a3 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System; using System.Diagnostics; using System.IO; using System.Reflection; @@ -14,6 +15,11 @@ internal static class StartupHookProvider private const string InitializeMethodName = "Initialize"; private const string DisallowedSimpleAssemblyNameSuffix = ".dll"; + private static bool IsSupported { get; } = StartupHookProviderIsSupported(); + + private static bool StartupHookProviderIsSupported() => + AppContext.TryGetSwitch("System.StartupHookProvider.IsSupported", out bool isSupported) ? isSupported : true; + private struct StartupHookNameOrPath { public AssemblyName AssemblyName; @@ -24,6 +30,9 @@ private struct StartupHookNameOrPath // containing a startup hook, and call each hook in turn. private static void ProcessStartupHooks() { + if (!IsSupported) + return; + // Initialize tracing before any user code can be called. System.Diagnostics.Tracing.RuntimeEventSource.Initialize(); diff --git a/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml b/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml index 9831ce9b6e2d2b..3b2cd77a1a5d1b 100644 --- a/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml +++ b/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml @@ -18,5 +18,8 @@ + + + From 70a401c9ddab785517c952e887c77e2f4edaaef9 Mon Sep 17 00:00:00 2001 From: Mateo Torres Ruiz Date: Fri, 30 Oct 2020 19:54:32 -0700 Subject: [PATCH 2/8] Add test --- .../HostActivation.Tests/RuntimeConfig.cs | 4 ++- .../HostActivation.Tests/StartupHooks.cs | 27 +++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/installer/tests/HostActivation.Tests/RuntimeConfig.cs b/src/installer/tests/HostActivation.Tests/RuntimeConfig.cs index 6e810dc5d7cde8..eaa95cb2dcced0 100644 --- a/src/installer/tests/HostActivation.Tests/RuntimeConfig.cs +++ b/src/installer/tests/HostActivation.Tests/RuntimeConfig.cs @@ -276,7 +276,9 @@ public void Save() JObject configProperties = new JObject(); foreach (var property in _properties) { - configProperties.Add(property.Item1, property.Item2); + var tokenValue = (property.Item2 == "false" || property.Item2 == "true") ? + JToken.Parse(property.Item2) : property.Item2; + configProperties.Add(property.Item1, tokenValue); } runtimeOptions.Add("configProperties", configProperties); diff --git a/src/installer/tests/HostActivation.Tests/StartupHooks.cs b/src/installer/tests/HostActivation.Tests/StartupHooks.cs index 12d9e23fc84a2c..3acb8b6656506c 100644 --- a/src/installer/tests/HostActivation.Tests/StartupHooks.cs +++ b/src/installer/tests/HostActivation.Tests/StartupHooks.cs @@ -13,6 +13,7 @@ public class StartupHooks : IClassFixture { private SharedTestState sharedTestState; private string startupHookVarName = "DOTNET_STARTUP_HOOKS"; + private string startupHookSupport = "System.StartupHookProvider.IsSupported"; public StartupHooks(StartupHooks.SharedTestState fixture) { @@ -639,6 +640,32 @@ public void Muxer_activation_of_StartupHook_With_Assembly_Resolver() .And.ExitWith(2); } + [Fact] + public void Muxer_activation_of_StartupHook_With_IsSupported_False() + { + var fixture = sharedTestState.PortableAppFixture.Copy(); + var dotnet = fixture.BuiltDotnet; + var appDll = fixture.TestProject.AppDll; + + var startupHookFixture = sharedTestState.StartupHookFixture.Copy(); + var startupHookDll = startupHookFixture.TestProject.AppDll; + + RuntimeConfig.FromFile(fixture.TestProject.RuntimeConfigJson) + .WithProperty(startupHookSupport, "false") + .Save(); + + // Startup hooks are not executed when the StartupHookSupport + // feature switch is set to false. + dotnet.Exec(appDll) + .EnvironmentVariable(startupHookVarName, startupHookDll) + .CaptureStdOut() + .CaptureStdErr() + .Execute() + .Should().Pass() + .And.NotHaveStdOutContaining("Hello from startup hook!") + .And.HaveStdOutContaining("Hello World"); + } + public class SharedTestState : IDisposable { // Entry point projects From d6082a670d25e9d2677545ed9b5113fd2bc31fa7 Mon Sep 17 00:00:00 2001 From: Mateo Torres Ruiz Date: Tue, 3 Nov 2020 13:53:08 -0800 Subject: [PATCH 3/8] Update md --- docs/design/features/host-startup-hook.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docs/design/features/host-startup-hook.md b/docs/design/features/host-startup-hook.md index 21ff3eb86418ff..989240e28dbbd8 100644 --- a/docs/design/features/host-startup-hook.md +++ b/docs/design/features/host-startup-hook.md @@ -256,4 +256,7 @@ hook dll. However, the feature will also work if the type is `public`. ### Linker unfriendly -The usage of startup hooks on a trimmed app is potentially dangerous since these could make use of assemblies, types or members that were removed by the linker, causing the app to crash. \ No newline at end of file +Startup hooks are disabled by default on trimmed apps. The usage of +startup hooks on a trimmed app is potentially dangerous since these +could make use of assemblies, types or members that were removed by +the linker, causing the app to crash. \ No newline at end of file From 0fded1c99102e1ff6c25d016786eafb312c84fdc Mon Sep 17 00:00:00 2001 From: Mateo Torres-Ruiz Date: Tue, 3 Nov 2020 14:51:45 -0800 Subject: [PATCH 4/8] Update src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs Co-authored-by: Jan Kotas --- .../System.Private.CoreLib/src/System/StartupHookProvider.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs b/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs index 365041ffc083a3..4a26ebf93bfb6f 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs @@ -15,10 +15,7 @@ internal static class StartupHookProvider private const string InitializeMethodName = "Initialize"; private const string DisallowedSimpleAssemblyNameSuffix = ".dll"; - private static bool IsSupported { get; } = StartupHookProviderIsSupported(); - - private static bool StartupHookProviderIsSupported() => - AppContext.TryGetSwitch("System.StartupHookProvider.IsSupported", out bool isSupported) ? isSupported : true; + private static bool IsSupported => AppContext.TryGetSwitch("System.StartupHookProvider.IsSupported", out bool isSupported) ? isSupported : true; private struct StartupHookNameOrPath { From 63ed112773256b032f2e3ed6737759720147a091 Mon Sep 17 00:00:00 2001 From: Mateo Torres Ruiz Date: Wed, 4 Nov 2020 14:04:48 -0800 Subject: [PATCH 5/8] Add UnreferencedCode attribute --- .../System.Private.CoreLib/src/System/StartupHookProvider.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs b/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs index 4a26ebf93bfb6f..4cba36f601e83e 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs @@ -3,6 +3,7 @@ using System; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.IO; using System.Reflection; using System.Runtime.Loader; @@ -102,6 +103,8 @@ private static void ProcessStartupHooks() // Load the specified assembly, and call the specified type's // "static void Initialize()" method. + [RequiresUnreferencedCode("Trimming may remove assemblies, types or members used by startup hooks. " + + "These can be disabled using the StartupHookSupport feature switch")] private static void CallStartupHook(StartupHookNameOrPath startupHook) { Assembly assembly; From 0cdc0f7ea9cb95eed09ef07b813d1458f3bd7e13 Mon Sep 17 00:00:00 2001 From: Mateo Torres-Ruiz Date: Tue, 10 Nov 2020 10:56:46 -0800 Subject: [PATCH 6/8] Apply suggestions from code review Co-authored-by: Vitek Karas --- docs/design/features/host-startup-hook.md | 4 ++-- docs/workflow/trimming/feature-switches.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/design/features/host-startup-hook.md b/docs/design/features/host-startup-hook.md index 989240e28dbbd8..ec0e35eb5d16bb 100644 --- a/docs/design/features/host-startup-hook.md +++ b/docs/design/features/host-startup-hook.md @@ -254,9 +254,9 @@ The type should be made `internal` to prevent exposing it as API surface to any managed code that happens to have access to the startup hook dll. However, the feature will also work if the type is `public`. -### Linker unfriendly +### Incompatible with trimming Startup hooks are disabled by default on trimmed apps. The usage of startup hooks on a trimmed app is potentially dangerous since these could make use of assemblies, types or members that were removed by -the linker, causing the app to crash. \ No newline at end of file +trimming, causing the app to crash. diff --git a/docs/workflow/trimming/feature-switches.md b/docs/workflow/trimming/feature-switches.md index 02647aa2339825..b0dadfd38cdf77 100644 --- a/docs/workflow/trimming/feature-switches.md +++ b/docs/workflow/trimming/feature-switches.md @@ -15,7 +15,7 @@ configurations but their defaults might vary as any SDK can set the defaults dif | InvariantGlobalization | System.Globalization.Invariant | All globalization specific code and data is trimmed when set to true | | UseSystemResourceKeys | System.Resources.UseSystemResourceKeys | Any localizable resources for system assemblies is trimmed when set to true | | HttpActivityPropagationSupport | System.Net.Http.EnableActivityPropagation | Any dependency related to diagnostics support for System.Net.Http is trimmed when set to false | -| StartupHookSupport | System.StartupHookProvider.IsSupported | Any StartupHook related logic is trimmed when set to false | +| StartupHookSupport | System.StartupHookProvider.IsSupported | Startup hooks are disabled when set to false. Startup hook related functionality can be trimmed. | Any feature-switch which defines property can be set in csproj file or on the command line as any other MSBuild property. Those without predefined property name From 0572274d49db20fa027efec2af872e3bd932157c Mon Sep 17 00:00:00 2001 From: Mateo Torres Ruiz Date: Tue, 10 Nov 2020 11:11:00 -0800 Subject: [PATCH 7/8] Update message --- .../System.Private.CoreLib/src/System/StartupHookProvider.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs b/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs index 4cba36f601e83e..1ad92333498e0a 100644 --- a/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs +++ b/src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs @@ -103,8 +103,8 @@ private static void ProcessStartupHooks() // Load the specified assembly, and call the specified type's // "static void Initialize()" method. - [RequiresUnreferencedCode("Trimming may remove assemblies, types or members used by startup hooks. " + - "These can be disabled using the StartupHookSupport feature switch")] + [RequiresUnreferencedCode("The StartupHookSupport feature switch has been enabled for this app which is being trimmed. " + + "Startup hook code is not observable by the trimmer and so required assemblies, types and members may be removed")] private static void CallStartupHook(StartupHookNameOrPath startupHook) { Assembly assembly; From 3ca5137b45ee8dde1dc12e60b039c929699df9fb Mon Sep 17 00:00:00 2001 From: Mateo Torres Ruiz Date: Fri, 13 Nov 2020 10:01:17 -0800 Subject: [PATCH 8/8] Update baseline --- .../src/ILLink/ILLink.Suppressions.Shared.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Suppressions.Shared.xml b/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Suppressions.Shared.xml index dda85d233600a9..f13541318ac62b 100644 --- a/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Suppressions.Shared.xml +++ b/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Suppressions.Shared.xml @@ -83,7 +83,7 @@ ILLink IL2026 member - M:System.StartupHookProvider.CallStartupHook(System.StartupHookProvider.StartupHookNameOrPath) + M:System.StartupHookProvider.ProcessStartupHooks() ILLink