-
Notifications
You must be signed in to change notification settings - Fork 47
Fixes for Partial Trust #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| using System.Security; | ||
|
|
||
| #if !NET35 | ||
| [assembly: AllowPartiallyTrustedCallers] | ||
| #if !PORTABLE | ||
| [assembly: SecurityRules(SecurityRuleSet.Level2)] | ||
| #endif | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,6 +79,7 @@ private AsyncVoidMethodBuilder(SynchronizationContext synchronizationContext) | |
| /// <summary> | ||
| /// Registers with UnobservedTaskException to suppress exception crashing. | ||
| /// </summary> | ||
| [SecuritySafeCritical] | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have a test that fails when this line is removed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll see what I can do. As I recall, without it, secannotate reports a transparency violation. Theoretically it will throw TypeLoadException if you try to use it in partial trust.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line is only relevant in a .Net 4.5 environment where you're loading a library built with AsyncBridge. In 4.5, TaskScheduler.UnobservedTaskException is marked SecurityCritical, causing it to throw TypeLoadException. AsyncBridge currently has a blind catch that swallows the exception. It won't automatically observe exceptions any more, but I think that's fine, since on 4.5 it won't bring down the AppDomain unless the application sets ThrowOnUnobservedTaskExceptions. If it does, that's what you'd expect anyway. We obviously want to fix this, since it's an exception that can be avoided. But it makes me wonder about the opposite scenario -- full trust on 4.5. With AsyncBridge as it is, even if the application sets ThrowOnUnobservedTaskExceptions, it will never throw because we automatically observe everything. It also introduces heisenbugs, whereby depending on whether you've run a 4.0 async method determines whether the application fails or silently swallows them. The only solution would be to somehow detect if we're running on 4.5 and not add the event handler. I'm not sure how we can do that, or if we event want to.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's enlightening, thanks! I don't like the blind catch. Is there a way we could get rid of it on all platforms? |
||
| internal static void PreventUnobservedTaskExceptions() | ||
| { | ||
| if (Interlocked.CompareExchange(ref s_preventUnobservedTaskExceptionsInvoked, 1, 0) != 0) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -254,17 +254,27 @@ private static void RunNoException(Action continuation) | |
| /// Copies the exception's stack trace so its stack trace isn't overwritten. | ||
| /// </summary> | ||
| /// <param name="exc"> The exception to prepare. </param> | ||
| [SecuritySafeCritical] | ||
| internal static Exception PrepareExceptionForRethrow(Exception exc) | ||
| { | ||
| if (s_prepForRemoting != null) | ||
| { | ||
| try | ||
| { | ||
| s_prepForRemoting.Invoke(exc, s_emptyParams); | ||
| #if !PORTABLE | ||
| new PermissionSet(Security.Permissions.PermissionState.Unrestricted).Assert(); | ||
| #endif | ||
| return (Exception)s_prepForRemoting.Invoke(exc, s_emptyParams); | ||
| } | ||
| catch | ||
| { | ||
| } | ||
| #if !PORTABLE | ||
| finally | ||
| { | ||
| CodeAccessPermission.RevertAssert(); | ||
| } | ||
| #endif | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have a test that fails when the Assert() is removed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the return, the PrepareRemoting method returns the exception. It just happens to return 'this' so ignoring it doesn't cause a problem. Personal preference to follow the obvious desire of the original code and use the return value. The tests don't fail, but in partial trust they will lose their full stack trace. I wasn't sure how to write a reliable test for that without some crazy string checking that would also make the test localisation specific.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, sounds good. The stack trace is a good example of the type of thing I'd like to put under test. How crazy, exactly? What if we check for the presence of a stack frame with a certain method name which we set up to get lost in one scenario and not the other?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That could work! I'll give it a shot. |
||
| } | ||
| return exc; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,273 @@ | ||
| using System; | ||
| using System.Reflection; | ||
| using System.Runtime.Serialization; | ||
| using System.Security; | ||
| using System.Security.Permissions; | ||
| using System.Security.Policy; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
| #if NET45 | ||
| using TaskEx = System.Threading.Tasks.Task; | ||
| #endif | ||
|
|
||
| #if NET45 | ||
| namespace ReferenceAsync.Tests | ||
| #elif NET35 | ||
| namespace AsyncBridge.Net35.Tests | ||
| #elif ATP | ||
| namespace AsyncTargetingPack.Tests | ||
| #else | ||
| namespace AsyncBridge.Tests | ||
| #endif | ||
| { | ||
| #if !ATP | ||
| public sealed class Sandbox : IDisposable | ||
| { | ||
| private readonly AppDomain _Sandbox; | ||
|
|
||
| public Sandbox() | ||
| { | ||
| var CurrentSetup = AppDomain.CurrentDomain.SetupInformation; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding #19 asap. Sorry one more time! 😟 |
||
| var MySetup = new AppDomainSetup() | ||
| { | ||
| ApplicationBase = CurrentSetup.ApplicationBase | ||
| }; | ||
|
|
||
| var MyPermissions = new PermissionSet(PermissionState.None); | ||
| MyPermissions.AddPermission(new SecurityPermission(SecurityPermissionFlag.Execution)); | ||
| // Ensure we can read our own assemblies and files | ||
| MyPermissions.AddPermission(new FileIOPermission(FileIOPermissionAccess.Read, CurrentSetup.ApplicationBase)); | ||
|
|
||
| _Sandbox = AppDomain.CreateDomain( | ||
| "Partial Trust Tests", | ||
| AppDomain.CurrentDomain.Evidence, | ||
| MySetup, | ||
| MyPermissions, | ||
| #if NET45 | ||
| new StrongName[0] | ||
| #else | ||
| // AsyncBridge and AsyncTargetingPack need full trust to work. The test classes remain untrusted | ||
| new StrongName[] { CreateStrongName(typeof(TaskEx).Assembly.GetName()) } | ||
| #endif | ||
| ); | ||
|
|
||
| _Sandbox.UnhandledException += OnUnhandledException; | ||
| } | ||
|
|
||
| public TRemote Create<TRemote>() where TRemote : MarshalByRefObject | ||
| { | ||
| // This does not demand ReflectionPermission, so we don't need to give the test assembly full trust | ||
| return (TRemote)Activator.CreateInstanceFrom(_Sandbox, typeof(TRemote).Assembly.ManifestModule.FullyQualifiedName, typeof(TRemote).FullName).Unwrap(); | ||
| } | ||
|
|
||
| public void Dispose() | ||
| { | ||
| AppDomain.Unload(_Sandbox); | ||
| } | ||
|
|
||
| public static void OnUnhandledException(object sender, UnhandledExceptionEventArgs e) | ||
| { | ||
| Write.Line(string.Format("Unhandled Remote Exception {0}", e.ExceptionObject.ToString())); | ||
| throw new ApplicationException("Unhandled Remote Exception", (Exception)e.ExceptionObject); | ||
| } | ||
|
|
||
| private static StrongName CreateStrongName(AssemblyName assemblyName) | ||
| { //**************************************** | ||
| var MyPublicKey = assemblyName.GetPublicKey(); | ||
| //**************************************** | ||
|
|
||
| if (MyPublicKey == null || MyPublicKey.Length == 0) | ||
| throw new InvalidOperationException(string.Format("Assembly Name for {0} must specify the full Public Key", assemblyName.Name)); | ||
|
|
||
| return new StrongName(new StrongNamePublicKeyBlob(MyPublicKey), assemblyName.Name, assemblyName.Version); | ||
| } | ||
| } | ||
|
|
||
| [TestClass] | ||
| public class PartialTrustTests | ||
| { | ||
| public PartialTrustTests() | ||
| { | ||
| // Forces .Net to load the culture-specific assembly for exception messages BEFORE one gets raised from partial trust. | ||
| // If we don't do this, it will try to do so while in a partially trusted context, | ||
| // and get into an chain of failed AssemblyResolve calls, ending in a failed Assert for "mscorlib recursive resource lookup bug". | ||
| new ArgumentException(); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void CreateInSandbox() | ||
| { | ||
| using (var Sandbox = new Sandbox()) | ||
| { | ||
| var RemoteObject = Sandbox.Create<CreateInSandboxClass>(); | ||
| Assert.AreEqual("Hello", RemoteObject.SayHello()); | ||
| } | ||
| } | ||
|
|
||
| public sealed class CreateInSandboxClass : MarshalByRefObject | ||
| { | ||
| public string SayHello() | ||
| { | ||
| return "Hello"; | ||
| } | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void ExecuteTaskInSandbox() | ||
| { | ||
| using (var Sandbox = new Sandbox()) | ||
| { | ||
| var RemoteObject = Sandbox.Create<ExecuteTaskInSandboxClass>(); | ||
|
|
||
| RemoteObject.Execute(); | ||
| } | ||
| } | ||
|
|
||
| public sealed class ExecuteTaskInSandboxClass : MarshalByRefObject | ||
| { | ||
| public void Execute() | ||
| { | ||
| TestUtils.RunAsync(async () => | ||
| { | ||
| await TaskEx.Yield(); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void ThrowTaskInSandbox() | ||
| { | ||
| using (var Sandbox = new Sandbox()) | ||
| { | ||
| var RemoteObject = Sandbox.Create<ThrowTaskClass>(); | ||
|
|
||
| try | ||
| { | ||
| RemoteObject.Execute(); | ||
|
|
||
| Assert.Fail(); | ||
| } | ||
| catch (InvalidOperationException e) | ||
| { | ||
| Assert.AreEqual("Exception from Task", e.Message); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public sealed class ThrowTaskClass : MarshalByRefObject | ||
| { | ||
| public void Execute() | ||
| { | ||
| TestUtils.RunAsync(async () => | ||
| { | ||
| await TaskEx.Yield(); | ||
|
|
||
| throw new InvalidOperationException("Exception from Task"); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void ThrowNestedTask() | ||
| { | ||
| var RemoteObject = new ThrowNestedTaskClass(); | ||
|
|
||
| try | ||
| { | ||
| RemoteObject.Execute(); | ||
|
|
||
| Assert.Fail(); | ||
| } | ||
| catch (InvalidOperationException e) | ||
| { | ||
| Assert.AreEqual("Exception from Task", e.Message); | ||
| } | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void ThrowNestedTaskInSandbox() | ||
| { | ||
| using (var Sandbox = new Sandbox()) | ||
| { | ||
| var RemoteObject = Sandbox.Create<ThrowNestedTaskClass>(); | ||
|
|
||
| try | ||
| { | ||
| RemoteObject.Execute(); | ||
|
|
||
| Assert.Fail(); | ||
| } | ||
| catch (InvalidOperationException e) | ||
| { | ||
| Assert.AreEqual("Exception from Task", e.Message); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public sealed class ThrowNestedTaskClass : MarshalByRefObject | ||
| { | ||
| public void Execute() | ||
| { | ||
| TestUtils.RunAsync(async () => | ||
| { | ||
| await TaskEx.Run(async () => | ||
| { | ||
| await TaskEx.Yield(); | ||
|
|
||
| throw new InvalidOperationException("Exception from Task"); | ||
| }); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| [TestMethod] | ||
| public void ThrowCustomExceptionInSandbox() | ||
| { | ||
| using (var Sandbox = new Sandbox()) | ||
| { | ||
| var RemoteObject = Sandbox.Create<ThrowCustomExceptionInSandboxClass>(); | ||
|
|
||
| try | ||
| { | ||
| RemoteObject.Execute(); | ||
|
|
||
| Assert.Fail(); | ||
| } | ||
| catch (CustomException e) | ||
| { | ||
| Assert.AreEqual("Exception from Task", e.Message); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public sealed class ThrowCustomExceptionInSandboxClass : MarshalByRefObject | ||
| { | ||
| public void Execute() | ||
| { | ||
| TestUtils.RunAsync(async () => | ||
| { | ||
| await TaskEx.Run(async () => | ||
| { | ||
| await TaskEx.Yield(); | ||
|
|
||
| throw new CustomException("Exception from Task"); | ||
| }); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| [Serializable] | ||
| public sealed class CustomException : Exception | ||
| { | ||
| public CustomException(string message) : base(message) | ||
| { | ||
| } | ||
|
|
||
| public CustomException(SerializationInfo info, StreamingContext context) : base(info, context) | ||
| { | ||
| } | ||
| } | ||
| } | ||
| #endif | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this line is removed, no tests fail. Is it possible to test this in some way? If not, can you add a comment above it explaining why it is needed?
Also, no biggie but would you mind renaming the file to AssemblyInfo.cs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SecurityRules is recommended to be added. On my phone so I can't reference the exact reason. Will check later.
Will rename. That's what the file was called in the older project before the cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.microsoft.com/en-us/dotnet/framework/misc/security-transparent-code-level-2#usage-examples-and-behaviors
"If you do not annotate an assembly, the .NET Framework 4 rules are used by default. However, the recommended best practice is to use the SecurityRulesAttribute attribute instead of depending on the default."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it's locking it rather than following the CLR default. That's fine so long as we can be sure that this doesn't cause any issues on CLR v2, which we don't run tests against. How would we verify that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the project even build against a vanilla 2.0? The attribute only exists in 4.0+, so maybe we just need to change the #if to specifically check for 4.0+
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I missed that this is still nested in
#if !NET35! Might read more clearly, but it's up to you.