jefffhaynes / BinarySerializer

A declarative serialization framework for controlling formatting of data at the byte and bit level using field bindings, converters, and code.
MIT License
289 stars 62 forks source link

When ItemSubtypeDefault is not specified, deserializing "bad" bytes results in a deadlock #160

Open sanek2k6 opened 3 years ago

sanek2k6 commented 3 years ago

Hello!

Feeding garbage bytes to a deserialize operation of an object containing ItemSubType entries seems to "hang" the deserialize operation when ItemSubtypeDefault attribute is not specified. In reality, it appears that it iterates long.MaxValue times in CollectionValueNode.cs:70, taking a really long time.

Repro:

public interface ITest
{

}

public class TestA : ITest
{
    [FieldOrder(0)]
    [SerializeAs(SerializedType.UInt1)]
    public byte Value1 { get; set; }

    [FieldOrder(1)]
    [SerializeAs(SerializedType.UInt1)]
    public byte Value2 { get; set; }
}

public class TestB : ITest
{
    [FieldOrder(0)]
    [SerializeAs(SerializedType.UInt2)]
    public ushort Value { get; set; }
}

public class ItemSubtypeClass
{
    [FieldOrder(0)]
    [SerializeAs(SerializedType.UInt1)]
    public byte Indicator { get; set; }

    [FieldOrder(1)]
    [ItemSubtype(nameof(Indicator), 1, typeof(TestA))]
    [ItemSubtype(nameof(Indicator), 2, typeof(TestB))]
    public List<ITest> Items { get; set; }
}

static void Main(string[] args)
{
    var serializer = new BinarySerializer();
    var testBytes = Enumerable.Repeat((byte)0xFF, 5).ToArray();

    // This will "hang"
    var testObject = serializer.Deserialize<ItemSubtypeClass>(testBytes);
}

Is the expected behavior here that the ItemSubtypeDefault should always be specified or is this a bug? That behavior would seem odd if its not a bug as looping long.MaxValue times is basically effectively a deadlock.

I would prefer that it would fail as soon as it gets an unexpected value rather than go through the default mappings for each item. So in the example above, when deserializing the Items property, it should see that there is no mapping for Indicator == 255, so it should just fail immediately.

Currently, working around this issue by having a default item subtype that just throws an exception - something like this:

public class DefaultTest : ITest, IBinarySerializable
{
    public void Serialize(Stream stream, Endianness endianness, BinarySerializationContext serializationContext)
    {
        throw new InvalidOperationException("Bad Item");
    }

    public void Deserialize(Stream stream, Endianness endianness, BinarySerializationContext serializationContext)
    {
        throw new InvalidOperationException("Bad Item");
    }
}