Fixes for Partial Trust #14
Conversation
Added partially trusted caller support Added more SecuritySafeCritical attributes so an AsyncBridge-compiled program running with 4.5 installed will not fail. Fixed exception propagation in partial trust
There was a problem hiding this comment.
Thanks for taking the time to write these tests! This is pretty awesome stuff and it's temping to merge it right away!
If any change is important enough to add to the codebase, I'd like to have it protected by a test. This serves two critical purposes for us: documentation, and protection from future regressions.
| #if !NET35 | ||
| [assembly: AllowPartiallyTrustedCallers] | ||
| #if !PORTABLE | ||
| [assembly: SecurityRules(SecurityRuleSet.Level2)] |
There was a problem hiding this comment.
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.
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.
"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.
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.
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.
My bad, I missed that this is still nested in #if !NET35! Might read more clearly, but it's up to you.
| @@ -79,6 +79,7 @@ private AsyncVoidMethodBuilder(SynchronizationContext synchronizationContext) | |||
| /// <summary> | |||
| /// Registers with UnobservedTaskException to suppress exception crashing. | |||
| /// </summary> | |||
| [SecuritySafeCritical] | |||
There was a problem hiding this comment.
Can we have a test that fails when this line is removed?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's enlightening, thanks! I don't like the blind catch. Is there a way we could get rid of it on all platforms?
| { | ||
| CodeAccessPermission.RevertAssert(); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Can we have a test that fails when the Assert() is removed?
Also, I noticed that s_prepForRemoting.Invoke became prefixed with return (Exception). Is that fixing a separate bug? Can we have a test that fails without the return?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
That could work! I'll give it a shot.
|
|
||
| public Sandbox() | ||
| { | ||
| var CurrentSetup = AppDomain.CurrentDomain.SetupInformation; |
There was a problem hiding this comment.
Adding #19 asap. Sorry one more time! 😟
Can you use camel-case for both fields and locals, optionally with underscore for fields?
Added partially trusted caller support
Added more SecuritySafeCritical attributes so an AsyncBridge-compiled program running with 4.5 installed will not fail.
Fixed exception propagation in partial trust