Skip to content

Prevent separator buffer overruns in string.Join - #134529

Merged
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-separator-overflow-guard
Sep 23, 2026
Merged

tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-separator-overflow-guard

Conversation

@tannergooding

Copy link
Copy Markdown
Member

String.JoinCore already anticipates concurrent input mutation and retries with a defensive copy when the copies do not match the allocated length. However, it only checked capacity for values: a value could consume the remaining space, after which the unchecked separator write could overrun the result buffer.

Use a destination span bounded by the allocated string's length for both value and separator writes, advancing it after each successful copy. Preserve the single-character separator fast path and the existing defensive-copy retry, including when a copy fails after exhausting the destination.

Add an outer-loop concurrency theory covering separator lengths 1, 2, and 4000 with mutation at two input positions. The test triggered a heap-integrity assertion on the unmodified checked runtime. With the fix, all six cases pass; the full StringTests run had 2,195 passes and seven existing platform skips. After the final range-syntax edit, the checked CoreLib rebuild and all 73 Join tests passed.

Local Release BenchmarkDotNet comparisons showed unchanged allocations. Throughput measurements were noisy and inconclusive; this does not establish performance neutrality.

Resolves #134515

Note

This PR description was drafted by GitHub Copilot.

Use a bounded, advancing destination span for value and separator copies while preserving the defensive-copy retry for concurrent input mutation.

Co-authored-by: Copilot App <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.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

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.

Copilot review overview

🟢 Approval recommended

All reviewed changes are covered by passing regression tests with no unresolved blocking issues.

Review effort: Lite
Findings: None

What changed in this PR

Fixes a concurrent-mutation buffer overrun in String.Join by bounding separator and value writes.

Changes:

  • Uses bounded destination spans with safe copy retries.
  • Adds regression tests for varied separator lengths and mutation positions.
File Description
src/​libraries/​System.Private.CoreLib/​src/​System/​String.Manipulation.cs Bounds result-buffer writes and preserves defensive-copy retry behavior.
src/​libraries/​Common/​tests/​Tests/​System/​StringTests.cs Adds concurrent-mutation regression coverage.

@tannergooding
tannergooding enabled auto-merge (squash) September 23, 2026 19:30
@tannergooding

Copy link
Copy Markdown
Member Author

/ba-g unrelated failure

@tannergooding
tannergooding merged commit 4c25eb6 into dotnet:main Sep 23, 2026
149 of 152 checks passed
@tannergooding
tannergooding deleted the tannergooding-separator-overflow-guard branch September 23, 2026 23:47
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 12.0-preview1 milestone Sep 24, 2026
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.

Libs: (bug) string.Join assumes the input is immutable

4 participants