Skip to content

Commit

Permalink
Proposal for fixing unsafe
Browse files Browse the repository at this point in the history
  • Loading branch information
jaredpar committed Oct 26, 2021
1 parent 2802e29 commit 1d6cf99
Showing 1 changed file with 101 additions and 0 deletions.
101 changes: 101 additions & 0 deletions proposals/unsafe.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
Completing our unsafe enforcement
===

## Summary
This proposal expands the enforcement of `unsafe` such that diagnostics are issued for APIs that are marked as `unsafe`, not just ones that contain pointers.

## Motivation
The design of `unsafe` meant that C# only enforces caller be in an `unsafe` context when they are calling an API that contains a pointer type. Specifically that means types like `int*`, `void*`, etc ...

Using pointer types as the mechanism of enforcement was always a bit flawed. For example the following APIs are equally dangerous, but only one is flagged as requiring `unsafe` to call:

```c#
unsafe int* Alloc(int size);
unsafe IntPtr Alloc2(int size);

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

Would it make sense to require unsafe for IntPtr by default? We have nint now that is a better fit for non-pointer uses.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 26, 2021

To me, IntPtr is similar to Int32. The only reason its not an exact alias is due to back-compat since .NET Framework 4.0 introduced custom operators.

With generic math, IntPtr itself is getting all the same support and while we could make it "explicitly implemented" (and therefore not visible), we have an opportunity here to just fix these and make them "true aliases" (particularly if we get user-defined checked operators as proposed).

It's on me to start the talks here with Chuck/other relevant people from LDM and with API review to see if this is a break we are willing to take.

This comment has been minimized.

Copy link
@jaredpar

jaredpar Oct 28, 2021

Author Owner

From a purist angle I think trying IntPtr like a real pointer type is the right approach. It seems challenging from a compat perspective. It would mean that this support is essentially tied to developers refactoring large amounts of IntPtr to nint.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 28, 2021

IntPtr is typically used as unmanaged pointer or equivalent that should be in unsafe context anyway.

It is very hard to use IntPtr as an integer and so it is rare to see it used that way - that's why we have introduced nint.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 28, 2021

This is also only a C# limitation and it can (and IMO should) go away.

F# has had native integer since v1 and it doesn't currently emit the attributes for nint compatibility.

The only reason IntPtr didn't support operators (and therefore have nint being an alias in the same way as int vs Int32) was because of some APIs the framework exposed in .NET Framework v4.0

We can fix this with generic math, particularly if we get user-defined checked operators and so we can stop treating them differently (avoiding numerous other issues and edge cases that currently exist and cause problems, including the problems around generic type parameters and usage in generic math).

```

Even with this limitation it was still easy for C# developers to spot unsafe APIs. The vast majority either used real pointer types or existed on the `Marshal` type (which is generally understood to be an unsafe type). Overall though developers could simply look at the types involved and know that if a real pointer, `IntPtr` or `UIntPtr` was used then the operation was unsafe.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

Also, DllImport has always been a common source of hidden unsafe code. Should we make it explicitly unsafe? It is probably not worth it. It would be a whole new level of disruptive. It would be nice to mention it.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

the Marshal type (which is generally understood to be an unsafe type)

The whole System.Runtime.InteropServices namespace is generally understood to be unsafe.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 26, 2021

DllImport has always been a common source of hidden unsafe code. Should we make it explicitly unsafe

My vote is yes. extern methods in general should be considered unsafe by default, if for no other reason than they explicitly leave normal managed control.

The impacted users are most likely the ones who are actively moving to each new .NET version anyways and so this may be a good venue to push them towards automated bindings (either via DllImportGenerator or some other tooling like CsWin32, ClangSharp, etc).

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

FWIW, unsafe in Rust treats extern methods as unsafe, so that may be +1 for C# to do the same.

https://doc.rust-lang.org/book/ch19-01-unsafe-rust.html

This comment has been minimized.

Copy link
@GrabYourPitchforks

GrabYourPitchforks Oct 27, 2021

Rust is an interesting example because they make very heavy use of runtime invariants, so they also use unsafe to denote mechanisms which might violate those invariants. For example, mechanisms like private reflection and free multithreading would fall under Rust's "unsafe" umbrella. I don't know how far we'd want to go with that ourselves, but it could give a glimpse into how far-reaching these changes might be in .NET if we wanted to follow their lead.

This comment has been minimized.

Copy link
@stephentoub

stephentoub Oct 27, 2021

There's a really slippery slope here. Midori, for example, considered mutable statics to be unsafe, which can't be on the table here. I think the scope of this for .NET should effectively be anything that would let you directly read/write to arbitrary memory / enable violating the runtime's (not just arbitrary types in corelib) ability to function correctly, which would mean anything with pointers, most surface area on {Memory}Marshal (even if it's using IntPtr), extern/DllImports, etc. We should not consider ArrayPool, GC.Allocate{Uninitialized}Array, etc. to be unsafe.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 27, 2021

Incorrect use of ArrayPool can lead to violating runtime. It is likely we have places in dotnet/runtime libraries where unexpected buffer modifications (that would be side-effect of double-free) lead to buffer overruns and similar bad crashes.

This comment has been minimized.

Copy link
@stephentoub

stephentoub Oct 27, 2021

By "runtime" here I meant GC, JIT, etc., and explicitly not C# code using ArrayPool that would result in data read from an array being something other than was expected.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 27, 2021

If you end up with buffer overrun due to unexpected buffer modification, it can easily crash the GC. For example, consider code like this:

https://github.com/dotnet/runtime/blob/b726f7b1c8e759bef03e44c370399a5f5cd44b12/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs#L220

and what is going to happen if somebody does var b = ArrayPool<GCHandle>.Shared.Rent(...); ArrayPool<GCHandle>.Shared.Return(b); ArrayPool<GCHandle>.Shared.Return(b);. It is very likely that it is going to be fatal crash in GC or unmanaged runtime.

I understand that GCHandle is unsafe type, so this is not a perfect example. For the sake of argument, assume that this code is changed to use ArrayPool<IntPtr> in .NET 7 to improve ArrayPool utilization.

This comment has been minimized.

Copy link
@stephentoub

stephentoub Oct 27, 2021

For the sake of argument, assume that this code is changed to use ArrayPool in .NET 7 to improve ArrayPool utilization.

In which case it's the IntPtr usage that's unsafe, not the ArrayPool usage.

If we go down the route of saying it's ArrayPool that's the unsafe thing here, then so too are many mutable statics.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 27, 2021

I think there is probably a balance between "unsafe when called in a particular way" and "unsafe because it directly does unsafe things" (memory or pointer management, leaves managed control, bypasses language safety rules or semantics, etc).

ArrayPool really seems to be closer to the former and I'd expect most of the discussions for this general topic are around the latter.

I think (this isn't a strong feeling) ArrayPool is likely the type of thing where we need a different API, feature, or analyzer to help cover the relevant usage semantics.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 27, 2021

I like to think about this in terms of global vs. local corrupted state.

Mutable statics can only cause a local corruption. If you have a bug around safe mutable statics, it will cause a non-fatal exception somewhere near. It is fine with me for the mutable statics to be consider safe.

ArrayPool<IntPtr> example leads to a global corruption. The bug is in one place and the fatal crash happens in a completely unrelated place. I think that the principle should be that safe user code, no matter how broken it is, should not lead to a global corruption.

This comment has been minimized.

Copy link
@stephentoub

stephentoub Oct 27, 2021

Mutable statics can only cause a local corruption.

I don't understand how:

public static readonly byte[] Array = new byte[100];

is any different from ArrayPool in this regard. Multiple threads can end up using this incorrectly and still end up corrupting each other's view of the world. Such a field would have rules around how to use it correctly, just as ArrayPool does. And we wouldn't / shouldn't require that any such field be marked unsafe.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 27, 2021

-- Slightly meta point, why don't we use a LockedHashSet (rather than a LockedStack) for returning arrays to avoid the double return issue?

There is still the issue that you return to two distinct array pools; but I don't think that's really fixable.

This comment has been minimized.

Copy link
@stephentoub

stephentoub Oct 27, 2021

why don't we use a LockedHashSet (rather than a LockedStack) for returning arrays to avoid the double return issue?

That would make usage slower and would only address a small subset of the misuse. I can still return an array then keep using it while someone else is. I can also return an array, someone else takes it, and then I return it again.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 27, 2021

Multiple threads can end up using this incorrectly and still end up corrupting each other's view of the world

Yes, that's correct. Bug like that will only cause local corruption that is likely to lead to non-fatal exception in the same .cs file or somewhere near. It won't lead to fatal crash in unrelated place.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 27, 2021

Perhaps the global is not the best word to use to describe the distinction.


The introduction of `Span<T>` blurred the lines here. The vast majority of `Span<T>` uses are safe but there are several APIs that very unsafe like `MemoryMarshal.CreateSpan`. Developers cannot tell at a glance which of these are or are not safe. The libraries team does have this understanding but has no mechanism by which to educate developers.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

I agree that Span<T> blurred the lines, but it has been source of confusion even before, especially for people new to the platform.

We can link examples of issues demonstrating the confusion. Recent one: dotnet/runtime#60758 (Passing a randomly generated IntPtr into an API in System.Runtime.InteropServices namespace leads to a fatal crash. Surprise?)

This comment has been minimized.

Copy link
@stephentoub

stephentoub Oct 26, 2021

The vast majority of Span<T> uses are safe but there are several APIs that very unsafe like MemoryMarshal.CreateSpan.

I don't know this is really span-related. Pretty much anything with "Marshal" in the name is as unsafe as pointers. That's true for all of the marshaling surface area that existed long before span.

This comment has been minimized.

Copy link
@GrabYourPitchforks

GrabYourPitchforks Oct 27, 2021

I've tried to catalog some of the unsafe-equivalent APIs in dotnet/runtime#41418. That issue focuses primarily on APIs which perform pointer or reference manipulation, but there are some nuggets which wax poetic about other kinds of unsafedness, such as structs being backed with illegal bit patterns.

This proposal aims to give developers the tools to require their API be used in an `unsafe` context irrespective of the types used in the API itself.

## Detailed design
The compiler will enforce that consumption of APIs or types marked as `unsafe` occur only in an `unsafe` context.

This comment has been minimized.

Copy link
@stephentoub

stephentoub Oct 26, 2021

I'm not in love with this. Up until now, this:

unsafe void Foo()
{
    ...
}

and

void Foo()
{
    unsafe
    {
        ...
    }
}

were semantically identical, and lots of developers (us included) take advantage of that to simplify the code and remove unnecessary indentation and lines. This proposal would create a distinction between them. Maybe it's worthwhile, but it's going to result in a whole lot of busy work. It's one thing to force developers to mark consumption of unsafe things as unsafe... I'm ok with that work. But just because unsafe shows up in the API signature does not mean it's unsafe... it's frequently an implementation detail of the method.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

This shortcut is part of the confusion. I had to explain more than hundred of times that unsafe in method signature only affects method implementation even though it shows up in the API signature.

This comment has been minimized.

Copy link
@stephentoub

stephentoub Oct 26, 2021

Sure. Same with async. Would we change that, too, as part of this?

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 26, 2021

I don't like this due to the forced increase nesting. I'd be fine with it if we also get something like unsafe; (as listed below) to help avoid this.

Alternatively, we already have UnmanagedCallersOnly. We could add an UnsafeCallersOnly and have it be implicitly there in certain contexts (extern methods, methods that contain pointer parameters, etc). This also gives an avenue for users to explicitly annotate their methods without it being potentially confusing to the people who do use unsafe most frequently.

An analyzer for this bit in particular could flag the cases around unsafe in the declaration being confusing for those not "in the know". It could suggesting adding UnsafeCallersOnly explicitly as the fix. This would avoid changing existing semantics, could warn unfamiliar users by default, and is trivially picked up by or opted out-of by users who do regularly write unsafe code.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

Sure. Same with async. Would we change that, too, as part of this?

I think async is a different situation. If we had a chance to design the system from scratch, we would encode async in a special way in the method signature to make async calling convention special and nearly as efficient as regular calling convention. We have been talking about various async improvements. I think there is non-zero chance that we may end up encoding async in method signature once they start materializing.

This comment has been minimized.

Copy link
@stephentoub

stephentoub Oct 26, 2021

That may very well be true. But today, a developer writes "async" in the signature to signal to the compiler something about the implementation, and it does not actually end up in the signature in metadata, just as with "unsafe".

This comment has been minimized.

Copy link
@jaredpar

jaredpar Oct 28, 2021

Author Owner

Sure. Same with async. Would we change that, too, as part of this?

The async modifier impacts implementation and consumption.

M1(); // warning
M2(); // no warning

async Task M1() => throw null!;
Task M2() => throw null;

This comment has been minimized.

Copy link
@stephentoub

stephentoub Oct 28, 2021

The async modifier impacts implementation and consumption.

Implementation, sure, just like unsafe.

Consumption, only as warnings within the same compilation unit.


```c#
unsafe void M() {
...
}

M(); // Error: `M` can only be used from an unsafe context
unsafe {
M(); // Okay
}
```

This will cause issues for APIs that are presently using the `unsafe` modifier as a convenience mechansim for using `unsafe` inside the method body:

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

Typo: mechansim


```c#
unsafe void Process() {
const int size = 42;
Widget* p = stackalloc Widget[size];
Read(p);
for (int i = 0; i < size; i++) {
Use(p->size);
}
}
```

The `unsafe` modifier was not expressing any safety issue with invoking the API, instead it was just a convenience to use `unsafe` code inside the implementation. It's an implementation detail incorrectly being expressed as a method modifier. There is no reason for callers of `Process` to be in an `unsafe` context here. Once this change is in effect APIs like `Process` will need to move to the following form:

```c#
void Process() {
unsafe {
const int size = 42;
Widget* p = stackalloc Widget[size];
Read(p);
for (int i = 0; i < size; i++) {
Use(p->size);
}
}
}
```

The rule here is straight forward. If the desire is callers need to use `unsafe` to invoke the API, or differently stated it creates a potential safety issue in the caller, then use `unsafe` as a modifier. If `unsafe` is an implementation detail of the method then use an `unsafe` block.

This requires only a small change to C#. The enforcement of `unsafe` moves to APIs that are marked as `unsafe` not whether or not they contain pointer types. Given that all APIs with pointer types required `unsafe` this will not remove any existing diagnostics.

To maintain back compat this change will only take effect when using `/langversion:11` or higher. This will be a new warning diagnostic. The reason for a warning vs. an error is this is not deemed important enough to be a .NET 7 adoption blocker. Developers should be able to adopt .NET 7, and by extension C# 11, and silence this warning if it's too disruptive. They can then come back to this when it's convenient for them and fix it up.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

There are part of this:

  • Emitting new annotations
  • Generating diagnostic using new annotations

This paragraph talks about generating diagnostic using new annotations. I agree that it should be warning.

Do we also need a way to control whether the compiler should emit the new annotations? I am not sure whether people would be happy that the compiler starts emitting the new annotations after upgrading to C# 11, before they get a chance to cleanup their code.


## Drawbacks
This will cause a fair mount of churn in the dotnet/runtime code base as well as in our customers code. It wasn't uncommon for `unsafe` to be used as a convenience mechanism for using `unsafe` code inside a method body. Any customer that did this will need to move to using `unsafe` as an implementation detail to have the correct API shape.

This comment has been minimized.

Copy link
@stephentoub

stephentoub Oct 26, 2021

Yeah, this is going to affect a ton of code.

Did you consider an alternative where unsafe remains as it is but we add an [Unsafe] attribute that can be used to annotate additional things, ala dotnet/runtime#31354?

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 26, 2021

This is what I suggested. Have an UnsafeCallersOnly (to parity the existing UnmanagedCallersOnly).

We'd need some attribute that gets annotated on these methods in either case so C# can detect unsafeness in referenced assemblies.

This comment has been minimized.

Copy link
@jaredpar

jaredpar Oct 28, 2021

Author Owner

I considered a new annotation but working through the details it doesn't seem to provide advantages. I think in a lot of cases developers end up having to say unsafe twice: once for I want to use unsafe in the method and again for require caller unsafe.

[RequireUnsafeCaller]
unsafe void M() { 
  int* pointersAreAwesome = stackalloc int[42];
  ... 
}

I think it also creates a bit of confusion. It means that developers need to understand what implicitly does and does not require unsafe caller. That probably isn't as simple as it sounds especially if we move foward with the typedef proposal. At that point you can't rely on "look for a *" in the signature to know there is a real pointer or not. Eventually I think we'd settle on a rule like "always use [RequireUnsafeCaller]" vs. letting developers rely on their understanding of the existing rules. At that point it's saying unsafe twice again.

Yeah, this is going to affect a ton of code.

I do emphasize strongly with this. It's why I had not previously suggested this cause it seems a steep price to pay.

The other viable approach I think we could consider is having an attribute that replaces unsafe. Imagine for example you had [RequireCallerUnsafe] that provided two behaviors:

  1. It allows unsafe usage in the method body or signature
  2. It requires that the caller be in an unsafe context

That makes the feature truly opt in, we can use error diagnostics instead of warnings, etc ... It also works fine if we apply it to types.

At the same time it can also lead to a bit of churn. Guessing there would be a push to "do the new unsafe" once it was available. Possibly it's in the more acceptable realm of churn though.


## Alternatives

### Write an analyzer
This is a long standing issue in .NET but it's never risen to the point of justifying a language change. The renewed emphasis for the desire to have `unsafe` enforcement can be traced back to a handful of new types in .NET: `MemoryMarshal` and `Unsafe` are the most prominent. An alternative is to write an analyzer that raises a diagnostic if these types are used outside an `unsafe` context.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

We have issue opened on adding the analyzer that can be linked from here:

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


### Unsafe implementations
This change will require a lot of white space changes as a part of moving to `unsafe` blocks. That is annoying. We could explore other ways of marking method implementations as `unsafe` that avoid such a shift. As an example imagine if we allowed the first line of a method to be `unsafe;` which effectively marks the entire method body as having an `unsafe` context.

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

While this is annoying, I do not feel bad about it. My take has always been that we should make it possible to write the unsafe code, but it does not have to be pretty.

This comment has been minimized.

Copy link
@stephentoub

stephentoub Oct 26, 2021

I do not feel bad about it

I feel bad about it :)

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 26, 2021

Same. As a user who maintains a binding generator and tons of hand written interop code for a graphics library, this will be a huge PITA and not trivial to handle on my end (at least for the hand written code using the auto-generated bindings).

It's also going to trip me up for a long time that unsafe at the method or class level no longer does what its done for 20 years and now has an additional meaning.

I think an explicit attribute + on-by-default warning/analyzer pointing users to add the attribute is the right way to go here since it can be disabled by users who are aware of the differences and/or have quick-fixes available to easily fix things for them (the fix is that if a method has unsafe at the declaration level, offer to add UnsafeCallersOnly attribute).

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

FWIW, this proposal as written makes C# unsafe to behave almost exactly as unsafe in Rust, including the need for extra indentation of the unsafe scopes.

https://doc.rust-lang.org/book/ch19-01-unsafe-rust.html

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 26, 2021

I understand, and if we were doing this from day 1 I might agree.

However, given the existing 20 years of unsafe code and the bindings/libraries I maintain myself; I would very much be dreading this breaking change. I think that an alternative here, including new by default warning, would work just as well here and make it simpler for existing devs to transition over.

Particularly given that we are going to need some attribute in metadata anyways.

This comment has been minimized.

Copy link
@GrabYourPitchforks

GrabYourPitchforks Nov 30, 2021

See also related comment at dotnet/runtime#31354 (comment), which ponders an interaction between this proposal and the newly introduced Regex source generators.


```c#
void Process {
unsafe;

const int size = 42;
Widget* p = stackalloc Widget[size];
Read(p);
for (int i = 0; i < size; i++) {
Use(p->size);
}
}
```

That particular syntax may not be appealing but possible there is a better suggestion out there which is

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

Nit: unfinished sentence


## Unresolved questions

This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

How does this change what unsafe does at the type level? I do not think unsafe at type level can make all methods of the type unsafe.

Related: Should we also have unsafe annotations on fields for symmetry with methods? For example, unsafe IntPtr _ptr; - accessing this field is only allowed in unsafe context.

This comment has been minimized.

Copy link
@GrabYourPitchforks

GrabYourPitchforks Oct 27, 2021

Are there thoughts about APIs that may be partially unsafe rather than fully unsafe? For example, we currently have GC.AllocateArray and GC.AllocateUninitializedArray as separate APIs, but what if they were instead a single API GC.AllocateArray(..., bool skipZeroInit)? Would we want a level of granularity to be able to say "you're calling an API in an unsafe fashion," or for simplicity would we want to separate out fully-safe vs potentially-unsafe APIs into different methods?

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 27, 2021

I'd think that would need to be an analyzer. I don't think we'd want to build something that goes down to the parameter input level here.


This comment has been minimized.

Copy link
@jkotas

jkotas Oct 26, 2021

We will need guidance for what methods should be marked as unsafe. I think it should be only used for methods that can introduce global state corruption (ie you can introduce a fatal runtime crash by using the method incorrectly). It should not be used on methods that can only introduce local state corruption.

For example, CollectionsMarshal.AsSpan should not be marked as unsafe since incorrect use can only lead to local state corruption.


## Design meetings

0 comments on commit 1d6cf99

Please sign in to comment.