micjahn/ZXing.Net

BitArray.xor() should check length of both arrays

Closed this issue · 3 comments

Annotated code from Source/lib/common/BitArray.cs, lines 346-362:

        /// <summary>
        /// XOR operation
        /// </summary>
        /// <param name="other"></param>
        public void xor(BitArray other)
        {
            if (size != other.size)
            {
                throw new ArgumentException("Sizes don't match");
            }
            for (int i = 0; i < bits.Length; i++) // Anlo2846: Checking only one array length, other.bits might be shorter
            {
                // The last int could be incomplete (i.e. not have 32 bits in
                // it) but there is no problem since 0 XOR 0 == 0.
                bits[i] ^= other.bits[i];
            }
        }

Code to reproduce bug:

using ZXing.Common;
BitArray a = new BitArray(128); // a.bits.Length == 4
a.appendBit(true);              // a.bits.Length == 6
BitArray b = new BitArray(129); // b.bits.Length == 5
b[128] = true;
a.xor(b);                       // System.IndexOutOfRangeException: 'Index was outside the bounds of the array.'

Suggested fix in Source/lib/common/BitArray.cs, line 356:

for (int i = 0; i < bits.Length && i < other.bits.Length; i++)

A really interesting error, thanks for reporting it.
What do you think about the following fix:

            var numberOfBits = bits.Length;
            if (other.bits.Length < numberOfBits)
                numberOfBits = other.bits.Length;

            for (int i = 0; i < numberOfBits; i++)
            {
                // The last int could be incomplete (i.e. not have 32 bits in
                // it) but there is no problem since 0 XOR 0 == 0.
                bits[i] ^= other.bits[i];
            }

Yeah, that will work. But I would name the variable numberOfInts (or similar) instead since it is not really the number of bits.

You are right. I named it that way because of the name of the array. But numberOfBits should be clearer.