[BUG] FieldBitLength use can cause hang and consume all memory

[knutsr]

Hi,

We ran into an issue using FieldBitLength. We have defined a class with some byte-wise fields and some bit-wise fields. Trying to deserialize an array of such class instance will fail if the input array size is not an exact multiple of the size of a single instance. The Deserialize call never returns and consumes all available memory.

Trying to deserialize by calling ser.Deserialize<BitFieldsAlone>(serialized); with this definition succeeds with any input array length:

    public class BitFieldsAlone
    {
        [FieldOrder(0)]
        [FieldBitLength(7)]
        public byte SevenBits;
        [FieldOrder(1)]
        [FieldBitLength(1)]
        public byte OneBit;
    }

Trying to deserialize by calling ser.Deserialize<BitFieldsPrecededByByte[]>(serialized); with this definition fails with input array lengths that are not multiples of 2:

    public class BitFieldsPrecededByByte
    {
        [FieldOrder(0)] 
        public byte First;
        [FieldOrder(1)]
        [FieldBitLength(7)]
        public byte SevenBits;
        [FieldOrder(2)]
        [FieldBitLength(1)]
        public byte OneBit;
    }

See attached project file and code to reproduce.

BinarySerializationMemoryIssue.csproj.txt
Program.cs.txt

[jefffhaynes]

Fixed in 8.6.2.1. Thanks!

[knutsr]

Great! Thank you.

[knutsr]

@jefffhaynes Unless I'm doing something stupid, it still fails in 8.6.2.1.

[jefffhaynes]

In the same place?

[knutsr]

I cloned the repo and added the attached project file and code to the solution. The test you added runs fine, but the test program does not.

[jefffhaynes]

Sorry, it was me doing something stupid :)

[jefffhaynes]

Try 8.6.2.2

[knutsr]

All is good now. Thanks a lot for fixing this 👍

[mathiash98]

We changed some models internally by removing some fields and encountered this issue again. I will see if I can create a minimal reproducible example. Would expect the parser to throw error instead of ending in infinite loop.

[jefffhaynes]

Ok, the only thing I'll mention is that it is easy to get into an infinite loop if you have recursive graphs. There is currently no check for that.

[mathiash98]

Finally got to produce a minimal reproducible unit test, I ran this unit test inside Issue180Tests.cs, tried stepping through the code to identify where the infinite loop occured, but I was unable to identify where it breaks.

The first Testmethod causes infinite loop which is parsing the ObservationWrapper, while the second TestMethod is OK which is not parsing on the wrapper. I'm not sure if this is related to the original issue or not

        public class ObservationWrapper
        {
            [FieldOrder(0)]
            public ObservationData[] Observations;
        }
        public class ObservationData
        {
            [FieldOrder(0)]
            [FieldCount(1)]
            public byte[] OneByte;
            
            [FieldOrder(1)]
            [FieldBitLength(7)]
            public byte SevenBits;
            
            [FieldOrder(2)]
            [FieldBitLength(1)]
            public byte OneBit;
        }

        [TestMethod]
        public void TestInfinityLoop_EndsUpInInfinityLoop()
        {
            var msg = new byte[]
            {
                1, // 1 observation
                67, // OneByteArray
                75, // 7 bits + 1 bit
            };
            var ser = new BinarySerializer();
            var message = ser.Deserialize<ObservationWrapper>(msg); // Infinite loop

            Assert.IsNotNull(message);
            Assert.AreEqual(1, message.Observations.Length);
            
            Assert.AreEqual(67, message.Observations[0].OneByte[0]);
            
            Assert.AreEqual(0, message.Observations[0].OneBit);
            Assert.AreEqual(75, message.Observations[0].SevenBits);
        }

        [TestMethod]
        public void TestInfinityLoop_DoesNotEndUpInInfinityLoop()
        {
            var msg = new byte[]
            {
                67, // OneByte
                75, // 7 bits + 1 bit
            };
            var ser = new BinarySerializer();
            var message = ser.Deserialize<ObservationData[]>(msg);

            Assert.IsNotNull(message);
            Assert.AreEqual(67, message[0].OneByte[0]);
            Assert.AreEqual(0, message[0].OneBit);
            Assert.AreEqual(75, message[0].SevenBits);
        }

I am happy to to spend more time debugging and looking into some fix, but I am a bit unsure on where to look for the issue.

[jefffhaynes]

Ok I'll take a look.

[mathiash98]

Ok I'll take a look.

I have spent some more time looking into it

Reason for infinity loop -> The provided binary message does not fit into the provided type

Observation: Inside ValueNode.cs#Deserialize we can see that stream.AvailableForReading goes below 0, during the infinity loop of serializing the binary message.

Potential fix -> Throw exception if stream.AvailableForReading < 0
See #236 where I have added the proposed exception check.

The check should probably be somewhere else?