From f2732c7af13110a72ded2667684f52a86a23de2f Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Mon, 10 Aug 2015 18:32:18 +0100 Subject: More TODOs done. - Removed a TODO without change in DescriptorPool.LookupSymbol - the TODOs were around performance, and this is only used during descriptor initialization - Make the CodedInputStream limits read-only, adding a static factory method for the rare cases when this is useful - Extracted IDeepCloneable into its own file. --- .../Google.Protobuf.Test/CodedInputStreamTest.cs | 48 +++---- .../Google.Protobuf.Test/CodedOutputStreamTest.cs | 2 +- csharp/src/Google.Protobuf/CodedInputStream.cs | 160 +++++++++++---------- csharp/src/Google.Protobuf/Google.Protobuf.csproj | 1 + csharp/src/Google.Protobuf/IDeepCloneable.cs | 54 +++++++ csharp/src/Google.Protobuf/IMessage.cs | 22 --- .../Google.Protobuf/Reflection/DescriptorPool.cs | 7 +- 7 files changed, 168 insertions(+), 126 deletions(-) create mode 100644 csharp/src/Google.Protobuf/IDeepCloneable.cs (limited to 'csharp') diff --git a/csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs b/csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs index c8326d80..54c44e47 100644 --- a/csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs +++ b/csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs @@ -320,38 +320,18 @@ namespace Google.Protobuf Assert.Throws(() => TestRecursiveMessage.Parser.ParseFrom(data65)); - CodedInputStream input = data64.CreateCodedInput(); - input.SetRecursionLimit(8); + CodedInputStream input = CodedInputStream.CreateWithLimits(new MemoryStream(data64.ToByteArray()), 1000000, 63); Assert.Throws(() => TestRecursiveMessage.Parser.ParseFrom(input)); } - /* [Test] public void SizeLimit() { // Have to use a Stream rather than ByteString.CreateCodedInput as SizeLimit doesn't // apply to the latter case. - MemoryStream ms = new MemoryStream(TestUtil.GetAllSet().ToByteString().ToByteArray()); - CodedInputStream input = new CodedInputStream(ms); - input.SetSizeLimit(16); - - Assert.Throws(() => TestAllTypes.ParseFrom(input)); - }*/ - - [Test] - public void ResetSizeCounter() - { - CodedInputStream input = new CodedInputStream( - new SmallBlockInputStream(new byte[256], 8)); - input.SetSizeLimit(16); - input.ReadRawBytes(16); - - Assert.Throws(() => input.ReadRawByte()); - - input.ResetSizeCounter(); - input.ReadRawByte(); // No exception thrown. - - Assert.Throws(() => input.ReadRawBytes(16)); + MemoryStream ms = new MemoryStream(SampleMessages.CreateFullTestAllTypes().ToByteArray()); + CodedInputStream input = CodedInputStream.CreateWithLimits(ms, 16, 100); + Assert.Throws(() => TestAllTypes.Parser.ParseFrom(input)); } /// @@ -423,7 +403,7 @@ namespace Google.Protobuf output.Flush(); ms.Position = 0; - CodedInputStream input = new CodedInputStream(ms, new byte[ms.Length / 2]); + CodedInputStream input = new CodedInputStream(ms, new byte[ms.Length / 2], 0, 0); uint tag = input.ReadTag(); Assert.AreEqual(1, WireFormat.GetTagFieldNumber(tag)); @@ -528,5 +508,23 @@ namespace Google.Protobuf Assert.AreEqual(WireFormat.MakeTag(1, WireFormat.WireType.StartGroup), input.ReadTag()); Assert.Throws(() => input.SkipLastField()); } + + [Test] + public void Construction_Invalid() + { + Assert.Throws(() => new CodedInputStream((byte[]) null)); + Assert.Throws(() => new CodedInputStream(null, 0, 0)); + Assert.Throws(() => new CodedInputStream((Stream) null)); + Assert.Throws(() => new CodedInputStream(new byte[10], 100, 0)); + Assert.Throws(() => new CodedInputStream(new byte[10], 5, 10)); + } + + [Test] + public void CreateWithLimits_InvalidLimits() + { + var stream = new MemoryStream(); + Assert.Throws(() => CodedInputStream.CreateWithLimits(stream, 0, 1)); + Assert.Throws(() => CodedInputStream.CreateWithLimits(stream, 1, 0)); + } } } \ No newline at end of file diff --git a/csharp/src/Google.Protobuf.Test/CodedOutputStreamTest.cs b/csharp/src/Google.Protobuf.Test/CodedOutputStreamTest.cs index c1bf7bd6..3297fe87 100644 --- a/csharp/src/Google.Protobuf.Test/CodedOutputStreamTest.cs +++ b/csharp/src/Google.Protobuf.Test/CodedOutputStreamTest.cs @@ -334,7 +334,7 @@ namespace Google.Protobuf } // Now test Input stream: { - CodedInputStream cin = new CodedInputStream(new MemoryStream(bytes), new byte[50]); + CodedInputStream cin = new CodedInputStream(new MemoryStream(bytes), new byte[50], 0, 0); Assert.AreEqual(0, cin.Position); // Field 1: uint tag = cin.ReadTag(); diff --git a/csharp/src/Google.Protobuf/CodedInputStream.cs b/csharp/src/Google.Protobuf/CodedInputStream.cs index c8b33b33..82c6ceab 100644 --- a/csharp/src/Google.Protobuf/CodedInputStream.cs +++ b/csharp/src/Google.Protobuf/CodedInputStream.cs @@ -53,16 +53,13 @@ namespace Google.Protobuf /// public sealed class CodedInputStream { - // TODO(jonskeet): Consider whether recursion and size limits shouldn't be readonly, - // set at construction time. - /// /// Buffer of data read from the stream or provided at construction time. /// private readonly byte[] buffer; /// - /// The number of valid bytes in the buffer. + /// The index of the buffer at which we need to refill from the stream (if there is one). /// private int bufferSize; @@ -106,61 +103,103 @@ namespace Google.Protobuf /// private int currentLimit = int.MaxValue; - /// - /// - /// private int recursionDepth = 0; - private int recursionLimit = DefaultRecursionLimit; + private readonly int recursionLimit; + private readonly int sizeLimit; + + #region Construction + // Note that the checks are performed such that we don't end up checking obviously-valid things + // like non-null references for arrays we've just created. /// - /// + /// Creates a new CodedInputStream reading data from the given byte array. /// - private int sizeLimit = DefaultSizeLimit; + public CodedInputStream(byte[] buffer) : this(null, Preconditions.CheckNotNull(buffer, "buffer"), 0, buffer.Length) + { + } - #region Construction /// - /// Creates a new CodedInputStream reading data from the given - /// byte array. + /// Creates a new CodedInputStream that reads from the given byte array slice. /// - public CodedInputStream(byte[] buf) : this(buf, 0, buf.Length) - { + public CodedInputStream(byte[] buffer, int offset, int length) + : this(null, Preconditions.CheckNotNull(buffer, "buffer"), offset, offset + length) + { + if (offset < 0 || offset > buffer.Length) + { + throw new ArgumentOutOfRangeException("offset", "Offset must be within the buffer"); + } + if (length < 0 || offset + length > buffer.Length) + { + throw new ArgumentOutOfRangeException("length", "Length must be non-negative and within the buffer"); + } } /// - /// Creates a new CodedInputStream that reads from the given - /// byte array slice. + /// Creates a new CodedInputStream reading data from the given stream. /// - public CodedInputStream(byte[] buffer, int offset, int length) + public CodedInputStream(Stream input) : this(input, new byte[BufferSize], 0, 0) { - this.buffer = buffer; - this.bufferPos = offset; - this.bufferSize = offset + length; - this.input = null; + Preconditions.CheckNotNull(input, "input"); } /// - /// Creates a new CodedInputStream reading data from the given stream. + /// Creates a new CodedInputStream reading data from the given + /// stream and buffer, using the default limits. /// - public CodedInputStream(Stream input) + internal CodedInputStream(Stream input, byte[] buffer, int bufferPos, int bufferSize) { - this.buffer = new byte[BufferSize]; - this.bufferSize = 0; this.input = input; + this.buffer = buffer; + this.bufferPos = bufferPos; + this.bufferSize = bufferSize; + this.sizeLimit = DefaultSizeLimit; + this.recursionLimit = DefaultRecursionLimit; } /// /// Creates a new CodedInputStream reading data from the given - /// stream, with a pre-allocated buffer. + /// stream and buffer, using the specified limits. /// - internal CodedInputStream(Stream input, byte[] buffer) + /// + /// This chains to the version with the default limits instead of vice versa to avoid + /// having to check that the default values are valid every time. + /// + internal CodedInputStream(Stream input, byte[] buffer, int bufferPos, int bufferSize, int sizeLimit, int recursionLimit) + : this(input, buffer, bufferPos, bufferSize) { - this.buffer = buffer; - this.bufferSize = 0; - this.input = input; + if (sizeLimit <= 0) + { + throw new ArgumentOutOfRangeException("sizeLimit", "Size limit must be positive"); + } + if (recursionLimit <= 0) + { + throw new ArgumentOutOfRangeException("recursionLimit!", "Recursion limit must be positive"); + } + this.sizeLimit = sizeLimit; + this.recursionLimit = recursionLimit; } #endregion + /// + /// Creates a with the specified size and recursion limits, reading + /// from an input stream. + /// + /// + /// This method exists separately from the constructor to reduce the number of constructor overloads. + /// It is likely to be used considerably less frequently than the constructors, as the default limits + /// are suitable for most use cases. + /// + /// The input stream to read from + /// The total limit of data to read from the stream. + /// The maximum recursion depth to allow while reading. + /// A CodedInputStream reading from with the specified size + /// and recursion limits. + public static CodedInputStream CreateWithLimits(Stream input, int sizeLimit, int recursionLimit) + { + return new CodedInputStream(input, new byte[BufferSize], 0, 0, sizeLimit, recursionLimit); + } + /// /// Returns the current position in the input stream, or the position in the input buffer /// @@ -182,59 +221,30 @@ namespace Google.Protobuf /// internal uint LastTag { get { return lastTag; } } - #region Limits for recursion and length /// - /// Set the maximum message recursion depth. + /// Returns the size limit for this stream. /// /// - /// In order to prevent malicious - /// messages from causing stack overflows, CodedInputStream limits - /// how deeply messages may be nested. The default limit is 64. + /// This limit is applied when reading from the underlying stream, as a sanity check. It is + /// not applied when reading from a byte array data source without an underlying stream. + /// The default value is 64MB. /// - public int SetRecursionLimit(int limit) - { - if (limit < 0) - { - throw new ArgumentOutOfRangeException("Recursion limit cannot be negative: " + limit); - } - int oldLimit = recursionLimit; - recursionLimit = limit; - return oldLimit; - } + /// + /// The size limit. + /// + public int SizeLimit { get { return sizeLimit; } } /// - /// Set the maximum message size. + /// Returns the recursion limit for this stream. This limit is applied whilst reading messages, + /// to avoid maliciously-recursive data. /// /// - /// In order to prevent malicious messages from exhausting memory or - /// causing integer overflows, CodedInputStream limits how large a message may be. - /// The default limit is 64MB. You should set this limit as small - /// as you can without harming your app's functionality. Note that - /// size limits only apply when reading from an InputStream, not - /// when constructed around a raw byte array (nor with ByteString.NewCodedInput). - /// If you want to read several messages from a single CodedInputStream, you - /// can call ResetSizeCounter() after each message to avoid hitting the - /// size limit. + /// The default limit is 64. /// - public int SetSizeLimit(int limit) - { - if (limit < 0) - { - throw new ArgumentOutOfRangeException("Size limit cannot be negative: " + limit); - } - int oldLimit = sizeLimit; - sizeLimit = limit; - return oldLimit; - } - - /// - /// Resets the current size counter to zero (see ). - /// - public void ResetSizeCounter() - { - totalBytesRetired = 0; - } - #endregion + /// + /// The recursion limit for this stream. + /// + public int RecursionLimit { get { return recursionLimit; } } #region Validation /// diff --git a/csharp/src/Google.Protobuf/Google.Protobuf.csproj b/csharp/src/Google.Protobuf/Google.Protobuf.csproj index 033edfbb..a17bf81c 100644 --- a/csharp/src/Google.Protobuf/Google.Protobuf.csproj +++ b/csharp/src/Google.Protobuf/Google.Protobuf.csproj @@ -83,6 +83,7 @@ + diff --git a/csharp/src/Google.Protobuf/IDeepCloneable.cs b/csharp/src/Google.Protobuf/IDeepCloneable.cs new file mode 100644 index 00000000..c9c71bbe --- /dev/null +++ b/csharp/src/Google.Protobuf/IDeepCloneable.cs @@ -0,0 +1,54 @@ +#region Copyright notice and license +// Protocol Buffers - Google's data interchange format +// Copyright 2015 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +#endregion + +namespace Google.Protobuf +{ + /// + /// Generic interface for a deeply cloneable type. + /// + /// + /// + /// All generated messages implement this interface, but so do some non-message types. + /// Additionally, due to the type constraint on T in , + /// it is simpler to keep this as a separate interface. + /// + /// + /// The type itself, returned by the method. + public interface IDeepCloneable + { + /// + /// Creates a deep clone of this object. + /// + /// A deep clone of this object. + T Clone(); + } +} diff --git a/csharp/src/Google.Protobuf/IMessage.cs b/csharp/src/Google.Protobuf/IMessage.cs index 773845ff..d089f946 100644 --- a/csharp/src/Google.Protobuf/IMessage.cs +++ b/csharp/src/Google.Protobuf/IMessage.cs @@ -35,8 +35,6 @@ using Google.Protobuf.Reflection; namespace Google.Protobuf { - // TODO(jonskeet): Split these interfaces into separate files when we're happy with them. - /// /// Interface for a Protocol Buffers message, supporting /// basic operations required for serialization. @@ -86,24 +84,4 @@ namespace Google.Protobuf /// The message to merge with this one. Must not be null. void MergeFrom(T message); } - - /// - /// Generic interface for a deeply cloneable type. - /// - /// - /// - /// All generated messages implement this interface, but so do some non-message types. - /// Additionally, due to the type constraint on T in , - /// it is simpler to keep this as a separate interface. - /// - /// - /// The type itself, returned by the method. - public interface IDeepCloneable - { - /// - /// Creates a deep clone of this object. - /// - /// A deep clone of this object. - T Clone(); - } } diff --git a/csharp/src/Google.Protobuf/Reflection/DescriptorPool.cs b/csharp/src/Google.Protobuf/Reflection/DescriptorPool.cs index 157ea400..759955e6 100644 --- a/csharp/src/Google.Protobuf/Reflection/DescriptorPool.cs +++ b/csharp/src/Google.Protobuf/Reflection/DescriptorPool.cs @@ -257,10 +257,12 @@ namespace Google.Protobuf.Reflection /// or unqualified. C++-like name lookup semantics are used to search for the /// matching descriptor. /// + /// + /// This isn't heavily optimized, but it's only used during cross linking anyway. + /// If it starts being used more widely, we should look at performance more carefully. + /// internal IDescriptor LookupSymbol(string name, IDescriptor relativeTo) { - // TODO(jonskeet): This could be optimized in a number of ways. - IDescriptor result; if (name.StartsWith(".")) { @@ -282,7 +284,6 @@ namespace Google.Protobuf.Reflection { // Chop off the last component of the scope. - // TODO(jonskeet): Make this more efficient. May not be worth using StringBuilder at all int dotpos = scopeToTry.ToString().LastIndexOf("."); if (dotpos == -1) { -- cgit v1.2.3