False Negative: Guards in while loop with break fail to 'control' variable usage outside

[JLLeitschuh]

Take the following two examples:

static File safe11() throws IOException {
    File temp = null;
    if (temp == null) {
        while (true) {
            temp = File.createTempFile("test", "directory");
            if (temp.delete() && temp.mkdir()) {
                break;
            }
        }
    }
    return temp;
}

File safe12temp;
File safe12() throws IOException {
    if (safe12temp == null) {
        while (true) {
            safe12temp = File.createTempFile("test", "directory");
            if (safe12temp.delete() && safe12temp.mkdir()) {
                break;
            }
        }
    }
    return safe12temp;
}

Let's take File.createTempFile as a source of taint, and _.mkdir() is a guard.

/**
 * All methods on the class `java.io.File` that create directories.
 */
class MethodFileCreatesDirs extends Method {
  MethodFileCreatesDirs() {
    getDeclaringType() instanceof TypeFile and
    hasName(["mkdir", "mkdirs"])
  }
}

/**
 * An expression that will create a directory without throwing an exception if a file/directory already exists.
 */
private predicate isNonThrowingDirectoryCreationExpression(Expr expr, MethodAccess creationCall) {
  creationCall.getMethod() instanceof MethodFileCreatesDirs and creationCall.getQualifier() = expr
}

private class DirectoryCreationBarrierGuard extends DataFlow::BarrierGuard {
  DirectoryCreationBarrierGuard() { isNonThrowingDirectoryCreationExpression(_, this) }

  override predicate checks(Expr e, boolean branch) {
    this.controls(e, branch)
  }
}

Running a query for DirectoryCreationBarrierGuard will find the safe12temp.mkdir() and temp.mkdir().
However, when quick evaluating the DirectoryCreationBarrierGuard::checks method, it will not find that safe12temp.mkdir() is a check for return safe12temp and temp.mkdir() is a check for return safe12temp;.

Interesting note: as soon as the outer null check is removed, codeQL does correctly identify the guard controls the return. For example, this is detected as correctly guarded:

File safe12temp;
File safe12() throws IOException {
	while (true) {
        safe12temp = File.createTempFile("test", "directory");
        if (safe12temp.delete() && safe12temp.mkdir()) {
            break;
        }
    }
    return safe12temp;
}

Practical example:

https://github.com/apache/hive/blob/cba74fa91d4035aee2c39dcf929a0ae480609540/ql/src/java/org/apache/hadoop/hive/ql/exec/spark/HiveKVResultCache.java#L89-L103

The above code is flagged as a false positive result when executing this new query: #4473

[smowton]

I don't think there is a bug here -- in the safe11 case we could do better by noticing that if (temp == null) { is in context always true, but we don't go to these lengths since this pattern is very rare and will be flagged up by simple static analysis tools as a probable bug.

In the safe12 case CodeQL is correctly identifying that the return value from safe12 is not necessarily preceded by a successful mkdir() call, because it could be non-null on function entry. If this is the only possible (* except for reflection) assignment to this variable then you need more than just Guard to figure the situation out -- you need to notice that this is the only assignment to the class member, and that mkdir dominates the enclosing loop exit, and therefore reads from safe12tmp outside the loop result in either null or a checked value.

[smowton]

Closing as notabug