Repository navigation
Clarify properties of memory safety in .NET programs #375
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
3a8a39c
Clarify properties of memory safety in .NET programs
agocke 00faad4
Update caller-unsafe.md
agocke 776fad6
Define requirements for managed references in unsafe code
agocke 9270c8d
Update managed references requirements in caller-unsafe.md
agocke 0a9bd6c
Update managed references requirements for safety
agocke 75d4d80
Clarify managed references requirements in documentation
agocke 3eadb92
Apply suggestion from @jkotas
agocke File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This statement may be interpreted as including "must be non-null", but that's not the case. Null managed references are fine. Do we need to clarify that?
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.
Imo it's unclear beyond just that. Maybe something like:
This makes the alignment requirement explicit, along with the type-compatibility requirement too (although, it could be worth having a section that actually defines what type-compatible actually is also, as it's not particularly simple - e.g.,
ref readonly objectpointing tostringstorage is fine, butref objectpointing tostringstorage is not;ref bytepointing tointstorage is fine, butref intpointing tobyte x4storage is not correctly aligned in general, so you'd somehow have to guarantee being appropriately aligned for the whole duration on top of that, unlike in the other direction (e.g., by fixed, or by being in a type with 4B alignment & starting at 4B alignment, etc.); etc.), along with explicitly allowingnull.It also removes this part:
which reads to me as that I have to be able to do
var someValue = *(T*)Unsafe.AsPointer(ref myRefT);, which is certainly not always valid even for valid byrefs.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.
My understanding is that this is true for every non null byref. Why wouldn't this be true?
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.
It's not, because the GC could move it before you actually dereference, which results in a GC hole. This is especially true if you do a bunch of stuff in between the conversion & dereference (but also true for the code sample I provided, just you are unlikely to see it in practice with that particular sample). It's only valid if it's otherwise pinned & otherwise kept alive for the whole duration that the pointer can be dereferenced for (if either of these fail, then you run into issues, as the pointer will neither pin it nor track a relocation, nor keep it alive, if a GC occurs at just the wrong time - whereas having a byref keeps it tracked & using it keeps it alive).
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 wonder whether the cases this is intended to cover would be better explained in the clarifying examples toward the end of the document. All the cases I can think of already follow from requiring safe code to maintain the two core properties. This seems redundant as a core property.
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.
Maybe we need a section like "this is valid" and another section like "for publicly exposed things, it's unsafe if these properties are not met"? E.g., a ref to end of array is valid & one existing doesn't break the safety invariant on its own, but it's unsafe to publicly expose it.
In particular, to clarify, both aspects are part of the safety model - the first aspect discusses what has to be met to not break relevant runtime invariants, and the second discusses additional requirements that have to be met if you publicly expose it for callers to not break those same runtime invariants without
unsafe.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 don't think it's actually illegal to have such references, only to dereference them with improper alignment/managed type.
MemoryMarshal.GetArrayDataReference(Array)is a proof of that.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.
You are correct, but if you publicly expose it that would be unsafe. This lines up well with the idea I just posted before you commented :)
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've tried to rephrase this by adding a new requirements section. I think rules about managed references are reasonable to split out since they're not about any particular API, they're global rules separate from any API.
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.
One thing I want to help people with: a lot of unsafe code is about users being aware of the requirements that APIs are placing on them. But the system also places requirements on people who write unsafe code. And violating those rules can destabilize the system, regardless of what the caller does. It's good to call these things out because these are basically "the rules for writing valid unsafe code" and they're separate from the rules about using unsafe code.