Skip to content

System.IO: Make text and sequence streams non-seekable - #132023

Open
jozkee wants to merge 3 commits into
dotnet:mainfrom
jozkee:agents/copy-artifacts-and-implement-seek-tests-0dd68e98
Open

System.IO: Make text and sequence streams non-seekable#132023
jozkee wants to merge 3 commits into
dotnet:mainfrom
jozkee:agents/copy-artifacts-and-implement-seek-tests-0dd68e98

Conversation

@jozkee

@jozkee jozkee commented Aug 7, 2026

Copy link
Copy Markdown
Member

Summary

  • clarify that ReadOnlyMemoryStream and WritableMemoryStream immediately expose their backing memory contents
  • make ReadOnlySequenceStream intentionally non-seekable and remove its seek state and traversal logic
  • keep StringStream intentionally non-seekable and document why backward positioning would require rerunning the encoder
  • make Length, Position, and Seek consistently throw the standard unseekable-stream NotSupportedException
  • update stream conformance and focused unit coverage

Rationale

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. For StringStream, backward positioning requires rerunning the encoder. Repeated seeks can turn both cases into worst-case O(N) work.

Validation

  • checked and Release CoreLib builds
  • System.Memory build
  • 405 focused ReadOnlySequenceStream conformance tests
  • 275 focused StringStream conformance/unit tests

Note

This pull request description was generated with GitHub Copilot.

jozkee and others added 2 commits August 7, 2026 12:28
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

Copy link
Copy Markdown
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ReadOnlySequenceStream intentionally non-seekable by removing seek state/traversal and having Length/Position/Seek throw NotSupportedException consistently.
  • Clarify ReadOnlyMemoryStream / WritableMemoryStream docs 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.

Comment thread src/libraries/System.Private.CoreLib/src/System/IO/ReadOnlyMemoryStream.cs Outdated
Reuse the sliced sequence in CopyToAsync and clarify stream wrapper documentation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 7, 2026 19:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • CopyToAsyncCore only assigns _position = _sequence.End after the full loop completes. If WriteAsync throws/cancels after some segments were written, the stream’s _position stays 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 (ReadOnlySequenceStream is in System.Memory ref) report CanSeek == false and throw NotSupportedException from Length/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

  • CopyTo writes multiple segments but only updates _position after the loop completes. If destination.Write(...) throws after some segments were successfully written, the stream will keep its old _position even 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;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants