Expose a concept of "unsafe callers only"

[tannergooding]

Unsafe Callers Only

Summary

C# should support the ability to annotate user-defined methods as unsafe such that they require callers to be in an unsafe { } context. This could be achieved by a new attribute [UnsafeCallersOnly] which preserves backwards compatibility or by taking a break to the language such that unsafe means unsafe.

Motivation

There is no way for a user to declare a type or method and indicate to the caller that the API is considered unsafe without it also directly exposing a pointer like member. This often leads to an assumption that APIs which don't require unsafe are somehow safe or at least "safer" than the pointer taking equivalents. Likewise there are many APIs which are unsafe which don't take pointers at all and there is currently no way to surface this information to the consumer.

Providing a mechanism through which this information can be surfaced will result in higher visibility in code review and better surfacing of important information to the consumer.

Design

The non-breaking design is that we introduce a new attribute [UnsafeCallersOnly] (whose name mimics the [UnmanagedCallersOnly] attribute used for unmanaged function pointers). When this attribute is encountered on a type or member, the language will require that the caller likewise be marked [UnsafeCallersOnly] or be calling the API from an unsafe { } context. If the caller is not in an unsafe { } context a diagnostic will be raised (it would be up to LDM to decide if error or warning was more appropriate).

An alternative design that was suggested was that unsafe should mean unsafe. That is, the unsafe modifier on a type or member would be equivalent to the proposed semantics of placing [UnsafeCallersOnly] on the same. This would be a breaking change in that existing code would now surface errors on recompile using a new language version (and presumably emit something similar to [UnsafeCallersOnly] at the IL level). However, it would match what many users I've talked to have indicated they thought unsafe at this level meant. unsafe { } within a member body would not convey the same, and so:

unsafe void N() { }     // only unsafe can call
void P() { unsafe { } } // anyone can call

The .NET Libraries would then be able to utilize this functionality on APIs such as System.Runtime.CompilerServices.Unsafe, System.Runtime.InteropServices.CollectionsMarshal, System.Runtime.InteropServices.MemoryMarshal, and System.Runtime.InteropServices.Marshal (subject to API review and approval).

Open Questions

Is taking a breaking change worthwhile or is it better to preserve backwards compatibility and use an attribute instead?

Should this be surfaced as a warning or an error?

Could this be achieved using an analyzer rather than language support?

What would be the behavior in the case of [UnsafeCallersOnly] being placed on a member that overrides a virtual?

[CyrusNajmabadi]

Could this be done as an analyzer instead? The case of having some attribute lead to errors/enforcement on other code seems like the poster child for analyzers.

[CyrusNajmabadi]

Possibly, but there have been many discussions on this including with @jaredpar and they've all been at the "it would be great to get proper language enforcement of this core concept" level.

I dislike the concept of something not being 'proper' if it's not at the lang level. To me, a lang level thing would be desirable if it's really just so painful as an analyzer that it's not feasible, or it really is using new syntax. For something which, up front, says it wants to be an attribute+rules, taht feels like something that should be an analyzer.

At the very least, it could start there pretty easily. And through the learnings from that, that could help inform a lang feature if it turned out the analyzer wasn't sufficient :)

[tannergooding]

Analyzers don't always provide the guarantees desired and that's been expressed many times in many forums. It's also been expressed that solving that is a very hard problem and its believe, by multiple LDM members, to be a potentially improbable problem fix due to constraints that exist in our ecosystem (such as many of the multi-pass build systems that exist where one or more phases must disable analyzers, including WPF/XAML).

As indicated, this issue was created based on discussions that have already happened (and which have continued to happen) over the past several releases. The breaking change that is unsafe means unsafe is one of the more prominent discussion points and was proposed by Jared. It is an avenue that would only be possible via the language and which, while breaking, would align with a public perception that already exists related to unsafe.

It's possible that we ultimately determine an analyzer is sufficient. It's even possible we determine that "do nothing, we're fine in today's world" is the right approach. But the starting point is a proposal covering the topic, it getting a champion, and being driven to conclusion.

[acaly]

But the starting point is a proposal covering the topic, it getting a champion, and being driven to conclusion.

Again (after the file private), time to consider clarifying the "Proposals that only make existing syntax optionally illegal will be rejected by the language design committee to prevent increased language complexity." in the README? :P

[tannergooding]

This is a proposal that has already had interest and discussion with LDM members.

[jaredpar]

Analyzers don't always provide the guarantees desired and that's been expressed many times in many forums.

And the compiler team has addressed these points many times and continue to believe that analyzers are an appropriate solution for these problems. Even the most stringent security teams at Microsoft use analyzers with the limitations these forums find as "blocking".

It's also important to remember that this is an existing problem for which no diagnostic is emitted. Yet .NET has been running just fine for 20 years now. That makes it very hard to argue that doing it in the compiler is a requirement, or even a significant data point to consider here.

such as many of the multi-pass build systems that exist where one or more phases must disable analyzers, including WPF/XAML

These are ecosystem bugs that come up from time to time and are always quickly addressed. Bugs in the ecosystem are not evidence for "this must be done in the compiler".

It's also been expressed that solving that is a very hard problem and its believe, by multiple LDM members, to be a potentially improbable problem fix due to constraints that exist in our ecosystem

May help to elaborate on this.

The breaking change that is unsafe means unsafe is one of the more prominent discussion points and was proposed by Jared

If we did the unsafe means unsafe approach then yes that would need to be done in the compiler. This has generally not been viewed favorably by the runtime and libraries team. The rough feedback is that it would be too disruptive to the ecosystem. Instead they'd prefer an attribute based approach. If we go the attribute based approach then it does become a valid question of "why isn't this an analyzer". The analysis here is very similar to banned API which is done as an analyzer so technically it is very doable.

But the starting point is a proposal covering the topic, it getting a champion, and being driven to conclusion.

I agree. But I don't think taking the conversation into rehashing the design of analyzers is appropriate. Many teams across at many companies use analyzers for important diagnostic work. Yes there are trade offs as compared to compiler diagnostics but every one of them, particularly when it comes to warnings, have an analogous compiler limitation. Bringing this up every time does not help the conversation here, it just derails it.

[tannergooding]

CC. @stephentoub, @jkotas, @jaredpar, @GrabYourPitchforks

This has been raised many times in the past and given that we are about to start considering .NET 8/C# 12 potential work items (and another discussion of this cropped up on Discord), I created a very basic rough draft of the topic and discussion points that have been raised.

Whether we land on language changes, an analyzer, or something else, this does keep getting brought up release after release so it might be good to land on some decision and drive it forward (even if that is a decision that we do nothing and why).

[EgorBo]

I'd want GetArrayDataReference to explicitly tell developer that it's unsafe so I support this feature

[teo-tsirpanis]

Should this be surfaced as a warning or an error?

Definitely a warning, the breaking potential of it is significant.

[hez2010]

I would prefer error, otherwise it will make the safe part of C# unsafe. It only introduces source breaking change, but not ABI breaking change. Developers can always use /p:LangVersion= to keep the old behavior for compatibility.

[theunrepentantgeek]

I'm not sure that's realistic - it would force developers into a corner where they couldn't upgrade language version and slowly adopt new features over time. They'd have to either do a massive migration or get stuck on an older version. Neither option is particularly palatable.

[stephentoub]

dotnet/runtime#31354
dotnet/roslyn-analyzers#972

[Joe4evr]

Also: dotnet/runtime#41418

[koszeggy]

There is no way for a user to declare a type or method and indicate to the caller that the API is considered unsafe without it also directly exposing a pointer like member.

Actually there is, [SecurityCritical] was used for a very similar purpose in .NET Framework. Using pointers (or some dangerous APIs such as serialization, etc.) required marking the method security critical. And the old analyzer enforced that all callers of a critical method must also be marked with [SecurityCritical], or [SecuritySafeCritical] where the latter could be used for 'safe entry points' to enter into a critical scope.

Actually I still use these attributes today because I compile my libraries even for .NET Framework; it's just they are completely ignored by the recent frameworks and analyzers. Instead of introducing new attributes for the same purpose wouldn't it be a good idea to make them meaningful again? So if you target also older platforms you don't need to unnecessarily duplicate the attributes.

Disclaimer: I know that using [SecurityCritical] and its friends were enforced at runtime on .NET Framework 4.x when Level2 security was enabled (and which actually didn't even work correctly) and this was gone by moving to .NET Core but I suggest to reintroduce these checks only at analyzer level.

[jkotas]

wouldn't it be a good idea to make them meaningful again?

The new meaning would be different. I do not think it would be wise to reuse the SecurityCritical attributes for this. For example, searching for SecurityCritical attribute returns many articles with guidance and answers on stackoverflow. None of it would apply to the new meaning and we would see a lot of confusion.

[tannergooding]

It would also risk unintentionally breaking APIs that already have and use this attribute for the original meaning.

This is along the same lines of why we likely wouldn't reuse the System.Diagnostics.Contracts.PureAttribute if .NET ever got an official (and enforced) view of "purity".

[danmoseley]

plus one might expect something called "security critical" to be applied in other places where security should receive special attention, not just places where memory access is potentially unsafe.

[GrabYourPitchforks]

Related: we've also started compiling a preliminary list of "unsafe" APIs over at dotnet/runtime#41418.

@EgorBo This also encompasses the GetArrayDataReference API you voiced concern about above.

[Xyncgas]

If I am using one of these functions in my function does the function that calls my function also has to be unsafe
: Do we pass the buck or the buck stops at the first function that calls the function with [UnsafeCallersOnly]

[Joe4evr]

That likely depends on what you know about your call-site. If you can determine as a private implementation detail that you calling these APIs over a single type/closed set of types are fine (won't lead to state corruption or anything), then only you have to wrap with unsafe { } as everything that calls it should be okay. If you can't, then it's also your responsibility to pass the buck to outside callers.

[Xyncgas]

Great

[MichalPetryka]

Personally I feel like changing the meaning of unsafe void N() { } would be too much of a breaking change since it'd affect pretty much every single project that uses unsafe. At the same the benefits it provides (being clearer to some users that don't use unsafe much) seem to not outweigh the issues it'd cause.

[Perksey]

As someone who has to constantly explain that for example Unsafe.SizeOf is just as unsafe as sizeof even if it doesn't require the keyword, I am definitely for this proposal. I agree that unsafe void X() should not itself carry any additional meaning or implications - technically every single function ever written can be thought of as unsafe as to get anything done we quite regularly leave the relative safety of managed land, but this is obviously in highly-scrutinised implementations in the BCL in the typical case where the necessary precautions have been taken for the user to just assume that it is safe and doesn't require extra care from a marshalling perspective. It should likely be its own attribute for those cases where that assumption is not safe to make for callers of the function.

An analyser could probably do it, but I'd worry about whether IDEs would get their wires crossed by spotting a superfluous unsafe that only an analyser requires. Moreover, this requires having the analyser whereas the function does not cease to have unsafe implications on the caller just because you Reference the dll directly or otherwise don't pull in the analyser.

This would definitely be helpful as someone that regularly gets community members asking for help on how to avoid unsafe code, when they're using Silk.NET - a direct 1:1 binding library with no additional validation even in the cases you can use refs intead of pointers for instance. I have been trying to manually instil these same concepts into my users' heads but I think the compiler would do a better job than I can.