Reject trailing bytes in unary Protobuf payloads#4553
Conversation
A unary Protobuf payload must contain exactly one length-prefixed message. DecodeProtobufMessageAsync returned after the first message and silently let the pipe reader discard any remaining bytes, so concatenated frames or trailing garbage went unnoticed instead of surfacing as a framing error.
There was a problem hiding this comment.
Pull request overview
This PR tightens unary Protobuf framing by ensuring a unary payload contains exactly one length-prefixed message and fails fast with InvalidDataException when extra bytes follow, addressing audit finding #4438.
Changes:
- Add a post-decode read in
DecodeProtobufMessageAsyncto detect and reject any trailing bytes after the single expected message. - Add unit tests covering trailing garbage bytes and concatenated framed messages in a unary payload.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/IceRpc.Protobuf/RpcMethods/Internal/PipeReaderExtensions.cs |
Enforces “exactly one message” semantics for unary Protobuf payload decoding by rejecting trailing bytes. |
tests/IceRpc.Protobuf.Tests/PipeReaderExtensionsTests.cs |
Adds new tests and a helper to construct framed Protobuf payloads with trailing data. |
| using Google.Protobuf.WellKnownTypes; | ||
| using IceRpc.Protobuf.RpcMethods.Internal; | ||
| using NUnit.Framework; | ||
| using System.Buffers; |
There was a problem hiding this comment.
using System.Buffers; is not used in this test file. The repo has TreatWarningsAsErrors=true, so this unused using will fail the build; please remove it (or use the type that requires it).
| using System.Buffers; |
| writer.Advance(4); | ||
| BinaryPrimitives.WriteInt32BigEndian(lengthBytes, message.CalculateSize()); |
There was a problem hiding this comment.
PipeWriter.GetSpan(4) should be written to before calling Advance(4). Advancing first tells the writer these 4 bytes are already initialized, which can violate the PipeWriter contract and makes the test helper brittle.
| writer.Advance(4); | |
| BinaryPrimitives.WriteInt32BigEndian(lengthBytes, message.CalculateSize()); | |
| BinaryPrimitives.WriteInt32BigEndian(lengthBytes, message.CalculateSize()); | |
| writer.Advance(4); |
InsertCreativityHere
left a comment
InsertCreativityHere
left a comment
There was a problem hiding this comment.
Looks good to me!
The previous order called Advance(4) on the placeholder span, then filled it via BinaryPrimitives.WriteInt32BigEndian. The PipeWriter contract treats Advance as a commit of bytes that have already been written; the back-patch idiom only applies when the value isn't known until later. The test helper has the message size up front, so write the length first and advance once.
Summary
DecodeProtobufMessageAsyncnow performs a final read after the single expected message and throwsInvalidDataExceptionif trailing bytes remain, so concatenated frames or junk after the message surface as a framing error instead of being silently discarded.Fixes #4438