From f2732c7af13110a72ded2667684f52a86a23de2f Mon Sep 17 00:00:00 2001
From: Jon Skeet <jonskeet@google.com>
Date: Mon, 10 Aug 2015 18:32:18 +0100
Subject: [PATCH] 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.
---
.../CodedInputStreamTest.cs | 48 +++---
.../CodedOutputStreamTest.cs | 2 +-
.../src/Google.Protobuf/CodedInputStream.cs | 160 ++++++++++--------
.../Google.Protobuf/Google.Protobuf.csproj | 1 +
csharp/src/Google.Protobuf/IDeepCloneable.cs | 54 ++++++
csharp/src/Google.Protobuf/IMessage.cs | 22 ---
.../Reflection/DescriptorPool.cs | 7 +-
7 files changed, 168 insertions(+), 126 deletions(-)
create mode 100644 csharp/src/Google.Protobuf/IDeepCloneable.cs
diff --git a/csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs b/csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs
index c8326d801b..54c44e4776 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 c1bf7bd638..3297fe8768 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 c8b33b33ba..82c6ceab92 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,61 +103,103 @@ 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>
@@ -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 033edfbb7e..a17bf81c82 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 0000000000..c9c71bbe2c
--- /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 773845ff87..d089f94639 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 157ea40088..759955e635 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)
{