-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Fix PipeStream leak on Windows when pipe is disposed with a pending operation #115453
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
Conversation
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.
Pull Request Overview
This PR fixes a pipe leak on Windows by ensuring that the pipe state is set to closed before disposing shared resources and by improving the reuse logic for pending operations.
- In PipeStream.cs, the pipe’s state is set to closed earlier in the disposal process to avoid races.
- In PipeStream.Windows.cs, the reusability logic now calls source.Dispose() instead of _preallocatedOverlapped.Dispose() to better handle concurrent operations.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.cs | Updates the disposal order by marking the pipe as closed before further disposal. |
src/libraries/System.IO/Pipes/src/System/IO/Pipes/PipeStream.Windows.cs | Adjusts the reuse logic to dispose of the whole source instance when a concurrent operation is detected. |
Comments suppressed due to low confidence (2)
src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.cs:150
- The redundant assignment of _state to PipeState.Closed has been removed by moving the state change earlier in the method. Please confirm that all code paths relying on _state consider the new disposal order.
_state = PipeState.Closed;
src/libraries/System.IO/Pipes/src/System/IO/Pipes/PipeStream.Windows.cs:272
- Switching from calling _preallocatedOverlapped.Dispose() to source.Dispose() may have additional side effects if source.Dispose() encompasses extra disposal logic. Verify that invoking source.Dispose() here safely and correctly releases all related resources.
source.Dispose();
Tagging subscribers to this area: @dotnet/area-system-io |
1188e15
to
826dcf6
Compare
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.
LGTM. thanks for adding a very detailed explanation @stephentoub 👍
…g operation The same pattern occurs in a couple of other places, as well.
45ea4a2
to
0c4f796
Compare
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.
Pull Request Overview
This PR fixes resource leaks by ensuring pending operations on disposed pipes are cleaned up correctly on Windows and Unix. Key changes include:
- Updating
TryToReuse
methods to callDispose()
on overlapped/task sources and clear reusable fields when closed. - Marking the pipe as closed earlier in
PipeStream.Dispose
to prevent races during disposal. - Adjusting Unix implementation of
DisposeCore
to cancel internal tokens only during managed disposal.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/.../SafeFileHandle.OverlappedValueTaskSource.Windows.cs | TryToReuse now calls Dispose() on the source and clears pool on close |
src/.../PipeStream.cs | Moved _state = PipeState.Closed before calling into DisposeCore to avoid races |
src/.../PipeStream.Windows.cs | Updated TryToReuse to dispose pooled PipeValueTaskSource instances when closed |
src/.../NamedPipeServerStream.Windows.cs | Similar TryToReuse update for connection value task sources |
src/.../NamedPipeServerStream.Unix.cs | Removed if (disposing) guard around _internalTokenSource.Cancel() |
Comments suppressed due to low confidence (2)
src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Unix.cs:140
- The removal of the
if (disposing)
guard causes_internalTokenSource.Cancel()
to run during finalization, which may race with managed disposal or throw on already-disposed objects. Restore theif (disposing)
check to ensure cancellation only occurs during explicit disposal.
if (State != PipeState.Closed)
src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.cs:150
- By setting
_state
outside of theif (disposing)
check, the finalizer path (disposing == false
) will also mark the stream closed, potentially hiding cleanup errors or causing incorrect behavior. Consider moving this assignment inside theif (disposing)
block or explicitly documenting that it applies to both managed and unmanaged disposal paths.
_state = PipeState.Closed;
/backport to release/9.0-staging |
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/15370998142 |
/backport to release/8.0-staging |
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/15370999069 |
The same pattern occurs in a few places.
Repro:
Before:
After: