Strip corrupted state exceptions handling

[jkotas]

We should never handle corrupted state exceptions in .NET Core. Corrupted state exception should always fail fast.

Some of the code to handle corrupted state exceptions seems to be reachable after the NS2.0 work. We should make sure to not ship it.

[jkotas]

cc @gkhanna79

[gkhanna79]

@rahku PTAL at this issue at the earliest.

[rahku]

is the work to disable FEATURE_CORRUPTING_EXCEPTIONS for coreclr build?

[jkotas]

We should make sure that the "state corrupting exceptions" are not converted to managed exceptions at all. They should fail fast or remain unhandled.

Stripping the code for FEATURE_CORRUPTING_EXCEPTIONS would be nice too, but it is just a nice-to-have cleanup.

[rahku]

There are two instances in corelib where HandleProcessCorruptedStateExceptions attribute is used
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Threading/ExecutionContext.cs#L150
https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/Threading/ExecutionContext.cs#L201

I believe there might be other scenarios where people might use this attribute on desktop like catch exceptions coming from pinvoked native code and perform some logging before exiting. Reading from this article it does not seem like the attribute was added for compat reasons. Because for compat we added config switch legacyCorruptedState­­ExceptionsPolicy=true . So in my opinion we should keep the functionality of this attribute in .net core. Let me know your thoughts?

[jkotas]

There are two instances in corelib where HandleProcessCorruptedStateExceptions attribute is

It is left over from the hardening against corrupted state exceptions that was never done completely.

I still think we should fail fast whenever process corrupting exception reaches managed code, and not do any processing or handling of them in .NET Core. In particular, any processing of dirty AVs is security liability. It is best to fail fast for them right away.

[gkhanna79]

In particular, any processing of dirty AVs is security liability. It is best to fail fast for them right away.

@jkotas Are you suggesting FX usage of catching such exceptions should issue FailFast or are you suggesting the runtime should just failfast (like we do when there is an AV in the runtime)?

[jkotas]

I am suggesting that we should fail fast, just like when there is an AV in the runtime.

There is no usage of this in corefx. There are two places in CoreLib where this is used that are left over from incomplete hardening of the framework for the corrupting exceptions.

[gkhanna79]

Isn't that breaking with respect to the Desktop?

[jkotas]

Yes, it is difference from desktop.

Handling of corrupted state exceptions is set of low-level desktop features that tried to make it possible to write managed code for what would be best written in plain C. We have abandoned this direction and not included features from this set in .NET Core whenever we had opportunity to do so. Constrained Execution Regions (CERs) or stack overflow handling are other features in this set. None of them are in .NET Core, so we are differing in this area from desktop in general. Also, the different features in this set are tied together e.g. if you really want to write robust handler for corrupted exceptions, you would need CERs for that in desktop, so it does not make sense to pick and choose them in isolation.

[CharliePoole]

As you can see in nunit/dotnet-test-nunit#115, users tend to expect their test runners to be able to report such errors. 😄

In days of yore, NUnit even reported stack overflow exceptions. I recall having written code to display a stack trace at the point of failure, eliminating duplicates so it would be readable. I can't remember if we lost the ability to do that with .NET 2 or .NET 4, but users still complain about it from time to time. They expect the test runner to report that such and such a test caused the stack to overflow starting at a particular point in the code and then continue with the execution of other tests. So demanding. 😄 Yet, not an unreasonable thing to want if you're trying to debug a problem.

Of course, it's dangerous to do anything but cancel the app when state is corrupted. That's why, even on the desktop, we have not added anything to our app.config that would automatically resolve the issue. It's in our users' hands and IMO that's where it should stay. I hope you can manage to keep it there.

[jkotas]

@CharliePoole Thank you for the feedback.

I think it would be reasonable to keep the legacyCorruptedStateExceptionsPolicy config switch that disables the fail fast on corrupting exceptions and let them propagate as regular exceptions. Do you think it would be sufficient?

Or do you believe that the fine grained corrupted exception handling is really needed?

[CharliePoole]

I'd say the configuration setting is probably enough. The attribute requires the user to know which test is causing the problem and that can't be reported unless we catch the exception.

[gkhanna79]

legacyCorruptedStateExceptionsPolicy

Do you mean propagated as regular CSE, which is what happens in Desktop. Right @jkotas ?