No Calculation Overflow

ID

csharp.no_calculation_overflow

Severity

high

Remediation Complexity

trivial

Remediation Risk

low

Remediation Effort

low

Resource

Reliability

Language

CSharp

Tags

arithmetic-overflow, correctness, numeric-types, reliability

Description

Reports integer arithmetic whose result provably does not fit the type the operation is carried out in: a 32-bit multiplication that is only widened to long, ulong, double or decimal afterwards, and arithmetic whose operands all hold known values whose exact result leaves the range of the operation’s own type.

Rationale

C# evaluates +, - and * in the type the operands promote to, and everything narrower than int is promoted to int first. Outside a checked context the bits that do not fit are discarded and execution continues, so the program carries on with a number that is not the one the expression describes. Nothing is logged and nothing is thrown; the wrong value simply travels onwards, and the report it eventually lands in looks plausible.

The most common form is a product assigned to something wide. Writing long ms = days * 86400000 reads as if the destination has room for the answer, and it does — but the multiplication has already been performed in 32 bits by the time the widening happens, and the widening cannot restore what was discarded. Just over 24 days of milliseconds is enough to exceed int. The fix is to widen an operand, not the destination.

The second form needs no reasoning about ranges at all: every operand has a single known value, so the exact result can be computed and compared against the bounds of the type the arithmetic is performed in. This is not the set the compiler already rejects. An overflowing const expression is a compile error, but the same arithmetic over ordinary locals compiles cleanly and wraps at run time.

Only +, - and * are reported. Division and remainder do not wrap this way, and the rule does not guess: an operand whose type does not resolve, or that is not integral, means no finding.

Arithmetic inside an unchecked block is left alone. There the wraparound has been declared to be the intent, which is exactly the statement the rule is looking for. A nested checked block restores reporting, since the innermost context is the one that governs. A GetHashCode() override is left alone for the same reason: combining hash contributions relies on wraparound.

public class Timings
{
    public long DayMillis(int days)
    {
        long total = days * 86400000;          // FLAW, the product is computed in 32 bits
        return total;
    }

    public int Capacity()
    {
        int gigabytes = 4;
        int bytesPerGigabyte = 1024 * 1024 * 1024;
        return gigabytes * bytesPerGigabyte;   // FLAW, exactly 4294967296, outside int
    }

    public long OkWidenedFirst(int days)
    {
        long total = (long) days * 86400000;   // OK, one operand is widened before the multiply
        return total;
    }

    public long OkDeliberate(int a, int b)
    {
        unchecked
        {
            return a * b;                      // OK, wraparound declared to be the intent
        }
    }
}

Remediation

Decide which type the arithmetic should be performed in, and make the operands say so.

To keep a wide result, widen an operand rather than the destination. One cast is enough, because the other operand is then promoted to match:

long total = (long) days * 86400000;

Where the operands cannot be widened — a value that genuinely has to stay 32 bits — bound the inputs before multiplying, or perform the arithmetic in a wider type and validate the result before narrowing it back.

Where overflow means the input was invalid, wrap the arithmetic in checked so it raises OverflowException instead of continuing with a wrong value, and handle that where the invalid input can be reported.

Where wraparound is genuinely intended — a hash, a checksum, a rolling counter — say so with unchecked. That documents the decision for the next reader and stops the rule reporting it.

References