New tests, double build errors, and a few miscellaneous fixes

8 files changed, 124 insertions, 15 deletions

diff --git a/src/ProtocolBuffers.Test/CodedInputStreamTest.cs b/src/ProtocolBuffers.Test/CodedInputStreamTest.cs
index ce4a6683..2ad8c45b 100644
--- a/src/ProtocolBuffers.Test/CodedInputStreamTest.cs
+++ b/src/ProtocolBuffers.Test/CodedInputStreamTest.cs
@@ -61,6 +61,7 @@ namespace Google.ProtocolBuffers {
 
       input = CodedInputStream.CreateInstance(data);
       Assert.AreEqual(value, input.ReadRawVarint64());
+      Assert.IsTrue(input.IsAtEnd);
 
       // Try different block sizes.
       for (int bufferSize = 1; bufferSize <= 16; bufferSize *= 2) {
@@ -69,7 +70,18 @@ namespace Google.ProtocolBuffers {
 
         input = CodedInputStream.CreateInstance(new SmallBlockInputStream(data, bufferSize));
         Assert.AreEqual(value, input.ReadRawVarint64());
+        Assert.IsTrue(input.IsAtEnd);
       }
+
+      // Try reading directly from a MemoryStream. We want to verify that it
+      // doesn't read past the end of the input, so write an extra byte - this
+      // lets us test the position at the end.
+      MemoryStream memoryStream = new MemoryStream();
+      memoryStream.Write(data, 0, data.Length);
+      memoryStream.WriteByte(0);
+      memoryStream.Position = 0;
+      Assert.AreEqual((uint)value, CodedInputStream.ReadRawVarint32(memoryStream));
+      Assert.AreEqual(data.Length, memoryStream.Position);
     }
 
     /// <summary>
@@ -77,7 +89,7 @@ namespace Google.ProtocolBuffers {
     /// expects them to fail with an InvalidProtocolBufferException whose
     /// description matches the given one.
     /// </summary>
-    private void AssertReadVarintFailure(InvalidProtocolBufferException expected, byte[] data) {
+    private static void AssertReadVarintFailure(InvalidProtocolBufferException expected, byte[] data) {
       CodedInputStream input = CodedInputStream.CreateInstance(data);
       try {
         input.ReadRawVarint32();
@@ -93,6 +105,14 @@ namespace Google.ProtocolBuffers {
       } catch (InvalidProtocolBufferException e) {
         Assert.AreEqual(expected.Message, e.Message);
       }
+
+      // Make sure we get the same error when reading directly from a Stream.
+      try {
+        CodedInputStream.ReadRawVarint32(new MemoryStream(data));
+        Assert.Fail("Should have thrown an exception.");
+      } catch (InvalidProtocolBufferException e) {
+        Assert.AreEqual(expected.Message, e.Message);
+      }
     }
 
     [Test]
@@ -139,12 +159,14 @@ namespace Google.ProtocolBuffers {
     private static void AssertReadLittleEndian32(byte[] data, uint value) {
       CodedInputStream input = CodedInputStream.CreateInstance(data);
       Assert.AreEqual(value, input.ReadRawLittleEndian32());
+      Assert.IsTrue(input.IsAtEnd);
 
       // Try different block sizes.
       for (int blockSize = 1; blockSize <= 16; blockSize *= 2) {
         input = CodedInputStream.CreateInstance(
           new SmallBlockInputStream(data, blockSize));
         Assert.AreEqual(value, input.ReadRawLittleEndian32());
+        Assert.IsTrue(input.IsAtEnd);
       }
     }
 
@@ -155,12 +177,14 @@ namespace Google.ProtocolBuffers {
     private static void AssertReadLittleEndian64(byte[] data, ulong value) {
       CodedInputStream input = CodedInputStream.CreateInstance(data);
       Assert.AreEqual(value, input.ReadRawLittleEndian64());
+      Assert.IsTrue(input.IsAtEnd);
 
       // Try different block sizes.
       for (int blockSize = 1; blockSize <= 16; blockSize *= 2) {
         input = CodedInputStream.CreateInstance(
           new SmallBlockInputStream(data, blockSize));
         Assert.AreEqual(value, input.ReadRawLittleEndian64());
+        Assert.IsTrue(input.IsAtEnd);
       }
     }
 
@@ -239,6 +263,21 @@ namespace Google.ProtocolBuffers {
       }
     }
 
+    /// <summary>
+    /// Test that a bug in SkipRawBytes has been fixed: if the skip
+    /// skips exactly up to a limit, this should bnot break things
+    /// </summary>
+    [Test]
+    public void SkipRawBytesBug() {
+      byte[] rawBytes = new byte[] { 1, 2 };
+      CodedInputStream input = CodedInputStream.CreateInstance(rawBytes);
+
+      int limit = input.PushLimit(1);
+      input.SkipRawBytes(1);
+      input.PopLimit(limit);
+      Assert.AreEqual(2, input.ReadRawByte());
+    }
+
     public void ReadHugeBlob() {
       // Allocate and initialize a 1MB blob.
       byte[] blob = new byte[1 << 20];
@@ -348,6 +387,31 @@ namespace Google.ProtocolBuffers {
       }
     }
 
+    [Test]
+    public void ResetSizeCounter() {
+      CodedInputStream input = CodedInputStream.CreateInstance(
+          new SmallBlockInputStream(new byte[256], 8));
+      input.SetSizeLimit(16);
+      input.ReadRawBytes(16);
+
+      try {
+        input.ReadRawByte();
+        Assert.Fail("Should have thrown an exception!");
+      } catch (InvalidProtocolBufferException e) {
+        // Success.
+      }
+
+      input.ResetSizeCounter();
+      input.ReadRawByte();  // No exception thrown.
+
+      try {
+        input.ReadRawBytes(16);  // Hits limit again.
+        Assert.Fail("Should have thrown an exception!");
+      } catch (InvalidProtocolBufferException e) {
+        // Success.
+      }
+    }
+
     /// <summary>
     /// Tests that if we read an string that contains invalid UTF-8, no exception
     /// is thrown.  Instead, the invalid bytes are replaced with the Unicode
diff --git a/src/ProtocolBuffers.Test/DynamicMessageTest.cs b/src/ProtocolBuffers.Test/DynamicMessageTest.cs
index edcce38e..2f3fefa8 100644
--- a/src/ProtocolBuffers.Test/DynamicMessageTest.cs
+++ b/src/ProtocolBuffers.Test/DynamicMessageTest.cs
@@ -32,6 +32,7 @@ using Google.ProtocolBuffers.TestProtos;
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 using NUnit.Framework;
+using System;
 
 namespace Google.ProtocolBuffers {
   [TestFixture]
@@ -57,6 +58,18 @@ namespace Google.ProtocolBuffers {
     }
 
     [Test]
+    public void DoubleBuildError() {
+      DynamicMessage.Builder builder = DynamicMessage.CreateBuilder(TestAllTypes.Descriptor);
+      builder.Build();
+      try {
+        builder.Build();
+        Assert.Fail("Should have thrown exception.");
+      } catch (InvalidOperationException e) {
+        // Success.
+      }
+    }
+
+    [Test]
     public void DynamicMessageSettersRejectNull() {
       IBuilder builder = DynamicMessage.CreateBuilder(TestAllTypes.Descriptor);
       reflectionTester.AssertReflectionSettersRejectNull(builder);
diff --git a/src/ProtocolBuffers.Test/GeneratedMessageTest.cs b/src/ProtocolBuffers.Test/GeneratedMessageTest.cs
index 3abf5bff..1980bbc1 100644
--- a/src/ProtocolBuffers.Test/GeneratedMessageTest.cs
+++ b/src/ProtocolBuffers.Test/GeneratedMessageTest.cs
@@ -91,6 +91,18 @@ namespace Google.ProtocolBuffers {
     }
 
     [Test]
+    public void DoubleBuildError() {
+      TestAllTypes.Builder builder = new TestAllTypes.Builder();
+      builder.Build();
+      try {
+        builder.Build();
+        Assert.Fail("Should have thrown exception.");
+      } catch (InvalidOperationException e) {
+        // Success.
+      }
+    }
+
+    [Test]
     public void DefaultInstance() {
       Assert.AreSame(TestAllTypes.DefaultInstance, TestAllTypes.DefaultInstance.DefaultInstanceForType);
       Assert.AreSame(TestAllTypes.DefaultInstance, TestAllTypes.CreateBuilder().DefaultInstanceForType);
@@ -367,7 +379,7 @@ namespace Google.ProtocolBuffers {
     }
 
     [Test]
-    public void TestOptimizedForSizeMergeUsesAllFieldsFromTarget() {
+    public void OptimizedForSizeMergeUsesAllFieldsFromTarget() {
       TestOptimizedForSize withFieldSet = new TestOptimizedForSize.Builder { I = 10 }.Build();
       TestOptimizedForSize.Builder builder = new TestOptimizedForSize.Builder();
       builder.MergeFrom(withFieldSet);
diff --git a/src/ProtocolBuffers/AbstractBuilder.cs b/src/ProtocolBuffers/AbstractBuilder.cs
index d7831cd1..8c852517 100644
--- a/src/ProtocolBuffers/AbstractBuilder.cs
+++ b/src/ProtocolBuffers/AbstractBuilder.cs
@@ -226,7 +226,7 @@ namespace Google.ProtocolBuffers {
     }
 
     public TBuilder MergeDelimitedFrom(Stream input, ExtensionRegistry extensionRegistry) {
-      int size = CodedInputStream.ReadRawVarint32(input);
+      int size = (int) CodedInputStream.ReadRawVarint32(input);
       Stream limitedStream = new LimitedInputStream(input, size);
       return MergeFrom(limitedStream, extensionRegistry);
     }
diff --git a/src/ProtocolBuffers/CodedInputStream.cs b/src/ProtocolBuffers/CodedInputStream.cs
index 89d8e33e..04308554 100644
--- a/src/ProtocolBuffers/CodedInputStream.cs
+++ b/src/ProtocolBuffers/CodedInputStream.cs
@@ -466,7 +466,7 @@ namespace Google.ProtocolBuffers {
     /// </summary>
     /// <param name="input"></param>
     /// <returns></returns>
-    internal static int ReadRawVarint32(Stream input) {
+    internal static uint ReadRawVarint32(Stream input) {
       int result = 0;
       int offset = 0;
       for (; offset < 32; offset += 7) {
@@ -476,7 +476,7 @@ namespace Google.ProtocolBuffers {
         }
         result |= (b & 0x7f) << offset;
         if ((b & 0x80) == 0) {
-          return result;
+          return (uint) result;
         }
       }
       // Keep reading up to 64 bits.
@@ -486,7 +486,7 @@ namespace Google.ProtocolBuffers {
           throw InvalidProtocolBufferException.TruncatedMessage();
         }
         if ((b & 0x80) == 0) {
-          return result;
+          return (uint) result;
         }
       }
       throw InvalidProtocolBufferException.MalformedVarint();
@@ -918,16 +918,33 @@ namespace Google.ProtocolBuffers {
 
         // Then skip directly from the InputStream for the rest.
         if (pos < size) {
-          // TODO(jonskeet): Java implementation uses skip(). Not sure whether this is really equivalent...
           if (input == null) {
             throw InvalidProtocolBufferException.TruncatedMessage();
           }
-          long previousPosition = input.Position;
-          input.Position += size - pos;
-          if (input.Position != previousPosition + size - pos) {
+          SkipImpl(size - pos);
+          totalBytesRetired += size - pos;
+        }
+      }
+    }
+
+    /// <summary>
+    /// Abstraction of skipping to cope with streams which can't really skip.
+    /// </summary>
+    private void SkipImpl(int amountToSkip) {
+      if (input.CanSeek) {
+        long previousPosition = input.Position;
+        input.Position += amountToSkip;
+        if (input.Position != previousPosition + amountToSkip) {
+          throw InvalidProtocolBufferException.TruncatedMessage();
+        }
+      } else {
+        byte[] skipBuffer = new byte[1024];
+        while (amountToSkip > 0) {
+          int bytesRead = input.Read(skipBuffer, 0, skipBuffer.Length);
+          if (bytesRead <= 0) {
             throw InvalidProtocolBufferException.TruncatedMessage();
           }
-          totalBytesRetired += size - pos;
+          amountToSkip -= bytesRead;
         }
       }
     }
diff --git a/src/ProtocolBuffers/Descriptors/DescriptorPool.cs b/src/ProtocolBuffers/Descriptors/DescriptorPool.cs
index 0e90770b..72dd1899 100644
--- a/src/ProtocolBuffers/Descriptors/DescriptorPool.cs
+++ b/src/ProtocolBuffers/Descriptors/DescriptorPool.cs
@@ -109,8 +109,7 @@ namespace Google.ProtocolBuffers.Descriptors {
             "package) in file \"" + old.File.Name + "\".");
         }
       }
-      // TODO(jonskeet): Check issue 25 wrt the ordering of these parameters
-      descriptorsByName[fullName] = new PackageDescriptor(fullName, name, file);
+      descriptorsByName[fullName] = new PackageDescriptor(name, fullName, file);
     }
 
     /// <summary>
diff --git a/src/ProtocolBuffers/DynamicMessage.cs b/src/ProtocolBuffers/DynamicMessage.cs
index 4b3a4060..16a7f319 100644
--- a/src/ProtocolBuffers/DynamicMessage.cs
+++ b/src/ProtocolBuffers/DynamicMessage.cs
@@ -296,7 +296,7 @@ namespace Google.ProtocolBuffers {
       }
 
       public override DynamicMessage Build() {
-   	    if (!IsInitialized) {
+        if (fields != null && !IsInitialized) {
           throw new UninitializedMessageException(new DynamicMessage(type, fields, unknownFields));
         }
         return BuildPartial();
@@ -315,6 +315,9 @@ namespace Google.ProtocolBuffers {
       }
 
       public override DynamicMessage BuildPartial() {
+        if (fields == null) {
+          throw new InvalidOperationException("Build() has already been called on this Builder.");
+        } 
         fields.MakeImmutable();
         DynamicMessage result = new DynamicMessage(type, fields, unknownFields);
         fields = null;
diff --git a/src/ProtocolBuffers/GeneratedBuilder.cs b/src/ProtocolBuffers/GeneratedBuilder.cs
index c3543be6..cdef93f6 100644
--- a/src/ProtocolBuffers/GeneratedBuilder.cs
+++ b/src/ProtocolBuffers/GeneratedBuilder.cs
@@ -187,7 +187,8 @@ namespace Google.ProtocolBuffers {
     /// Implementation of <see cref="IBuilder{TMessage, TBuilder}.Build" />.
     /// </summary>
     public override TMessage Build() {
-      if (!IsInitialized) {
+      // If the message is null, we'll throw a more appropriate exception in BuildPartial.
+      if (MessageBeingBuilt != null && !IsInitialized) {
         throw new UninitializedMessageException(MessageBeingBuilt);
       }
       return BuildPartial();