-
Notifications
You must be signed in to change notification settings - Fork 167
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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. | ||||||
|
|
||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This is my counterproposal.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There are common patterns that we can introduce safe alternatives for. A local scratch buffer pattern is the prime example (dotnet/runtime#52065).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problematic API pattern is: Global, typically low-level, API with manual lifetime management, that many higher-level components depend on. |
||||||
| - 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 | ||||||
|
|
||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.