System.IO: Make text and sequence streams non-seekable - #132023
Conversation
Clarify that read-only and writable memory streams expose the existing contents of supplied memory, including guidance for rented or reused buffers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Prevent backward positioning from repeatedly replaying segmented sequences or encoded text. Keep Length, Position, and Seek consistent with the standard non-seekable Stream contract, and update conformance coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR aligns several Stream implementations with an intentionally non-seekable contract (notably ReadOnlySequenceStream, and reinforcing StringStream expectations) and updates docs/tests accordingly, including clarifying that the memory-backed streams expose existing buffer contents immediately.
Changes:
- Make
ReadOnlySequenceStreamintentionally non-seekable by removing seek state/traversal and havingLength/Position/SeekthrowNotSupportedExceptionconsistently. - Clarify
ReadOnlyMemoryStream/WritableMemoryStreamdocs that backing memory contents are immediately readable. - Update conformance and focused unit tests to reflect the non-seekable behavior and exception expectations.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime/tests/System.IO.Tests/StringStream/StringStreamTests_String.cs | Extends capability tests to assert unseekable members throw NotSupportedException. |
| src/libraries/System.Private.CoreLib/src/System/IO/WritableMemoryStream.cs | Doc updates clarifying immediate exposure of existing buffer contents. |
| src/libraries/System.Private.CoreLib/src/System/IO/StringStream.cs | Adds rationale comment for keeping the stream non-seekable. |
| src/libraries/System.Private.CoreLib/src/System/IO/ReadOnlyMemoryStream.cs | Doc updates clarifying immediate exposure (and minor wording tweak needed). |
| src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceStream.ConformanceTests.cs | Updates conformance configuration to treat the stream as non-seekable; removes seek-specific override. |
| src/libraries/System.Memory/src/System/Buffers/ReadOnlySequenceStream.cs | Implements non-seekable contract; removes seek/position bookkeeping and logic. |
| src/libraries/System.Memory/src/Resources/Strings.resx | Adds NotSupported_UnseekableStream resource; removes no-longer-needed seek-related resource strings. |
Reuse the sliced sequence in CopyToAsync and clarify stream wrapper documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/libraries/System.Memory/src/System/Buffers/ReadOnlySequenceStream.cs:173
CopyToAsyncCoreonly assigns_position = _sequence.Endafter the full loop completes. IfWriteAsyncthrows/cancels after some segments were written, the stream’s_positionstays at the original value and subsequent reads can replay already-transferred bytes.
Advance _position after each successfully completed WriteAsync call.
{
await destination.WriteAsync(segment, cancellationToken).ConfigureAwait(false);
}
_position = _sequence.End;
src/libraries/System.Memory/src/System/Buffers/ReadOnlySequenceStream.cs:44
- This change makes a previously-seekable public
Stream(ReadOnlySequenceStreamis inSystem.Memoryref) reportCanSeek == falseand throwNotSupportedExceptionfromLength/Position/Seek. That’s a behavioral breaking change for existing consumers.
The PR description doesn’t reference a breaking-change tracking issue. Per docs/project/breaking-change-process.md, please link/create an issue marked breaking-change (with before/after behavior and mitigations) and reference it from the PR so reviewers can evaluate compatibility impact.
// segment boundaries may be indirectly controlled by an untrusted network client through
// packet framing, so even correct stitching logic can produce adversarial fragmentation.
// Consumers must remain resilient against the worst technically compliant implementation
// rather than assuming ASP.NET-like segmentation.
public override bool CanSeek => false;
src/libraries/System.Memory/src/System/Buffers/ReadOnlySequenceStream.cs:143
CopyTowrites multiple segments but only updates_positionafter the loop completes. Ifdestination.Write(...)throws after some segments were successfully written, the stream will keep its old_positioneven though bytes were already consumed, so subsequent reads / retries can duplicate data.
Update _position after each successfully-written segment so the stream’s internal read position always reflects the bytes already transferred.
This issue also appears on line 169 of the same file.
{
destination.Write(segment.Span);
}
_position = _sequence.End;
Summary
ReadOnlyMemoryStreamandWritableMemoryStreamimmediately expose their backing memory contentsReadOnlySequenceStreamintentionally non-seekable and remove its seek state and traversal logicStringStreamintentionally non-seekable and document why backward positioning would require rerunning the encoderLength,Position, andSeekconsistently throw the standard unseekable-streamNotSupportedExceptionRationale
Backward positioning requires replaying work from the beginning. For
ReadOnlySequenceStream, that means traversing segments whose boundaries may be indirectly controlled by an untrusted network client through packet framing. Even correct stitching can therefore produce adversarial fragmentation, and consumers must tolerate the worst technically compliant segmentation rather than assuming ASP.NET-like behavior. ForStringStream, backward positioning requires rerunning the encoder. Repeated seeks can turn both cases into worst-case O(N) work.Validation
ReadOnlySequenceStreamconformance testsStringStreamconformance/unit testsNote
This pull request description was generated with GitHub Copilot.