Skip to content
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

System.Runtime.CompilerServices.AsynUnsafeAttribute - .NET7 Security #72711

Closed
c-ohle opened this issue Jul 23, 2022 · 10 comments
Closed

System.Runtime.CompilerServices.AsynUnsafeAttribute - .NET7 Security #72711

c-ohle opened this issue Jul 23, 2022 · 10 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.CompilerServices

Comments

@c-ohle
Copy link

c-ohle commented Jul 23, 2022

Background and motivation

For security reasons and to avoid lots of code for security checks, it would be helpful to have an Attribute like [System.Runtime.CompilerServices.AsyncUnsafeAttribute].

This attribute should be applicable to class and value types or specific functions and properties.
The compiler should then simply forbid using the functions in asynchronous code.

Many classes and functions use ThreadStatic content and using it in an asynchronous context makes no sense anyway.
Doesn't make any sense like for example calling Windows.Forms functions directly, knowing that it leads to an Exception.

For .NET7 developments, in discussion with other dotnet-devs, we have to make core solutions absolutely end user save.
In our case, for example, this requires millions of relatively slow AsyncLocal<T> requests at runtime - just to catch a possible programming error - just to raise a runtime Exception.
Not only does it slow the computations down, it also requires a lot of extra code and additional static root objects.

How it could look like:

[System.Runtime.CompilerServices.AsyncUnsafeAttribute]
static class AClass 
{
  public static void AMethod();
}

async void AAsynFunc()
{
  AClass.AMethod(); // <- Error: CSXXXX "Not allowed in an asynchronous context."
  await SomeThing();
 
}

This could help make C# more secure and faster.

API Proposal

[System.Runtime.CompilerServices.AsyncUnsafeAttribute]
static class AClass 
{
  public static void AMethod();
}

async void AAsynFunc()
{
  AClass.AMethod(); // <- Error: CSXXXX "Not allowed in an asynchronous context."
  await SomeThing();
 
}

API Usage

A .NET 6 / .NET 7 project where it would really helpful:
https://github.com/c-ohle/RationalNumerics

Alternative Designs

No response

Risks

No response

@c-ohle c-ohle added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 23, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 23, 2022
@ghost
Copy link

ghost commented Jul 23, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

For security reasons and to avoid lots of code for security checks, it would be helpful to have an Attribute like [System.Runtime.CompilerServices.AsynUnsafeAttribute].

This attribute should be applicable to class and value types or specific functions and properties.
The compiler should then simply forbid using the functions in asynchronous code.

Many classes and functions use ThreadStatic content and using it in an asynchronous context makes no sense anyway.
Doesn't make any sense like for example calling Windows.Forms functions directly, knowing that it leads to an Exception.

For .NET7 developments, in discussion with other dotnet-devs, we have to make core solutions absolutely end user save.
In our case, for example, this requires millions of relatively slow AsyncLocal<T> requests at runtime - just to catch a possible programming error - just to raise a runtime Exception.
Not only does it slow the computations down, it also requires a lot of extra code and additional static root objects.

How it could look like:

[System.Runtime.CompilerServices.AsynUnsafeAttribute]
static class AClass 
{
  public static void AMethod();
}

async void AAsynFunc()
{
  AClass.AMethod(); // <- Error: CSXXXX "Not allowed in an asynchronous context."
  await SomeThing();
 
}

This could help make C# more secure and faster.

API Proposal

[System.Runtime.CompilerServices.AsynUnsafeAttribute]
static class AClass 
{
  public static void AMethod();
}

async void AAsynFunc()
{
  AClass.AMethod(); // <- Error: CSXXXX "Not allowed in an asynchronous context."
  await SomeThing();
 
}

API Usage

A .NET 6 / .NET 7 project where it would really helpful:
https://github.com/c-ohle/RationalNumerics

Alternative Designs

No response

Risks

No response

Author: c-ohle
Assignees: -
Labels:

api-suggestion, area-System.Runtime.CompilerServices

Milestone: -

@teo-tsirpanis
Copy link
Contributor

Can't it be implemented with an analyzer?

@c-ohle
Copy link
Author

c-ohle commented Jul 23, 2022

@teo-tsirpanis
Interesting question. Do you have experience? Is it possible to bind a class to such an existing or custom analyzer?
This could also help in other situations. I need it to protect a VM for arbitrary arithmetic for illegal cross-thread calls.

@teo-tsirpanis
Copy link
Contributor

I haven't written an analyzer but I think it's possible to do what you want to do.

Another mocu more approachable idea is to make this type a ref struct. They are forced to reside on the stack and so are thread-safe, in the sense that only one thread has access to them.

@c-ohle
Copy link
Author

c-ohle commented Jul 23, 2022

@teo-tsirpanis
good idea, I'm sure that would work for other cases but the VM to protect is class.
I was just reading about Custom Analyzer, seems like a lot of effort and more to check syntax and get coding hints.

@teo-tsirpanis
Copy link
Contributor

Your way to go then is to make the type a ref struct. The restrictions on its usage are enforced by both the language compilers and the VM.

@c-ohle
Copy link
Author

c-ohle commented Jul 23, 2022

@teo-tsirpanis I'll try, interesting to see what happens.
Actually, it wouldn't be a problem to reference the VM in such a read-only ref struct.
But an AsyncUnsafe Attribute or something like this should be a general better solution. Also for so many other cases.

@stephentoub
Copy link
Member

It seems like the core concern is a type that's using or relying on thread-related state across method invocations. If that's the issue, this is in no way limited to async: any code that, e.g., stored the instance into a static to be accessed by another thread, or passed the instance to a method like ThreadPool.QueurUserWorkItem (either explicitly in a boxed state object or captured implicitly in a lambda), etc. You used Windows Forms as an example; the constraint that a control only be used from the thread with which it's associated long predates async/await.

I see this issue stems from a BigRational proposal. I'm skeptical we'd ship a BigRational implementation that had these kinds of constraints. But if we did, it'd be done as @teo-tsirpanis says by making it a ref struct, which has all of the constraints that limit it to the stack.

@c-ohle
Copy link
Author

c-ohle commented Jul 23, 2022

@stephentoub
Thanks for pointing that out, the more I think about it - it really wouldn't be a problem and I could keep the ref struct in pointer size. Not only because of that - I'm involved in a discussion to realize a builder solution for BigInteger where same problems occur. Perhaps you are the "brain" behind BigInteger since you fixed the bugs? Someone to discuss?

@stephentoub
Copy link
Member

stephentoub commented Jul 23, 2022

@tannergooding is the right person to discuss it with, and I see they're already involved in that other issue. I'll go ahead and close this one. Thanks.

@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 23, 2022
@teo-tsirpanis teo-tsirpanis closed this as not planned Won't fix, can't repro, duplicate, stale Jul 25, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.CompilerServices
Projects
None yet
Development

No branches or pull requests

3 participants