From b4407b457d75e3e5ce8f7ad5ceccc2213cd2e889 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 9 Mar 2020 08:07:25 +0100 Subject: [PATCH 1/2] Remove unnecesary branch from ReadTag --- csharp/src/Google.Protobuf.Test/IssuesTest.cs | 21 ++++++++++++++++++ .../src/Google.Protobuf/CodedInputStream.cs | 22 ++++++++----------- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/csharp/src/Google.Protobuf.Test/IssuesTest.cs b/csharp/src/Google.Protobuf.Test/IssuesTest.cs index 2caf80a93fa4..ae6fade30139 100644 --- a/csharp/src/Google.Protobuf.Test/IssuesTest.cs +++ b/csharp/src/Google.Protobuf.Test/IssuesTest.cs @@ -33,6 +33,7 @@ using Google.Protobuf.Reflection; using UnitTest.Issues.TestProtos; using NUnit.Framework; +using System.IO; using static UnitTest.Issues.TestProtos.OneofMerging.Types; namespace Google.Protobuf @@ -90,5 +91,25 @@ public void OneofMerging() merged.MergeFrom(message2); Assert.AreEqual(expected, merged); } + + // See https://github.com/protocolbuffers/protobuf/pull/7289 + [Test] + public void CodedInputStream_LimitReachedRightAfterTag() + { + MemoryStream ms = new MemoryStream(); + var cos = new CodedOutputStream(ms); + cos.WriteTag(11, WireFormat.WireType.Varint); + Assert.AreEqual(1, cos.Position); + cos.WriteString("some extra padding"); // ensure is currentLimit distinct from the end of the buffer. + cos.Flush(); + + var cis = new CodedInputStream(ms.ToArray()); + cis.PushLimit(1); // make sure we reach the limit right after reading the tag. + + // we still must read the tag correctly, even though the tag is at the very end of our limited input + // (which is a corner case and will most likely result in an error when trying to read value of the field + // decribed by this tag, but it would be a logical error not to read the tag that's actually present). + cis.AssertNextTag(WireFormat.MakeTag(11, WireFormat.WireType.Varint)); + } } } diff --git a/csharp/src/Google.Protobuf/CodedInputStream.cs b/csharp/src/Google.Protobuf/CodedInputStream.cs index b9feda53cbc5..9976f5823480 100644 --- a/csharp/src/Google.Protobuf/CodedInputStream.cs +++ b/csharp/src/Google.Protobuf/CodedInputStream.cs @@ -308,16 +308,16 @@ internal void CheckReadEndOfStreamTag() } } - internal void CheckLastTagWas(uint expectedTag) - { - if (lastTag != expectedTag) { - throw InvalidProtocolBufferException.InvalidEndTag(); - } - } + internal void CheckLastTagWas(uint expectedTag) + { + if (lastTag != expectedTag) { + throw InvalidProtocolBufferException.InvalidEndTag(); + } + } #endregion - + #region Reading of tags etc - + /// /// Peeks at the next field tag. This is like calling , but the /// tag is not consumed. (So a subsequent call to will return the @@ -395,10 +395,6 @@ public uint ReadTag() // If we actually read a tag with a field of 0, that's not a valid tag. throw InvalidProtocolBufferException.InvalidTag(); } - if (ReachedLimit) - { - return 0; - } return lastTag; } @@ -644,7 +640,7 @@ public void ReadGroup(IMessage builder) } ++recursionDepth; - uint tag = lastTag; + uint tag = lastTag; int fieldNumber = WireFormat.GetTagFieldNumber(tag); builder.MergeFrom(this); From a33b05687fd48cf64862f9acab6bef810f21d320 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Wed, 11 Mar 2020 07:00:22 -0400 Subject: [PATCH 2/2] address comments --- csharp/src/Google.Protobuf.Test/IssuesTest.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/csharp/src/Google.Protobuf.Test/IssuesTest.cs b/csharp/src/Google.Protobuf.Test/IssuesTest.cs index ae6fade30139..941bce016099 100644 --- a/csharp/src/Google.Protobuf.Test/IssuesTest.cs +++ b/csharp/src/Google.Protobuf.Test/IssuesTest.cs @@ -92,7 +92,7 @@ public void OneofMerging() Assert.AreEqual(expected, merged); } - // See https://github.com/protocolbuffers/protobuf/pull/7289 + // Check that a tag immediately followed by end of limit can still be read. [Test] public void CodedInputStream_LimitReachedRightAfterTag() { @@ -109,6 +109,7 @@ public void CodedInputStream_LimitReachedRightAfterTag() // we still must read the tag correctly, even though the tag is at the very end of our limited input // (which is a corner case and will most likely result in an error when trying to read value of the field // decribed by this tag, but it would be a logical error not to read the tag that's actually present). + // See https://github.com/protocolbuffers/protobuf/pull/7289 cis.AssertNextTag(WireFormat.MakeTag(11, WireFormat.WireType.Varint)); } }