CodeQL queries 1.25
Skip to end of metadata
Go to start of metadata

Name: Unchecked return value

Description: If most of the calls to a method use the return value of that method, the calls that do not check the return value may be mistakes.

ID: cs/unchecked-return-value

Kind: problem

Severity: warning

Precision: low

Query: UncheckedReturnValue.ql
/**
 * @name Unchecked return value
 * @description If most of the calls to a method use the return value
 *              of that method, the calls that do not check the return value may be mistakes.
 * @kind problem
 * @problem.severity warning
 * @precision low
 * @id cs/unchecked-return-value
 * @tags reliability
 *       correctness
 *       external/cwe/cwe-252
 *       statistical
 *       non-attributable
 */

import csharp
import semmle.code.csharp.frameworks.system.IO
import semmle.code.csharp.Chaining

/** Holds if `m` is a method whose return value should always be checked. */
predicate important(Method m) {
  exists(Method read | read = any(SystemIOStreamClass c).getReadMethod() |
    m = read.getAnOverrider*()
  )
  or
  exists(Method readByte | readByte = any(SystemIOStreamClass c).getReadByteMethod() |
    m = readByte.getAnOverrider*()
  )
  // add more ...
}

/** Holds if the return type of `m` is an instantiated type parameter from `m`. */
predicate methodHasGenericReturnType(ConstructedMethod cm) {
  exists(UnboundGenericMethod ugm |
    ugm = cm.getSourceDeclaration() and
    ugm.getReturnType() = ugm.getATypeParameter()
  )
}

/** Holds if `m` is a method whose return value should be checked because most calls to `m` do. */
// statistically dubious:
predicate dubious(Method m, int percentage) {
  not important(m) and
  // Suppress on Void methods
  not m.getReturnType() instanceof VoidType and
  // Suppress on methods designed for chaining
  not designedForChaining(m) and
  exists(int used, int total, Method target |
    target = m.getSourceDeclaration() and
    used =
      count(MethodCall mc |
        mc.getTarget().getSourceDeclaration() = target and
        not mc instanceof DiscardedMethodCall and
        (methodHasGenericReturnType(m) implies m.getReturnType() = mc.getTarget().getReturnType())
      ) and
    total =
      count(MethodCall mc |
        mc.getTarget().getSourceDeclaration() = target and
        (methodHasGenericReturnType(m) implies m.getReturnType() = mc.getTarget().getReturnType())
      ) and
    used != total and
    percentage = used * 100 / total and
    percentage >= 90 and
    chainedUses(m) * 100 / total <= 45 // no more than 45% of calls to this method are chained
  )
}

int chainedUses(Method m) {
  result =
    count(MethodCall mc, MethodCall qual |
      m = mc.getTarget() and
      hasQualifierAndTarget(mc, qual, qual.getTarget())
    )
}

predicate hasQualifierAndTarget(MethodCall mc, Expr qualifier, Method m) {
  qualifier = mc.getQualifier() and
  m = mc.getTarget()
}

/** Holds if `m` is a white-listed method where checking the return value is not required. */
predicate whitelist(Method m) {
  m.hasName("TryGetValue")
  or
  m.hasName("TryParse")
  or
  exists(Namespace n | n = m.getDeclaringType().getNamespace().getParentNamespace*() |
    n.getName().regexpMatch("(Fluent)?NHibernate") or
    n.getName() = "Moq"
  )
  // add more ...
}

class DiscardedMethodCall extends MethodCall {
  DiscardedMethodCall() {
    this.getParent() instanceof ExprStmt
    or
    exists(Callable c |
      this = c.getExpressionBody() and
      not c.canReturn(this)
    )
  }

  string query() {
    exists(Method m |
      m = getTarget() and
      not whitelist(m) and
      // Do not alert on "void wrapper methods", i.e., methods that are inserted
      // to deliberately ignore the returned value
      not getEnclosingCallable().getStatementBody().getNumberOfStmts() = 1
    |
      important(m) and result = "should always be checked"
      or
      exists(int percentage | dubious(m, percentage) |
        result = percentage.toString() + "% of calls to this method have their result used"
      )
    )
  }
}

from DiscardedMethodCall dmc, string message
where message = dmc.query()
select dmc, "Result of call to '" + dmc.getTarget().getName() + "' is ignored, but " + message + "."

Ignoring a method's return value can be a security risk and a common source of defects. If an attacker can force the method to fail, the subsequent program logic could lead to a vulnerability because the program will be running in an unexpected state. This rule checks that the return value of standard library methods like System.IO.Stream.Read(...) is always used. Furthermore, it identifies any calls to methods that ignore the return value despite it being frequently used elsewhere. That is, if more than 90% of the total calls to a particular method use its return value, all other calls that discard its return value are flagged as suspicious.

Recommendation

Check the return value of the method and take appropriate action.

Example

In the method IgnoreOne, there are lots of calls to DoPrint where the return value is checked. However, the return value of DoPrint("J") is not checked.

In the method IgnoreRead, the result of the call to FileStream.Read is ignored. This is dangerous, as the amount of data read might be less than the length of the array it is being read into.

using System;
using System.IO;

class Bad
{
    public bool DoPrint(string s) => true;

    public void IgnoreOne()
    {
        if (DoPrint("A"))
            Console.WriteLine("A");
        if (DoPrint("B"))
            Console.WriteLine("B");
        if (DoPrint("C"))
            Console.WriteLine("C");
        if (DoPrint("D"))
            Console.WriteLine("D");
        if (DoPrint("E"))
            Console.WriteLine("E");
        if (DoPrint("F"))
            Console.WriteLine("F");
        if (DoPrint("G"))
            Console.WriteLine("G");
        if (DoPrint("H"))
            Console.WriteLine("H");
        if (DoPrint("I"))
            Console.WriteLine("I");

        DoPrint("J");
    }

    void IgnoreRead(string path)
    {
        var file = new byte[10];
        using (var f = new FileStream(path, FileMode.Open))
            f.Read(file, 0, file.Length);
    }
}

References
  • Common Weakness Enumeration: CWE-252.