IndexOf Check Wrong Bound

ID

csharp.indexof_check_wrong_bound

Severity

high

Remediation Complexity

trivial

Remediation Risk

low

Remediation Effort

low

Resource

Reliability

Language

CSharp

Tags

off-by-one, reliability, suspicious-comparison

Description

Reports an IndexOf, IndexOfAny, LastIndexOf or LastIndexOfAny result tested against > 0, in either operand order. Position 0 is a valid match, so the test silently misses an occurrence at the start.

Rationale

All four searches return the zero-based position of the match, and -1 when there is none. > 0 therefore rejects both "not found" and "found at the very beginning" — and the beginning is precisely the input that tends to matter: the leading separator of a path, the flag at the head of a list, the quote that opens a token. The code reads as a containment test and behaves as one for every input except the interesting one, which is why the defect usually reaches production.

The dependable spellings are >= 0 (or != -1) for "found" and < 0 (or == -1) for "not found". A bound at another position, such as > 1, states a deliberate "after the first character" intent and is not reported; neither is the correct > -1.

public bool ContainsSeparator(string path)
{
    return path.IndexOf('/') > 0;          // FLAW — "/etc" answers false
}

public bool StartsWithVowel(string word, char[] vowels)
{
    return 0 < word.IndexOfAny(vowels);    // FLAW — the mirror image of the same test
}

public bool ContainsSeparatorOk(string path)
{
    return path.IndexOf('/') >= 0;         // OK — position 0 counts as found
}

public bool MissingOk(string path)
{
    return path.IndexOf('/') == -1;        // OK — the canonical not-found test
}

public bool AfterFirstOk(string path)
{
    return path.IndexOf('/') > 1;          // OK — a deliberate position bound
}

Remediation

Replace > 0 with >= 0 when the question is whether the value was found, or with < 0 / == -1 when the question is whether it was absent. For a plain containment test on a string, Contains states the intent directly and cannot be got wrong; Enumerable.Contains does the same for sequences. Where the position itself is used afterwards, keep the index but test it with >= 0 before indexing.

References