Proposal for non-cooperative abortion of code execution

[AntonLapounov]

There is a number of execution environments for .NET code that may benefit from having an option to perform non-cooperative abortion of user code, e.g., C# and F# interactive compilers (csi and fsi) or .NET Interactive Notebooks (see #41291). While the legacy Framework provided the Thread.Abort method, it has never been supported by .NET Core, because aborting code execution at a random point is inherently unsafe and may leave the process in an inconsistent or a corrupted state. Thus, the only available option to interrupt stuck code execution is to terminate the process and lose its state, which is undesirable for interactive scenarios.

Below is the proposed System.Runtime.CompilerServices.ControlledExecution API to allow controlled execution of given code. The Run method starts execution of the delegate and TryAbort method may be used to attempt aborting its execution by throwing an asynchronous exception on that thread. Aborting may be successful only when the code is executing managed code, e.g., running a CPU-intensive loop, on the original thread. This API would not help cases where the thread is stuck in unmanaged code or waiting for some other thread to finish its work.

public class ControlledExecution
{
    public ControlledExecution(Action action);

    // Runs the specified action.
    // Throws an InvalidOperationException if the action is already running.
    // Throws a ThreadAbortException if execution was aborted.
    public void Run();

    // Attempts to abort the execution of the action by injecting an asynchronous exception.
    // Returns true on success or false otherwise.
    // Throws an InvalidOperationException if the action is not running.
    public bool TryAbort(TimeSpan timeout);
}

The implementation will generally use the same mechanism as the one utilized by the debugger to abort threads doing function evaluation (Thread::UserAbort). That includes marking the thread for abortion, suspending it, checking whether it is running managed code and safe to redirect, redirecting the thread to the ThrowControlForThread runtime helper, walking the stack to verify the tread is in a safe spot, and raising the exception for thread abort.

A possible option to make the implementation simpler is to rely only on GC polls / polls for thread abort instead of redirecting the thread on Windows, similarly to what is done on Unixes.

[stephentoub]

We've deleted a whole bunch of code in .NET Core that was there to help try to improve reliability in the face of thread aborts, e.g. we might now have:

_spinLock.Enter();
_i++;
_j++;
_spinLock.Exit();

instead of:

bool entered = false;
try
{
    _spinLock.Enter(ref entered);
    _i++;
    _j++;
}
finally
{
    if (entered)
    {
        _spinLock.Exit();
    }
}

New code has also been written assuming no thread aborts. Is the proposal that all such code needs to come back? Or is the idea that if a ThreadAbortException is thrown out of Run, the caller should promptly tear down the process?

[AntonLapounov]

This class will not provide any consistency guarantees and it should not be used in product environments. It may still be useful in development scenarios like interactive evaluation.

[Clockwork-Muse]

... if it's in development scenarios, what's wrong with manually killing the process (which would also take care of unmanaged code)?

[AntonLapounov]

Killing the process and losing its state is too harsh price for making a typo that led to an infinite loop in interactive evaluation (as an example). This API is not for general use and that should be clearly documented.

[alexrp]

Relevant context: #41291

[mangod9]

walking the stack to verify the tread is in a safe spot, and raising the exception for thread abort.

What constitutes a "safe" spot? Also would it raise Thread abort or another specific exception for this case?

[AntonLapounov]

We have the Thread::ReadyForAsyncException function that checks whether a thread is ready for an asynchronous exception. We prohibit even rude thread aborts in some x64 epilogs.

[AntonLapounov]

Also would it raise Thread abort or another specific exception for this case?

Good question. We may consider introducing a new exception class, e.g., ControlledExecutionException, instead of reusing ThreadAbortException.

[KevinRansom]

The proposal looks good. Although I was wondering why there is Abort and TryAbort, especially since abort is not guaranteed.

I like the idea of a ControlledExecutionException, obviously ResetAbort should not work for the new exception. But it should be automatically rethrown if caught and handled.

[jkotas]

But it should be automatically rethrown if caught and handled.

I do not think we should do automatic rethrow if the exception is handled. That was one of the features of thread abort that was very difficult to get right (consider situations like a try/catch block nested inside finally block). I would leave it up to the hosting code to decide whether it wants to inject a new abort exception.

[AntonLapounov]

If we allow the exception to be swallowed, TryAbort returning true would not necessarily mean a successful abort, which might be considered misleading. Something like TryInjectException might be a better naming then. (And if the exception to inject is passed as an argument, we would not need to introduce a special exception type.)

That was one of the features of thread abort that was very difficult to get right (consider situations like a try/catch block nested inside finally block).

Oh, I have just tried this nested scenario and I think I found a bug.

[jkotas]

Something like TryInjectException might be a better naming then.

Sounds good to me.

[AntonLapounov]

Although I was wondering why there is Abort and TryAbort, especially since abort is not guaranteed.

The underlying Thread::UserAbort function has two arguments: a timeout and a flag indicating whether to do a safe or a rude thread abort. A safe thread abort may happen only at a "safe" spot: when the thread is not holding locks and not inside catch/finally regions. For this reason a safe thread abort is not guaranteed and may timeout. On the other hand, a rude thread abort ignores those restrictions and it is almost guaranteed to succeed (it is still prohibited to happen in some x64 epilogs).

Do we want to support both types of aborts? For instance, the debugger first tries to perform a safe thread abort with a half second timeout before initiating a rude thread abort with another half second timeout.

[jkotas]

I think we can have enum that allows the caller to suppress (best effort) the exception injection in certain situations, like when the thread is executing inside catch or finally blocks on stack or when leaf method that the thread is executing is in the corelib (again, best effort - it can be imprecise due to inlining).

The app code can then decide in which order and for how long to try the different abort strategies. This is policy heavy piece that we should avoid baking into the runtime.

[AntonLapounov]

Thank you for valuable insights. Since we can no longer guarantee safety for the reasons you described, I am not sure introducing a enum for a "slightly safer" strategy is justified. We might start with providing a rude thread abort only and reconsider if there is good evidence that a better strategy would be helpful.

[jkotas]

Is the rude thread abort going to run the exception handling (finally and catch blocks)? (I think it should - just trying to clarify the details.)

[AntonLapounov]

If we allow running catch (Exception) { } blocks in the code executed by ControlledExecution.Run(), we cannot guarantee any progress for the abort since you suggested to not do automatic rethrow. Can we skip processing EH until we unwind to the ControlledExecution.Run() frame? Or maybe ControlledExecution.Run() should use a dedicated thread to run the code and skip EH. That may still cover the scenario we want to support.

[jkotas]

Yes, this again a case of a policy choice between higher risk of corrupted process vs. higher risk of not making progress.

A related situation is what happens when the thread has interleaving of managed -> unmanaged -> managed on stack? How are we going to unwind that?

[jkotas]

void Abort();
bool TryAbort(int millisecondsTimeout);

I assume that we would allow Timeout.Infinite for the timeout and so Abort would be implemented as TryAbort(Timeout.Infinite). I think we can have TryAbort only.

[AntonLapounov]

If we provide a rude thread abort only, I guess we could keep just Abort, because a rude thread abort is quite unlikely to not succeed. Otherwise, I agree we may have TryAbort only.

[jkotas]

Rude thread abort can take infinite amount of time in pretty normal situations when the thread is stuck in unmanaged code. I think having the timeout on it is a good idea.

[AntonLapounov]

Ah, good point. I'll leave just TryAbort then.

[TonyValenti]

I would really like to see a core framework / library for handling out of process function invocation.

I have a (rather specific) library I've built that I use to do the following:

1. I pass it a type and a method with parameters.
2. It creates a brand new process that instantiates the object and hosts it via JsonRpc.
3. I connect to the "remote" object and invoke it.
4. If I need to, I can simply terminate the entire hosting process.

I have found this really helpful when invoking wonky COM objects that might hang. I also like it because it keeps unsanitary threads/instances out of my core process and contains them in a disposable process.

[jkotas]

There is no reason for this to be in a core framework. It can be standalone nuget package just fine.

[AntonLapounov]

It seems we have somewhat conflicting design requirements here:

• We do not want to rethrow the injected exception automatically if it was handled. (As that would be error-prone.)
• That means we may not be able to abort, for example, a long-running loop containing try/catch all.
• That suggests we have to provide a rude thread abort as an option.
• Internally, a rude thread abort destroys the thread; it cannot be handled by managed code.
• That suggests we have to run the specified delegate on a separate thread.

Does that make sense? If yes, we may start with a rude thread abort on a separate thread and see how that goes. @KevinRansom

[KevinRansom]

@AntonLapounov my only concern is that because the abort path is an execution it is conceivable that the script author can include code that catches and swallows the exception thus defeating the whole abort thingy. The desktop runtime used to have unhandlable execeptions, threadabort was one of them. I expect that ControlledExecutionException is similar, in that we would implement the handler, and user scripts would not be able to handle the exception.

[jkotas]

I suppose forcing the script execution on to a separate thread, forces the developer to manage IO such that output from the script and the UI is coordinated

Yes, forcing execution on a separate thread (that the API creates for you) makes this API bakes policy into the API. If somebody wants to run the execution, they should be responsible for creating the thread.

I think it is very likely that you won't be able to use the API if it forces execution on a separate thread. If you believe that it should not be a problem, could you please make a change in F# interactive that creates a separate thread for each execution and see how many people get broken by it?

[AntonLapounov]

One interesting difference between C# and F# Interactive is preserving the ExecutionContext. C# Interactive uses async calls to run submitted snippets and any changes to ExecutionContext, e.g., setting the thread's CurrentCulture, are reverted between submissions:

> Thread.CurrentThread.CurrentCulture = new CultureInfo("de-DE");
> Thread.CurrentThread.CurrentCulture.Name
"en-US"

while F# Interactive preserves them:

> Thread.CurrentThread.CurrentCulture <- CultureInfo.GetCultureInfo("de-DE");;
val it: unit = ()

> Thread.CurrentThread.CurrentCulture.Name;;
val it: string = "de-DE"

I agree that running on a separate thread may break some scenarios. The question here is how much code changes and testing is required to allow rude thread aborts (if we support them) without destroying the thread.

[N-Olbert]

Does providing the possibility of a rude thread-abort only offers any significant benefits compared to what is already possible using PInvoke (pthread_create/pthread_cancel on *NIX, and CreateThread/TerminateThread on Windows) ?

If not why not providing a separate package with the corresponding functionality instead of expanding the framework?

Im leery in general whether it's "good" to (re-)introduce such an API, especially since its intended use ('development scenarios') is quite narrow and - in my opinion not a primary thing which has to be offered by a general runtime like .NET (it could be part of the SDK though). If this is going to be introduced into the runtime you can be absolutely sure - no matter how clear you advise against its use in the docs - that this API will eventually being abused as a replacement for 'Thread.Abort' even in production scenarios. Since the .NET runtime seems to be a lot less stable against aborts nowadays, I expect application quality to degrade. In .NET Framework the Framework itself was (more or less) “abort-safe”, and the majority of abort issues were caused by application code, now – even if the application code is indeed “abort-safe”- the framework is not unless the code changes mentioned by @stephentoub are going to be reintroduced.

I also doubt that thread-abortion is the correct level of control to stop the execution of arbitrary user code, since only processes can be aborted (killed) for sure. F. e. a (malicious) script could simply be

new Thread(() =>
                {
                    Thread.CurrentThread.IsBackground = false;
                    while (true) { }
                }).Start();

For this script thread-abortion / the new API does not provide any benefits – since the script created its own thread, the abortion of the main scripting thread has no effect at all.

[AntonLapounov]

@N-Olbert TerminateThread may easily corrupt the native runtime to the point where further process execution is no longer possible (e.g., terminating a thread that is in the middle of a garbage collection). Injecting a managed exception is a much safer mechanism. Please see #41291 for use cases that may benefit from having this functionality. I agree on your other points that this API is not bulletproof and may be potentially abused.

[teo-tsirpanis]

Who marked the discussion as api-ready-for-review? They have to make an issue; discussions with this label will not show up in https://apireview.net/.

[AntonLapounov]

Yes, I have already realized that. I will create an issue. Thanks!

[rickbrew]

I have an amendment I'd like to propose: Can there be a way to disable this? Possibly with a DOTNET_DisableControlledExecutionAsyncAbort=1; some kind of process-wide policy, I mean.

My reason for proposing/requesting this is that I do not want plugins in my app to be able to use something this dangerous, as it could have cascading effects on the stability of the app, ultimately resulting in data loss for the user. In my case, the app is Paint.NET. There are many plugins (some users have 1000+ installed!).

The experience level of plugin developers is wide-ranging. Some are expert software engineers, some are students, while many are just hackers having fun with a copy/paste/modify/run/"neat!" loop. Many of the latter do not know C#, or barely know it, and just push things around it until they get their desired visual result (for an effect plugin).

I absolutely do NOT trust plugin developers to write "good" code. Not just fast code. I mean code that doesn't do bad things, intentionally or not. I say this because I've seen a lot of plugins do very seriously weird or even downright evil stuff -- using reflection to grab things inside of API objects or to modify the Paint.NET UI, hooking major event handlers (AppDomain stuff) to achieve local results but with negative global consequences, etc. While porting Paint.NET to .NET 5 last year I ran into a whole lot of this stuff and had to engineer a lot of creative ways to shim the plugins to workaround and avoid some of the worst behaviors (I go as far as literally rewriting the plugin's IL to detour them around bad methods).

I'm seriously worried that some plugin will use this, and then other plugins will copy-by-example. Or it will get inside of some library / nuget package that becomes popular, with the same result. I want to be able to just block any of that silliness from ever entering my process.

I do believe I'd have a workaround if this does not make it passed API review: I have a highly customized system for loading plugins, using AssemblyLoadContexts, and I can just block DLLs with known names and/or versions from loading. (I currently use this to prevent a plugin from using already-loaded DLLs from nuget packages that the app uses, in order to avoid version hell -- a plugin must bundle its own copy of, e.g. Newtonsoft.Json.dll or TerraFX.Interop.Windows.dll). However, I don't know if other apps with plugin ecosystems will necessarily have things set up to easily accomplish that.

[rickbrew]

@rickbrew This will be implemented in System.Private.CoreLib.dll and the native runtime.

I missed this comment before my reply after it.

If that's the case then a Disable() method is perfectly alright for me.

[rickbrew]

(As long as there's no Enable() method of course :) )

[stephentoub]

We have number of features that are dangerous or otherwise problematic under a configuration switches.

Can you share examples of APIs considered dangerous that throw unless a switch is set?

If we want to ship an analyer that warns by default if you use anything we consider super dangerous, fine, we've done that with ModuleInitializer (it could look for known pinvokes, too) . But I don't see how this particular API is so much worse than so many other things you can easily do just by copying/pasting answers from stack overflow.

At the end of the day, I don't care super strongly about it, though.

[jkotas]

Number of the switches for security and trimmability make APIs throw. For example, EnableUnsafeUTF7Encoding, BuiltInComInteropSupport, EnableUnsafeBinaryFormatterSerialization.

I understand that these switches are not exactly same as what we are talking about here. These switches have often different defaults based on other build properties, and they were often used to fix historical mistakes. However, if you look at it without taking historic context into account, there is very little difference.

[stephentoub]

Thanks.

However, if you look at it without taking historic context into account, there is very little difference.

FWIW, I disagree. These are about, as you say, fixing mistakes, and sunsetting security risks we actively want to prohibit devs from using, with the eventual goal of removing the functionality entirely. Here we're talking about adding such a switch at the exact same time we're introducing the brand-new functionality.

[rickbrew]

Also, the environment variable is just an idea. Neither that nor the DLL blocking in the ALC are perfect solutions, I can already think of a few ways for a plugin to get around either.

If it's possible for this setting to be in .runtimeconfig.json, that might be the strongest blocking mechanism.

[AntonLapounov]

After working on a prototype and test cases, I started leaning towards the original proposal with TryAbort and an automatically rethrown exception. I think that would lead to smaller and safer code churn on the runtime side. The existing abort machinery in the runtime automatically rethrows the exception until the TS_AbortRequested flag on the thread is cleared. While it is possible to introduce a new abort mode, besides TA_Safe and TA_Rude, that will clear the flag before the first throw (I think Thread::HandleThreadAbort is the right place for that), @KevinRansom's team will have to reimplement rethrowing in their code generator to undo our suppression. On the other hand, if we simply reuse the existing TA_Safe abort mode and rethrow the exception until we unwind to the corresponding ControlledExecution.Run frame, where we will clear the TS_AbortRequested flag, that seems exactly what was requested with no code generator changes required. (We will need to properly handle nested ControlledExecution.Run calls though.) And for a general scenario, where catch blocks are not under the host's control, injecting an easily catchable exception seems too weak API. While the host may attempt to re-inject the exception in a loop, there is no forward progress guarantee. That makes me think that rethrowing the exception may be a better solution for the general case as well, even if there are some corner cases where rethrowing may not work as expected. Please let me know if I missed anything. @jkotas

[jkotas]

We will need to properly handle nested ControlledExecution.Run calls though

I do not think we need to. We can say that nested ControlledExecution is not supported.

[jkotas]

if we simply reuse the existing TA_Safe abort mode and rethrow the exception

Is the TA_Safe abort mode used by the debugger funceval today?

I see your point that inventing a new scheme is complicated.

I think it is reasonable to say that this behaves exactly same as funceval abort under debugger. In other words, this is just exposing what's available as debugger API via a managed API. Do you think we can do that?

[mangod9]

@janvorli here since some of this TA logic might change as part of CET work, but doesnt look like it will affect this feature.

[AntonLapounov]

Is the TA_Safe abort mode used by the debugger funceval today?

Yes. For some reason GitHub's search does not find that, maybe because the file is too long:

runtime/src/coreclr/debug/ee/debugger.cpp

Line 15157 in 77a8d3c

hr = pDE->m_thread->UserAbort(EEPolicy::TA_Safe, (DWORD)FUNC_EVAL_DEFAULT_TIMEOUT_VALUE);

Using the same call to abort ControlledExecution.Run seems to work and requires minimal changes on the native side. (Introducing a new thread abort mode is also possible if there is a compelling reason.) As Manish suggested, we may put the [RequiresPreviewFeatures] attribute on the ControlledExecution class and get feedback on this API before finalizing it.

[AntonLapounov]

I have shared my prototype with @KevinRansom the last week. If anyone is interested to try or review, please take a look: AntonLapounov@f6025e9. As I mentioned earlier, reusing existing UserAbort semantics as much as possible allowed to minimize code churn in the native runtime.

I am wondering if we should consider making public the switch to disable finally cloning in the JIT compiler. Right now, execution of finally statements during TryAbort depends on whether it is a debug or release build, the length of the finally statement, etc. Disabling the switch would at least give some predictability in that respect (even though we have promised no guarantees).

[jkotas]

I would stick to promising no guarantees.