Pass number of bytes to Buffer.BlockCopy

[terrajobst]

The count argument is in bytes, but it's easy to accidentally do something like Buffer.BlockCopy(src, 0, dst, 0, src.Length), and if src is an array of elements larger than bytes, this is a bug.

Category: Reliability

[jeffhandley]

Estimates:

• Analyzer: Small
• Fixer: Not Applicable

[jeffhandley]

If the first argument is an array of a primitive other than byte or sbyte, and the last argument is .Length for that array, then we should match and present the warning.

Anything other than a simple .Length would not match. Any array of a non-primitive would not match.

We can consider an expansion of the .Length value as an argument, identifying places where division is not used, we could match--but we would not match when division is used.

[carlossanlop]

Usage example:

using System;

namespace MyNamespace
{
    class Sample
    {
        public static void Main()
        {
            int[] src = ...;
            int[] dst = ...;

            // Warn
            Buffer.BlockCopy(src, 0, dst, 0, src.Length);

            // Warn
            Buffer.BlockCopy(src, 0, dst, 0, dst.Length);

            // Warn
            int numBytes2 = src.Length;
            Buffer.BlockCopy(src, 0, dst, 0, numBytes2);

            // Warn
            int numBytes3 = dst.Length;
            Buffer.BlockCopy(src, 0, dst, 0, numBytes3);

            // Do not warn
            Buffer.BlockCopy(src, 0, dst, 0, 5);

            // Do not warn
            int numBytes = ...;
            Buffer.BlockCopy(src, 0, dst, 0, numBytes);

            // Do not warn
            Buffer.BlockCopy(src, 0, dst, 0, GetNumBytes());


           // Allowed array data types
            
            byte[] srcByte = ...;
            byte[] dstByte = ...;

            // Do not warn if underlying array type is byte
            Buffer.BlockCopy(srcByte, 0, dstByte, 0, srcByte.Length);

            // Do not warn if underlying array type is byte
            Buffer.BlockCopy(srcByte, 0, dstByte, 0, dstByte.Length);

            sbyte[] srcSbyte = ...;
            sbyte[] dstSbyte = ...;
            // Do not warn if underlying array type is sbyte
            Buffer.BlockCopy(srcSbyte, 0, dstSbyte, 0, srcSbyte.Length);

            // Do not warn if underlying array type is sbyte
            Buffer.BlockCopy(srcSbyte, 0, dstSbyte, 0, dstSbyte.Length);
        }

        static int GetNumBytes()
        {
            return ...;
        }
    }
}

• Suggested severity: warning.
• No code fixer expected for this.

Question

@jeffhandley 's last suggestion to detect if a division is used would not warn anyway because the passed argument would not exactly be src.Length or dst.Length, but an expression. Would we be interested in detecting if the expression is src.Length/sizeof(int) or dst.Length/sizeof(int) or a division that would make sense to determine the size in bytes, depending on the data type of the array?

[terrajobst]

Video

• On by default as warning seems reasonable, given the scoped nature
• Adding a fixer that offers to multiply the length or call Array.Copy instead

[carlossanlop]

Thanks. The Buffer.BlockCopy documentation remarks should also be expanded to also describe the suggestions that this analyzer will provide: https://docs.microsoft.com/en-us/dotnet/api/system.buffer.blockcopy?view=net-5.0#remarks

[mahdiva]

Just bringing up a point for clarification. If we had the following code, I don't think the analyzer should be triggered

using System;

class Program
{
    static void Main()
    {
        int[] src = new int[] {1, 2, 3, 4};
        int[] dst = new int[] {0, 0, 0, 0};
        int numOfBytes = src.Length;
        
        SomeFunction(src, dst, numOfBytes);
    }

    static void SomeFunction(int[] src, int[] dst, int count)
    {
        Buffer.BlockCopy(src, 0, dst, 0, count); // I don't think it's possbile to know where count is coming from. Looking just at this expression, `count`'s variable declaration will be the parameter in the function definition. I think we should NOT report a diagnostic in this case
    }
}

[jeffhandley]

Closed via dotnet/roslyn-analyzers#5242