Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sanek2k6 opened this issue Oct 19, 2020 · 0 comments

Comments

@sanek2k6
Copy link

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");
    }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant