Off-by-one comparison against container length¶
ID: cs/index-out-of-boundsKind: problemSecurity severity: Severity: errorPrecision: highTags: - quality - reliability - correctness - external/cwe/cwe-193Query suites: - csharp-security-and-quality.qls
Click to see the query in the CodeQL repository
An indexing operation against a collection, string or array should use an index at most one less than the length. If the index to be accessed is compared to be less than or equal to the length (<=), instead of less than the length (<), the index could be out of bounds.
Recommendation¶
Use less than (<) rather than less than or equals (<=) when comparing a potential index against a length. For loops that iterate over every element in an array or collection, prefer to use theforeach syntax instead of looping over explicit indexes.
Example¶
The following example shows two methods which identify whether a value appears in a comma-separated list of values.
In the first example, a loop using an index variable -i - is used to iterate over the elements in the comma-separated list. However, the terminating condition of the loop is incorrectly specified asi<=values.Length. This condition will succeed ifi is equal tovalues.Length, but the accessvalues[i] in the body of the loop will fail because the index is out of bounds.
One potential solution would be to replacei<=values.Length withi<values.Length. However, arrays in C# can also be used inforeach loops. This is shown in the second example. This circumvents the need to specify an index at all, and therefore prevents errors where the index is out of bounds.
usingSystem.IO;usingSystem;classContainerLengthOffByOne{publicstaticbooleanBadContains(stringsearchName,stringnames){string[]values=names.Split(',');// BAD: index could be equal to lengthfor(inti=0;i<=values.Length;i++){// When i = length, this access will be out of boundsif(values[i]==searchName){returntrue;}}}publicstaticbooleanGoodContains(stringsearchName,stringnames){string[]values=names.Split(',');// GOOD: Avoid using indexes, and use foreach insteadforeach(stringnameinvalues){if(name==searchName){returntrue;}}}}
References¶
Common Weakness Enumeration:CWE-193.