From 15bf55e2251df4ad798a4f88b8514899c26e3dc3 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Tue, 4 Aug 2015 12:38:53 +0100 Subject: [PATCH] Validate that after reading a message, we've consumed as many bytes as we expected to. We should now have no conformance failures. --- conformance/failure_list_csharp.txt | 22 -------------- .../GeneratedMessageTest.cs | 26 ++++++++++++----- .../src/Google.Protobuf/CodedInputStream.cs | 29 +++++++++++++++++++ 3 files changed, 47 insertions(+), 30 deletions(-) diff --git a/conformance/failure_list_csharp.txt b/conformance/failure_list_csharp.txt index 34846589..e69de29b 100644 --- a/conformance/failure_list_csharp.txt +++ b/conformance/failure_list_csharp.txt @@ -1,22 +0,0 @@ -ProtobufInput.PrematureEofBeforeUnknownValue.BOOL -ProtobufInput.PrematureEofBeforeUnknownValue.BYTES -ProtobufInput.PrematureEofBeforeUnknownValue.DOUBLE -ProtobufInput.PrematureEofBeforeUnknownValue.ENUM -ProtobufInput.PrematureEofBeforeUnknownValue.FIXED32 -ProtobufInput.PrematureEofBeforeUnknownValue.FIXED64 -ProtobufInput.PrematureEofBeforeUnknownValue.FLOAT -ProtobufInput.PrematureEofBeforeUnknownValue.INT32 -ProtobufInput.PrematureEofBeforeUnknownValue.INT64 -ProtobufInput.PrematureEofBeforeUnknownValue.MESSAGE -ProtobufInput.PrematureEofBeforeUnknownValue.SFIXED32 -ProtobufInput.PrematureEofBeforeUnknownValue.SFIXED64 -ProtobufInput.PrematureEofBeforeUnknownValue.SINT32 -ProtobufInput.PrematureEofBeforeUnknownValue.SINT64 -ProtobufInput.PrematureEofBeforeUnknownValue.STRING -ProtobufInput.PrematureEofBeforeUnknownValue.UINT32 -ProtobufInput.PrematureEofBeforeUnknownValue.UINT64 -ProtobufInput.PrematureEofInDelimitedDataForKnownNonRepeatedValue.MESSAGE -ProtobufInput.PrematureEofInDelimitedDataForKnownRepeatedValue.MESSAGE -ProtobufInput.PrematureEofInDelimitedDataForUnknownValue.BYTES -ProtobufInput.PrematureEofInDelimitedDataForUnknownValue.MESSAGE -ProtobufInput.PrematureEofInDelimitedDataForUnknownValue.STRING diff --git a/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.cs b/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.cs index 1ef3d753..6fdd1066 100644 --- a/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.cs +++ b/csharp/src/Google.Protobuf.Test/GeneratedMessageTest.cs @@ -259,7 +259,7 @@ namespace Google.Protobuf output.WriteTag(TestMap.MapInt32ForeignMessageFieldNumber, WireFormat.WireType.LengthDelimited); var nestedMessage = new ForeignMessage { C = 20 }; // Size of the entry (tag, size written by WriteMessage, data written by WriteMessage) - output.WriteRawVarint32((uint)(nestedMessage.CalculateSize() + 3)); + output.WriteLength(2 + nestedMessage.CalculateSize()); output.WriteTag(2, WireFormat.WireType.LengthDelimited); output.WriteMessage(nestedMessage); output.Flush(); @@ -283,7 +283,7 @@ namespace Google.Protobuf // Each field can be represented in a single byte, with a single byte tag. // Total message size: 6 bytes. - output.WriteRawVarint32(6); + output.WriteLength(6); output.WriteTag(1, WireFormat.WireType.Varint); output.WriteInt32(key); output.WriteTag(2, WireFormat.WireType.Varint); @@ -309,7 +309,7 @@ namespace Google.Protobuf // Each field can be represented in a single byte, with a single byte tag. // Total message size: 4 bytes. - output.WriteRawVarint32(4); + output.WriteLength(4); output.WriteTag(2, WireFormat.WireType.Varint); output.WriteInt32(value); output.WriteTag(1, WireFormat.WireType.Varint); @@ -335,7 +335,7 @@ namespace Google.Protobuf var key1 = 10; var value1 = 20; output.WriteTag(TestMap.MapInt32Int32FieldNumber, WireFormat.WireType.LengthDelimited); - output.WriteRawVarint32(4); + output.WriteLength(4); output.WriteTag(1, WireFormat.WireType.Varint); output.WriteInt32(key1); output.WriteTag(2, WireFormat.WireType.Varint); @@ -345,7 +345,7 @@ namespace Google.Protobuf var key2 = "a"; var value2 = "b"; output.WriteTag(TestMap.MapStringStringFieldNumber, WireFormat.WireType.LengthDelimited); - output.WriteRawVarint32(6); // 3 bytes per entry: tag, size, character + output.WriteLength(6); // 3 bytes per entry: tag, size, character output.WriteTag(1, WireFormat.WireType.LengthDelimited); output.WriteString(key2); output.WriteTag(2, WireFormat.WireType.LengthDelimited); @@ -355,7 +355,7 @@ namespace Google.Protobuf var key3 = 15; var value3 = 25; output.WriteTag(TestMap.MapInt32Int32FieldNumber, WireFormat.WireType.LengthDelimited); - output.WriteRawVarint32(4); + output.WriteLength(4); output.WriteTag(1, WireFormat.WireType.Varint); output.WriteInt32(key3); output.WriteTag(2, WireFormat.WireType.Varint); @@ -383,7 +383,7 @@ namespace Google.Protobuf // First entry output.WriteTag(TestMap.MapInt32Int32FieldNumber, WireFormat.WireType.LengthDelimited); - output.WriteRawVarint32(4); + output.WriteLength(4); output.WriteTag(1, WireFormat.WireType.Varint); output.WriteInt32(key); output.WriteTag(2, WireFormat.WireType.Varint); @@ -391,7 +391,7 @@ namespace Google.Protobuf // Second entry - same key, different value output.WriteTag(TestMap.MapInt32Int32FieldNumber, WireFormat.WireType.LengthDelimited); - output.WriteRawVarint32(4); + output.WriteLength(4); output.WriteTag(1, WireFormat.WireType.Varint); output.WriteInt32(key); output.WriteTag(2, WireFormat.WireType.Varint); @@ -619,5 +619,15 @@ namespace Google.Protobuf var empty = Empty.Parser.ParseFrom(data); Assert.AreEqual(new Empty(), empty); } + + // This was originally seen as a conformance test failure. + [Test] + public void TruncatedMessageFieldThrows() + { + // 130, 3 is the message tag + // 1 is the data length - but there's no data. + var data = new byte[] { 130, 3, 1 }; + Assert.Throws(() => TestAllTypes.Parser.ParseFrom(data)); + } } } diff --git a/csharp/src/Google.Protobuf/CodedInputStream.cs b/csharp/src/Google.Protobuf/CodedInputStream.cs index fee31e3b..5da03b5c 100644 --- a/csharp/src/Google.Protobuf/CodedInputStream.cs +++ b/csharp/src/Google.Protobuf/CodedInputStream.cs @@ -54,13 +54,37 @@ namespace Google.Protobuf /// public sealed class CodedInputStream { + /// + /// Buffer of data read from the stream or provided at construction time. + /// private readonly byte[] buffer; + + /// + /// The number of valid bytes in the buffer. + /// private int bufferSize; + private int bufferSizeAfterLimit = 0; + /// + /// The position within the current buffer (i.e. the next byte to read) + /// private int bufferPos = 0; + + /// + /// The stream to read further input from, or null if the byte array buffer was provided + /// directly on construction, with no further data available. + /// private readonly Stream input; + + /// + /// The last tag we read. 0 indicates we've read to the end of the stream + /// (or haven't read anything yet). + /// private uint lastTag = 0; + /// + /// The next tag, used to store the value read by PeekTag. + /// private uint nextTag = 0; private bool hasNextTag = false; @@ -456,6 +480,11 @@ namespace Google.Protobuf ++recursionDepth; builder.MergeFrom(this); CheckLastTagWas(0); + // Check that we've read exactly as much data as expected. + if (!ReachedLimit) + { + throw InvalidProtocolBufferException.TruncatedMessage(); + } --recursionDepth; PopLimit(oldLimit); }