Remove unnecessary branch from ReadTag (#7289)

* Remove unnecesary branch from ReadTag * address comments
protocolbuffers · Mar 11, 2020 · a0cc0e8 · a0cc0e8
1 parent 9bccb25
commit a0cc0e8
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 13 deletions.
diff --git 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,26 @@ public void OneofMerging()
             merged.MergeFrom(message2);
             Assert.AreEqual(expected, merged);
         }
+
+        // Check that a tag immediately followed by end of limit can still be read.
+        [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).
+            // See https://github.com/protocolbuffers/protobuf/pull/7289
+            cis.AssertNextTag(WireFormat.MakeTag(11, WireFormat.WireType.Varint));
+        }
     }
 }
diff --git 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
-
+
         /// <summary>
         /// Peeks at the next field tag. This is like calling <see cref="ReadTag"/>, but the
         /// tag is not consumed. (So a subsequent call to <see cref="ReadTag"/> 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);