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. --- 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 +- 5 files changed, 144 insertions(+), 100 deletions(-) create mode 100644 csharp/src/Google.Protobuf/IDeepCloneable.cs (limited to 'csharp/src/Google.Protobuf') 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