From c298c89229a4f3d2ec19f431de2f46cc597142af Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Sat, 30 May 2009 10:07:09 +0100 Subject: New tests, double build errors, and a few miscellaneous fixes --- src/ProtocolBuffers.Test/CodedInputStreamTest.cs | 66 ++++++++++++++++++++++- src/ProtocolBuffers.Test/DynamicMessageTest.cs | 13 +++++ src/ProtocolBuffers.Test/GeneratedMessageTest.cs | 14 ++++- src/ProtocolBuffers/AbstractBuilder.cs | 2 +- src/ProtocolBuffers/CodedInputStream.cs | 33 +++++++++--- src/ProtocolBuffers/Descriptors/DescriptorPool.cs | 3 +- src/ProtocolBuffers/DynamicMessage.cs | 5 +- src/ProtocolBuffers/GeneratedBuilder.cs | 3 +- 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); } /// @@ -77,7 +89,7 @@ namespace Google.ProtocolBuffers { /// expects them to fail with an InvalidProtocolBufferException whose /// description matches the given one. /// - 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 { } } + /// + /// Test that a bug in SkipRawBytes has been fixed: if the skip + /// skips exactly up to a limit, this should bnot break things + /// + [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. + } + } + /// /// 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] @@ -56,6 +57,18 @@ namespace Google.ProtocolBuffers { reflectionTester.AssertAllFieldsSetViaReflection(message); } + [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); 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 @@ -90,6 +90,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); @@ -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 { /// /// /// - 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; + } + } + } + + /// + /// Abstraction of skipping to cope with streams which can't really skip. + /// + 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); } /// 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 . /// 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(); -- cgit v1.2.3