From 50fa4a6def543e61af53c30b24585fd33939f9b8 Mon Sep 17 00:00:00 2001 From: Jose Date: Fri, 24 Apr 2026 13:26:26 +0200 Subject: [PATCH 1/2] Reject trailing bytes in unary Protobuf payloads 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. --- .../Internal/PipeReaderExtensions.cs | 17 +++++++ .../PipeReaderExtensionsTests.cs | 48 +++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/src/IceRpc.Protobuf/RpcMethods/Internal/PipeReaderExtensions.cs b/src/IceRpc.Protobuf/RpcMethods/Internal/PipeReaderExtensions.cs index 86ce27f48a..5e03495283 100644 --- a/src/IceRpc.Protobuf/RpcMethods/Internal/PipeReaderExtensions.cs +++ b/src/IceRpc.Protobuf/RpcMethods/Internal/PipeReaderExtensions.cs @@ -31,6 +31,23 @@ internal static async ValueTask DecodeProtobufMessageAsync( cancellationToken).ConfigureAwait(false); Debug.Assert(message is not null); + + // A unary payload must contain exactly one message; any trailing bytes indicate a framing error. + ReadResult readResult = await reader.ReadAsync(cancellationToken).ConfigureAwait(false); + // We never call CancelPendingRead; an interceptor or middleware can but it's not correct. + if (readResult.IsCanceled) + { + throw new InvalidOperationException("Unexpected call to CancelPendingRead."); + } + bool hasTrailingBytes = !readResult.Buffer.IsEmpty; + reader.AdvanceTo(readResult.Buffer.End); + if (hasTrailingBytes) + { + throw new InvalidDataException( + "The payload contains unexpected trailing bytes after the Protobuf message."); + } + Debug.Assert(readResult.IsCompleted); + return message; } diff --git a/tests/IceRpc.Protobuf.Tests/PipeReaderExtensionsTests.cs b/tests/IceRpc.Protobuf.Tests/PipeReaderExtensionsTests.cs index 26a6704204..f6bfd783bd 100644 --- a/tests/IceRpc.Protobuf.Tests/PipeReaderExtensionsTests.cs +++ b/tests/IceRpc.Protobuf.Tests/PipeReaderExtensionsTests.cs @@ -1,8 +1,11 @@ // Copyright (c) ZeroC, Inc. +using Google.Protobuf; using Google.Protobuf.WellKnownTypes; using IceRpc.Protobuf.RpcMethods.Internal; using NUnit.Framework; +using System.Buffers; +using System.Buffers.Binary; using System.IO.Pipelines; namespace IceRpc.Protobuf.Tests; @@ -26,4 +29,49 @@ await pipeReader.DecodeProtobufMessageAsync( CancellationToken.None)); pipeReader.Complete(); } + + [Test] + public void Decode_message_throws_invalid_data_exception_when_payload_has_trailing_bytes() + { + // Arrange + var pipe = new Pipe(); + WriteLengthPrefixedMessage(pipe.Writer, new StringValue { Value = "hello" }); + pipe.Writer.Write(new byte[] { 0xDE, 0xAD, 0xBE, 0xEF }); + pipe.Writer.Complete(); + + // Act & Assert + Assert.ThrowsAsync(async () => + await pipe.Reader.DecodeProtobufMessageAsync( + StringValue.Parser, + maxMessageLength: 1024, + CancellationToken.None)); + pipe.Reader.Complete(); + } + + [Test] + public void Decode_message_throws_invalid_data_exception_when_payload_has_concatenated_messages() + { + // Arrange + var pipe = new Pipe(); + WriteLengthPrefixedMessage(pipe.Writer, new StringValue { Value = "hello" }); + WriteLengthPrefixedMessage(pipe.Writer, new StringValue { Value = "world" }); + pipe.Writer.Complete(); + + // Act & Assert + Assert.ThrowsAsync(async () => + await pipe.Reader.DecodeProtobufMessageAsync( + StringValue.Parser, + maxMessageLength: 1024, + CancellationToken.None)); + pipe.Reader.Complete(); + } + + private static void WriteLengthPrefixedMessage(PipeWriter writer, IMessage message) + { + writer.Write(new byte[] { 0 }); // Not compressed + Span lengthBytes = writer.GetSpan(4); + writer.Advance(4); + BinaryPrimitives.WriteInt32BigEndian(lengthBytes, message.CalculateSize()); + message.WriteTo(writer); + } } From 74b30b1c0cf6029289d6cc5a849f566dfe809b19 Mon Sep 17 00:00:00 2001 From: Jose Date: Mon, 27 Apr 2026 10:52:40 +0200 Subject: [PATCH 2/2] Write the length placeholder before advancing the pipe writer 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. --- tests/IceRpc.Protobuf.Tests/PipeReaderExtensionsTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/IceRpc.Protobuf.Tests/PipeReaderExtensionsTests.cs b/tests/IceRpc.Protobuf.Tests/PipeReaderExtensionsTests.cs index f6bfd783bd..2aaff7d7a2 100644 --- a/tests/IceRpc.Protobuf.Tests/PipeReaderExtensionsTests.cs +++ b/tests/IceRpc.Protobuf.Tests/PipeReaderExtensionsTests.cs @@ -70,8 +70,8 @@ private static void WriteLengthPrefixedMessage(PipeWriter writer, IMessage messa { writer.Write(new byte[] { 0 }); // Not compressed Span lengthBytes = writer.GetSpan(4); - writer.Advance(4); BinaryPrimitives.WriteInt32BigEndian(lengthBytes, message.CalculateSize()); + writer.Advance(4); message.WriteTo(writer); } }