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.

7 files changed, 168 insertions, 126 deletions

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<InvalidProtocolBufferException>(() => TestRecursiveMessage.Parser.ParseFrom(data65));

 

-            CodedInputStream input = data64.CreateCodedInput();

-            input.SetRecursionLimit(8);

+            CodedInputStream input = CodedInputStream.CreateWithLimits(new MemoryStream(data64.ToByteArray()), 1000000, 63);

             Assert.Throws<InvalidProtocolBufferException>(() => 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<InvalidProtocolBufferException>(() => 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<InvalidProtocolBufferException>(() => input.ReadRawByte());

-

-            input.ResetSizeCounter();

-            input.ReadRawByte(); // No exception thrown.

-

-            Assert.Throws<InvalidProtocolBufferException>(() => input.ReadRawBytes(16));

+            MemoryStream ms = new MemoryStream(SampleMessages.CreateFullTestAllTypes().ToByteArray());

+            CodedInputStream input = CodedInputStream.CreateWithLimits(ms, 16, 100);

+            Assert.Throws<InvalidProtocolBufferException>(() => TestAllTypes.Parser.ParseFrom(input));

         }

 

         /// <summary>

@@ -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<InvalidProtocolBufferException>(() => input.SkipLastField());

         }

+

+        [Test]

+        public void Construction_Invalid()

+        {

+            Assert.Throws<ArgumentNullException>(() => new CodedInputStream((byte[]) null));

+            Assert.Throws<ArgumentNullException>(() => new CodedInputStream(null, 0, 0));

+            Assert.Throws<ArgumentNullException>(() => new CodedInputStream((Stream) null));

+            Assert.Throws<ArgumentOutOfRangeException>(() => new CodedInputStream(new byte[10], 100, 0));

+            Assert.Throws<ArgumentOutOfRangeException>(() => new CodedInputStream(new byte[10], 5, 10));

+        }

+

+        [Test]

+        public void CreateWithLimits_InvalidLimits()

+        {

+            var stream = new MemoryStream();

+            Assert.Throws<ArgumentOutOfRangeException>(() => CodedInputStream.CreateWithLimits(stream, 0, 1));

+            Assert.Throws<ArgumentOutOfRangeException>(() => 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
     /// </remarks>

     public sealed class CodedInputStream

     {

-        // TODO(jonskeet): Consider whether recursion and size limits shouldn't be readonly,

-        // set at construction time.

-

         /// <summary>

         /// Buffer of data read from the stream or provided at construction time.

         /// </summary>

         private readonly byte[] buffer;

 

         /// <summary>

-        /// 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).

         /// </summary>

         private int bufferSize;

 

@@ -106,62 +103,104 @@ namespace Google.Protobuf
         /// </summary> 

         private int currentLimit = int.MaxValue;

 

-        /// <summary>

-        /// <see cref="SetRecursionLimit"/>

-        /// </summary>

         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.

 

         /// <summary>

-        /// <see cref="SetSizeLimit"/>

+        /// Creates a new CodedInputStream reading data from the given byte array.

         /// </summary>

-        private int sizeLimit = DefaultSizeLimit;

+        public CodedInputStream(byte[] buffer) : this(null, Preconditions.CheckNotNull(buffer, "buffer"), 0, buffer.Length)

+        {            

+        }

 

-        #region Construction

         /// <summary>

-        /// Creates a new CodedInputStream reading data from the given

-        /// byte array.

+        /// Creates a new CodedInputStream that reads from the given byte array slice.

         /// </summary>

-        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");

+            }

         }

 

         /// <summary>

-        /// Creates a new CodedInputStream that reads from the given

-        /// byte array slice.

+        /// Creates a new CodedInputStream reading data from the given stream.

         /// </summary>

-        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");

         }

 

         /// <summary>

-        /// 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.

         /// </summary>

-        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;

         }

 

         /// <summary>

         /// Creates a new CodedInputStream reading data from the given

-        /// stream, with a pre-allocated buffer.

+        /// stream and buffer, using the specified limits.

         /// </summary>

-        internal CodedInputStream(Stream input, byte[] buffer)

+        /// <remarks>

+        /// 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.

+        /// </remarks>

+        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

 

         /// <summary>

+        /// Creates a <see cref="CodedInputStream"/> with the specified size and recursion limits, reading

+        /// from an input stream.

+        /// </summary>

+        /// <remarks>

+        /// 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.

+        /// </remarks>

+        /// <param name="input">The input stream to read from</param>

+        /// <param name="sizeLimit">The total limit of data to read from the stream.</param>

+        /// <param name="recursionLimit">The maximum recursion depth to allow while reading.</param>

+        /// <returns>A <c>CodedInputStream</c> reading from <paramref name="input"/> with the specified size

+        /// and recursion limits.</returns>

+        public static CodedInputStream CreateWithLimits(Stream input, int sizeLimit, int recursionLimit)

+        {

+            return new CodedInputStream(input, new byte[BufferSize], 0, 0, sizeLimit, recursionLimit);

+        }

+

+        /// <summary>

         /// Returns the current position in the input stream, or the position in the input buffer

         /// </summary>

         public long Position 

@@ -182,59 +221,30 @@ namespace Google.Protobuf
         /// </summary>

         internal uint LastTag { get { return lastTag; } }

 

-        #region Limits for recursion and length

         /// <summary>

-        /// Set the maximum message recursion depth.

+        /// Returns the size limit for this stream.

         /// </summary>

         /// <remarks>

-        /// 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.

         /// </remarks>

-        public int SetRecursionLimit(int limit)

-        {

-            if (limit < 0)

-            {

-                throw new ArgumentOutOfRangeException("Recursion limit cannot be negative: " + limit);

-            }

-            int oldLimit = recursionLimit;

-            recursionLimit = limit;

-            return oldLimit;

-        }

+        /// <value>

+        /// The size limit.

+        /// </value>

+        public int SizeLimit { get { return sizeLimit; } }

 

         /// <summary>

-        /// Set the maximum message size.

+        /// Returns the recursion limit for this stream. This limit is applied whilst reading messages,

+        /// to avoid maliciously-recursive data.

         /// </summary>

         /// <remarks>

-        /// 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.

         /// </remarks>

-        public int SetSizeLimit(int limit)

-        {

-            if (limit < 0)

-            {

-                throw new ArgumentOutOfRangeException("Size limit cannot be negative: " + limit);

-            }

-            int oldLimit = sizeLimit;

-            sizeLimit = limit;

-            return oldLimit;

-        }

-

-        /// <summary>

-        /// Resets the current size counter to zero (see <see cref="SetSizeLimit"/>).

-        /// </summary>

-        public void ResetSizeCounter()

-        {

-            totalBytesRetired = 0;

-        }

-        #endregion

+        /// <value>

+        /// The recursion limit for this stream.

+        /// </value>

+        public int RecursionLimit { get { return recursionLimit; } }

 

         #region Validation

         /// <summary>

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 @@
     <Compile Include="Compatibility\TypeExtensions.cs" />

     <Compile Include="FieldCodec.cs" />

     <Compile Include="FrameworkPortability.cs" />

+    <Compile Include="IDeepCloneable.cs" />

     <Compile Include="JsonFormatter.cs" />

     <Compile Include="MessageExtensions.cs" />

     <Compile Include="IMessage.cs" />

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
+{
+    /// <summary>
+    /// Generic interface for a deeply cloneable type.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// All generated messages implement this interface, but so do some non-message types.
+    /// Additionally, due to the type constraint on <c>T</c> in <see cref="IMessage{T}"/>,
+    /// it is simpler to keep this as a separate interface.
+    /// </para>
+    /// </remarks>
+    /// <typeparam name="T">The type itself, returned by the <see cref="Clone"/> method.</typeparam>
+    public interface IDeepCloneable<T>
+    {
+        /// <summary>
+        /// Creates a deep clone of this object.
+        /// </summary>
+        /// <returns>A deep clone of this object.</returns>
+        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.

-

     /// <summary>

     /// Interface for a Protocol Buffers message, supporting

     /// basic operations required for serialization.

@@ -86,24 +84,4 @@ namespace Google.Protobuf
         /// <param name="message">The message to merge with this one. Must not be null.</param>

         void MergeFrom(T message);

     }

-

-    /// <summary>

-    /// Generic interface for a deeply cloneable type.

-    /// </summary>

-    /// <remarks>

-    /// <para>

-    /// All generated messages implement this interface, but so do some non-message types.

-    /// Additionally, due to the type constraint on <c>T</c> in <see cref="IMessage{T}"/>,

-    /// it is simpler to keep this as a separate interface.

-    /// </para>

-    /// </remarks>

-    /// <typeparam name="T">The type itself, returned by the <see cref="Clone"/> method.</typeparam>

-    public interface IDeepCloneable<T>

-    {

-        /// <summary>

-        /// Creates a deep clone of this object.

-        /// </summary>

-        /// <returns>A deep clone of this object.</returns>

-        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.
         /// </summary>
+        /// <remarks>
+        /// 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.
+        /// </remarks>
         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)
                     {