docs: clarify String.GetPinnableReference() returns a read-only interior pointer#130237
docs: clarify String.GetPinnableReference() returns a read-only interior pointer#130237AaronRobinsonMSFT with Copilot wants to merge 2 commits into
Conversation
…aint Closes #130225 The XML documentation on `String.GetPinnableReference()` has been updated to explicitly state that the returned `ref readonly char` interior pointer is read-only. Writing to the referenced memory (for example, by casting away `const` in C++/CLI when using `PtrToStringChars`) is not permitted and will silently corrupt the CLR heap. Co-authored-by: AaronRobinsonMSFT <30635565+AaronRobinsonMSFT@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-runtime |
| /// <para> | ||
| /// When using this method (or the equivalent C++/CLI <c>PtrToStringChars</c> function) to | ||
| /// obtain a pinned interior pointer to the string's character data, the resulting pointer | ||
| /// must be treated as read-only. In C++/CLI, <c>PtrToStringChars</c> exposes the pointer |
There was a problem hiding this comment.
I do not think we want to be talking about C++/CLI specific methods in the documentation for System.String.
The discussion of C++/CLI PtrToStringChars should be in C++/CLI documentation.
| /// <remarks> | ||
| /// <para> | ||
| /// The <see cref="string"/> type is immutable: the value of a <see cref="string"/> object | ||
| /// cannot be changed after the object is created. The reference returned by this method is | ||
| /// <c>ref readonly</c> and must only be used for reading characters; writing to | ||
| /// the referenced memory is not permitted and results in undefined behavior, including | ||
| /// potential corruption of the managed heap. | ||
| /// </para> | ||
| /// <para> | ||
| /// When using this method (or the equivalent C++/CLI <c>PtrToStringChars</c> function) to | ||
| /// obtain a pinned interior pointer to the string's character data, the resulting pointer | ||
| /// must be treated as read-only. In C++/CLI, <c>PtrToStringChars</c> exposes the pointer | ||
| /// as <c>const wchar_t*</c> to reflect this constraint. Casting away <c>const</c> | ||
| /// and writing through the pointer will silently corrupt the CLR heap and may cause | ||
| /// crashes or incorrect behavior at an unrelated location later in program execution. | ||
| /// </para> | ||
| /// </remarks> |
There was a problem hiding this comment.
The method already returns a ref readonly char; why do we need to get into this much detail that we should not mutate the returned memory?
There was a problem hiding this comment.
I agree - this method alone is the least of the problem since it returns ref readonly.
The general issue is covered in https://learn.microsoft.com/en-us/dotnet/standard/unsafe-code/best-practices#17-string-mutations . There are many ways to get the pointer to string characters using unsafe code, it applies to all of them.
| /// Returns a read-only reference to the first element of the String. If the string is null, an access will throw a NullReferenceException. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> |
There was a problem hiding this comment.
The source of truth for official docs in dotnet-api-docs repo has number of other remarks: https://learn.microsoft.com/en-us/dotnet/api/system.string.getpinnablereference?view=net-10.0
It may be better to do the changes there.
|
This isn't where I was expecting copilot to do the work. There is already a doc page for this and that should be updated, nothing in the runtime. |
String.GetPinnableReference()(and the equivalent C++/CLIPtrToStringChars) returns an interior pointer into an immutable managed string. The existing doc comment didn't communicate this constraint, leaving the door open for dangerous patterns like casting awayconstin C++/CLI and writing through the pointer — which silently corrupts the CLR heap.Change: Expanded the XML doc on
String.GetPinnableReference()to explicitly state:ref readonly— reads only; writes are undefined behaviorPtrToStringCharssurfaces this asconst wchar_t*for exactly this reasonconstand writing through the pointer will silently corrupt the managed heap, with crashes manifesting at unrelated call sites