Conversation
Clarified the discussion on resource lifetime and reflection in the context of memory safety.
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| - 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. | ||
|
|
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Just curious what would be a safe alternative? Isn't the issue fundamental to array pooling, the caller may return and continue use?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| - 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. | ||
|
|
||
| - 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. This may be addressed a future proposal. |
There was a problem hiding this comment.
We should also address handles (OS handles in particular) since they are in similar category: They are not pure memory but produce same types of corruption. I assume that we are going to mark APIs that operate on raw (OS) handles without enforced lifetime management as unsafe. For example, public SafeFileHandle(IntPtr preexistingHandle, bool ownsHandle).
There was a problem hiding this comment.
Yeah, I expect that there's some potential for memory corruption in these cases, somewhere, so they would end up marked in the same way.
There was a problem hiding this comment.
The first order failure for OS handles lifetime issues is not a memory corruption. It is a syscall returning invalid handle error or unexpected data (e.g. you end up reading from a different file than the one you have opened originally). The invalid memory access can be a second order effect caused by the unexpected data.
It is very similar to the array pool. The first order failure for array pool lifetime issues is not a memory corruption. It is an array containing unexpected data. The invalid memory access can be a second order effect caused by the unexpected data.
There was a problem hiding this comment.
Related discussion about the recently approved ProcessStartInfo.InheritedHandle API: dotnet/runtime#126318 (comment) .
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
4 participants
Clarified the discussion on resource lifetime and reflection in the context of memory safety.