Skip to content

Update memory-safety with known issues #364

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.

Sign up for GitHub

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

Open
agocke wants to merge 2 commits into
base: main
Choose a base branch
from
+3 −1
Open

Update memory-safety with known issues #364

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c98515
Update memory-safety with known issues
agocke Mar 25, 2026
129b748
Update accepted/2025/memory-safety/caller-unsafe.md
agocke Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion accepted/2025/memory-safety/caller-unsafe.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,9 @@ According to the above definition, there are two valid constructors: the explici

These data races are considered fundamental to the .NET type system. The safe subset of C# does not protect against them.

- Resource lifetime. Some code patterns, like object pools, require manual lifetime management. When this management is done incorrectly bad behaviors can occur, including improper memory reuse. Notably, none of those behaviors include invalid memory access, although it can include symptoms that look like memory corruption. Because invalid memory access is not possible, this is considered safe.
- Resource lifetime. Some code patterns, like object pools, require manual lifetime management. When this management is done incorrectly bad behaviors can occur, including improper memory reuse. Notably, none of those behaviors include invalid memory access, although it can include symptoms that look like memory corruption. Because invalid memory access is not possible, this is considered safe. Any resource lifetime issues that can cause invalid memory access _are_ considered unsafe.
Copy link
Copy Markdown
Member

@jkotas jkotas Mar 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with this position.

If we are not going to address memory safety issues of the global ArrayPool in this project, we have failed.

Copy link
Copy Markdown
Member

@tannergooding tannergooding Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the point is that we want to address these types of ownership issues, but that its separate from the standard memory safety issues.

They will need a different language feature and set of attributes/annotations.

Copy link
Copy Markdown
Member

@jkotas jkotas Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is important that this is in scope for the overall unsafe evolution project. I agree that it is separate set of workitems.


Copy link
Copy Markdown
Member

@jkotas jkotas Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Mutable global state in .NET makes it possible to create APIs that are prone to produce symptoms identical to memory corruptions caused by lifetime management bugs in user code. `ArrayPool<byte>.Shared` is the prime example of such API. Even though API implementation may be 100% safe code, we will take a discretion to mark such APIs in core libraries as unsafe and propose safe alternatives to replace the most common usage patterns.

This is my counterproposal.

Copy link
Copy Markdown
Member Author

@agocke agocke Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not adjust our safety rules to include memory reuse from ArrayPool? That is, rather than create an exception for ArrayPool without putting it in the rules, I'd be OK just putting the ArrayPool approach in our rules directly.

Copy link
Copy Markdown
Member

@danmoseley danmoseley Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious what would be a safe alternative? Isn't the issue fundamental to array pooling, the caller may return and continue use?

Copy link
Copy Markdown
Member

@jkotas jkotas Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious what would be a safe alternative?

There are common patterns that we can introduce safe alternatives for. A local scratch buffer pattern is the prime example (dotnet/runtime#52065).

Copy link
Copy Markdown
Member

@jkotas jkotas Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not adjust our safety rules to include memory reuse from ArrayPool?

We have more global pool-like APIs. For example, System.Runtime.InteropServices.GCHandle. It happens to be implemented in C++, but it would be possible to re-implement it in safe C# and ensure that accessing the handle does not ever perform invalid memory access even when user code has use-after-free or double-free bugs. A safe implementation like that would not move the needle on GCHandle safety. It would be as unsafe as it is today.

Copy link
Copy Markdown
Member

@jkotas jkotas Mar 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So code only using GCHandle is fine because it is safe and only dangerous once you try to use something unsafe alongside it, like Unsafe.As.

It is impossible to write correct runtime code with these assumptions. We would have to split these global services into unsafe reliable ones, and "safe" ones where the calling code have to assume potential of arbitrary data corruptions. It does not make sense to me.

NativeMemory.Free, however, is potentially dangerous because while the behavior is well-defined for null and well-defined for a valid pointer returned by malloc

Let's say we would replace the standard (glibc) malloc/free implementation with an implementation that does not itself crash on double free or arbitrary pointers passed into it. It would be "safe" according to the current definition. Again, it would be impossible to write correct code when you have to assume arbitrary data corruptions.

Copy link
Copy Markdown
Member

@tannergooding tannergooding Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is impossible to write correct runtime code with these assumptions. We would have to split these global services into unsafe reliable ones, and "safe" ones where the calling code have to assume potential of arbitrary data corruptions. It does not make sense to me.

I don't see how, could you elaborate?

Assuming you aren't using an unsafe API like Unsafe.As, then you will either get a null Target, indicating that it was freed -or- you get a valid object back. That valid object can then be checked if it is the expected T, in which case it either is or isn't. In this latter scenario you have no way of knowing if the handle was freed and reallocated or if something simply called set

This is purely in the realm of logical bugs and not memory safety. It is somewhat memory adjacent in that it is potentially lifetime or ownership related, but it is very much not what we've defined "unsafe" to mean here and is fundamentally no different than someone defining a Dictionary<int, object> where previously used keys can be reused after a removal.

Let's say we would replace the standard (glibc) malloc/free implementation with an implementation that does not itself crash on double free or arbitrary pointers passed into it.

Well we can't due to the documented guarantee that this corresponds specifically to malloc/free and can be paired with APIs that document the same (it's the same reason we cannot change Marshal.AllocHGlobal on Windows).

But assuming we could, then NativeMemory.Free would be safe and documented as such. Indirecting the pointer is then still dangerous because we have no way of ensuring it is alive and therefore preventing or blocking NativeMemory.Free(ptr); ptr[0] = ...; We could build a higher level wrapper that does make this guarantee, but then it is providing the safety and stopping the propagation. That is after all what the GC itself is doing for reference types.

Copy link
Copy Markdown
Member

@jkotas jkotas Mar 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming you aren't using an unsafe API like Unsafe.As, then you will either get a null Target, indicating that it was freed -or- you get a valid object back

The problem is that GCHandles are used with Unsafe.As or equivalent a lot (in the unmanaged runtime code). It would be close to impossible to rewrite all that code to avoid doing that and be robust in the presence of arbitrary GC handle corruption.

assuming we could, then NativeMemory.Free would be safe and documented as such

My point is that this would be useless feature. It would not move the actual safety at all (quite the opposite actually).

I believe that our current definition of memory safety is leading us into building a useless feature, and I would like to see us to correct that.

Copy link
Copy Markdown
Member

@tannergooding tannergooding Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that GCHandles are used with Unsafe.As or equivalent a lot (in the unmanaged runtime code). It would be close to impossible to rewrite all that code to avoid doing that and be robust in the presence of arbitrary GC handle corruption.

I guess I still don't understand the point you're trying to make here. If we're using Unsafe.As then the code in question is going to be unsafe regardless and should likely at minimum have a Debug.Assert(handle.Target is T). Some code that isn't using Unsafe.As would be safe, but potentially slower.

However, even if we are checking the type and being memory safe, its still trivial to create logical bugs here. The same is true in any nearly any language, including Rust, even when are otherwise correctly tracking ownership, lifetimes, and avoiding memory unsafety; and so I don't think we will ever have any kind of feature that will meaningfully address that. We can only help reduce or mitigate certain kinds of bugs by highlighting what is trivially caught by static analysis.

My point is that this would be useless feature. It would not move the actual safety at all (quite the opposite actually).

I don't understand this either.

The point of the feature is to highlight memory safety issues. It is not to prevent all types of logic bugs. The feature, as being designed, is then doing what we set it out to do, which is to highlight where the memory unsafety is and allow its propagation up. We then have plans for potential additional features around ownership/lifetime that can help highlight other common issues, but which are not strictly memory safety issues.

We could start trying to encompass lifetime/ownership as part of this, but that then really gets into "all code can be broken down basically the same thing". What stops us from having to then flag all locks as unsafe, all multithreading as unsafe, and so on.

The feature ends up far less meaningful when we classify "everything" as unsafe and we were also trying to avoid a "feelings" based annotation strategy so we don't have issues of "this feels dangerous, lets mark it unsafe". It's why we all agreed that CollectionsMarshal wasn't unsafe because even though the span or byref could "disconnect" from the backing storage of the collection, it didn't provide memory unsafety, only potential logic bugs.

Copy link
Copy Markdown
Member

@jkotas jkotas Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I still don't understand the point you're trying to make here. If we're using Unsafe.As then the code in question is going to be unsafe regardless and should likely at minimum have a Debug.Assert(handle.Target is T).

The code in question assumes that the basic (safe or unsafe) services are correct. It is fairly common for unsafe code to depend on safe services behaving correctly; thus it is not acceptable for safe code to corrupt integrity of safe services that in turn breaks memory safety of the unsafe code that depends on the safe services.

- Reflection. Reflection is a known hole in the current unsafe model. Reflection can be used to call unsafe methods or access unsafe properties without the reflection code containing any unsafe blocks. A simple solution to close this hole would be to mark reflection invoke APIs as unsafe. We believe that doing so would be too difficult to adopt in user code. This may be addressed in a future proposal.

### Evolution

Expand Down
Loading