Fix two ice protocol connection bugs#4767
Merged
Merged
Conversation
Fixes icerpc/icerpc-csharp-audit#19. Fixes icerpc/icerpc-csharp-audit#32. - ReadFramesAsync validated only the upper bound of the peer-supplied frame size. A Request or Reply prologue with a frame size smaller than the prologue size (e.g. 0) made the frame body size negative, which threw ArgumentOutOfRangeException from ReadOnlySequence.Slice and reached the Debug.Fail in the generic exception handler: an assertion failure on untrusted input in debug builds, a less-clear connection abort in release builds. The frame size lower bound is now validated and rejected as invalid data. - When a reply was received in the same window the invocation observed its cancellation, the reply frame reader set on the response completion source (whose ownership the read frames loop had already transferred) was discarded without Complete: the pipe's rented memory pool segments were never returned. The invocation's finally block now completes the reader when it was never retrieved.
There was a problem hiding this comment.
Pull request overview
This PR addresses two robustness issues in IceProtocolConnection: validating malformed frame sizes early to avoid debug assertions/crashes on untrusted input, and ensuring reply frame readers are always completed to prevent pooled buffer leaks during a narrow cancellation race.
Changes:
- Reject incoming Ice frames whose
FrameSizeis smaller thanIceDefinitions.PrologueSize, treating it as malformed peer input. - Fix a cancellation race by completing a reply
PipeReaderwhen it was produced by the read loop but never retrieved by the canceled invocation. - Add a regression test that corrupts the incoming request prologue’s frame size to verify the connection aborts cleanly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/IceRpc.Tests/IceProtocolConnectionTests.cs | Adds a regression test for receiving a frame smaller than the Ice prologue size. |
| src/IceRpc/Internal/IceProtocolConnection.cs | Adds a lower-bound FrameSize check and completes leaked reply frame readers on a cancellation race. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pepone
commented
Jun 17, 2026
pepone
commented
Jun 17, 2026
pepone
commented
Jun 17, 2026
bernardnormier
approved these changes
Jun 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes icerpc/icerpc-csharp-audit#19.
Fixes icerpc/icerpc-csharp-audit#32.
Two small fixes in
IceProtocolConnection:Missing lower-bound check on the ice frame size (audit C# should prefer string interpolation to concatenation #19):
ReadFramesAsyncvalidated onlyFrameSize > _maxFrameSize. A Request/Reply prologue withFrameSizesmaller than the prologue size (14) madeframeSize - PrologueSizenegative, which threwArgumentOutOfRangeExceptionfromReadOnlySequence.Sliceand reached theDebug.Failin the generic exception handler — an assertion failure on untrusted input in debug builds. The lower bound is now rejected asInvalidDataException(malformed peer input), like the upper bound.Reply frame reader leak on a cancellation race (audit Remove Value factories, protected keyword and postMarshal/perUnmarshal callbacks #32): when the read frames loop completes
responseCompletionSourcewith the reply frame reader (transferring ownership — the loop no longer completes it) and the invocation'sWaitAsync(invocationCts.Token)observes cancellation in that same window, the reader was discarded withoutComplete(): the frame reader pipe's rentedMemoryPool<byte>segments were never returned. The invocation'sfinallynow completes the reader when the completion source holds a reader that was never retrieved (!responseCreated && frameReader is null). This also covers the read-loop-wins micro-race against thefinally's ownTrySetResult.Tests
Receiving_a_frame_smaller_than_the_prologue_size_aborts_the_connection: a server-side duplex read decorator rewrites the incoming request's frame size to 0; the connection aborts withConnectionAborted. Without the fix this test fails with the exact failure mode from the audit:ArgumentOutOfRangeExceptionfromSlicereachingDebug.FailinReadFramesAsync.finally's already-completed branch and is covered by review plus the existing suite (all response/cancellation paths still pass).Full
IceRpc.Testssuite passes (988 passed / 0 failed),dotnet buildanddotnet format --verify-no-changesclean.