search

taohyson / protobuf-net

Automatically exported from code.google.com/p/protobuf-net
Other
0 stars 0 forks source link

protoreader position overflow #415

Open GoogleCodeExporter opened 8 years ago

GoogleCodeExporter commented 8 years ago 
summary: I think this problem is now hidden by setting the position to 0 more 
often than before, but as long as position is int while the streams are using 
long, I suspect this bug could trigger again with larger file while reading 
decimals as the code (atleast in old build) is reader.blockEnd = 
reader.position + len; in StartSubItem(..). Before the exception was thrown, 
atleast 3 decimals were read without exception but with incorrect values, 
though this was still while looping over the fields, so I don't know if there 
really is a scenario where it could have ended up returning incorrect data 
without exceptions.

fix: I would probably use ulong instead of long or int, just incase someone 
calls this over never ending network stream. Infact I think the code should be 
reworked to work over infinite size streams at some point.

I'm using a bit older build so I run into this problem sooner that I should 
have. In the old build the code looked like:
reader.available = reader.depth = reader.fieldNumber = reader.ioIndex = 0;
while in the newer build it's:
reader.position = reader.available = reader.depth = reader.fieldNumber = 
reader.ioIndex = 0;

So since my files are typically less than ~2GB I wouldn't have found this issue.

What I'm doing is simply calling
ProtoBuf.Serializer.NonGeneric.TryDeserializeWithLengthPrefix(mystream, 
PrefixStyle.Base128, resolver , out obj);
where mystream changes to point to different files. After reading few GB's of 
data with this, I got a mysterious exception that didn't really say anything 
about overflow but more suggesting a corruption in the data.

at

   at ProtoBuf.ProtoReader.EndSubItem(SubItemToken token, ProtoReader reader) in protobuf-net\ProtoReader.cs:line 616
   at ProtoBuf.BclHelpers.ReadDecimal(ProtoReader reader) in protobuf-net\BclHelpers.cs:line 254
   at ProtoBuf.Serializers.DecimalSerializer.Read(Object value, ProtoReader source) in protobuf-net\Serializers\DecimalSerializer.cs:line 37
   at ProtoBuf.Serializers.TagDecorator.Read(Object value, ProtoReader source) in protobuf-net\Serializers\TagDecorator.cs:line 80
   at ProtoBuf.Serializers.DefaultValueDecorator.Read(Object value, ProtoReader source) in protobuf-net\Serializers\DefaultValueDecorator.cs:line 45
   at ProtoBuf.Serializers.FieldDecorator.Read(Object value, ProtoReader source) in protobuf-net\Serializers\FieldDecorator.cs:line 42
   at ProtoBuf.Serializers.TypeSerializer.Read(Object value, ProtoReader source) in protobuf-net\Serializers\TypeSerializer.cs:line 227
   at ProtoBuf.Meta.RuntimeTypeModel.Deserialize(Int32 key, Object value, ProtoReader source) in protobuf-net\Meta\RuntimeTypeModel.cs:line 775
   at ProtoBuf.Meta.TypeModel.DeserializeWithLengthPrefix(Stream source, Object value, Type type, PrefixStyle style, Int32 expectedField, TypeResolver resolver, Int32& bytesRead, Boolean& haveObject, SerializationContext context) in protobuf-net\Meta\TypeModel.cs:line 353
   at ProtoBuf.Meta.TypeModel.DeserializeWithLengthPrefix(Stream source, Object value, Type type, PrefixStyle style, Int32 expectedField, TypeResolver resolver, Int32& bytesRead) in protobuf-net\Meta\TypeModel.cs:line 296
   at ProtoBuf.Meta.TypeModel.DeserializeWithLengthPrefix(Stream source, Object value, Type type, PrefixStyle style, Int32 expectedField, TypeResolver resolver) in protobuf-net\Meta\TypeModel.cs:line 276
   at ProtoBuf.Serializer.NonGeneric.TryDeserializeWithLengthPrefix(Stream source, PrefixStyle style, TypeResolver resolver, Object& value) in protobuf-net\Serializer.cs:line 476

Original issue reported on code.google.com by virtan...@gmail.com on 17 Nov 2013 at 1:39

GoogleCodeExporter commented 8 years ago 
When I posted the bug I didn't notice the 

reader.position = reader.available = reader.depth = reader.fieldNumber = 
reader.ioIndex = 0;

is called on every TryDeserializeWithLengthPrefix call, which is good enough 
for me. 

I suppose there might be some weird edge case where there could still be 
overflow, so personally I wouldn't convert longs to ints in my own code but I 
suppose you know whether the overflow can happen in any supported scenario.

Original comment by virtan...@gmail.com on 8 Dec 2013 at 2:02