False Negative: IterableIterator.ql misses `iterator() == this` implementations once the guard logic is hidden behind helpers or trivial control flow.

[Carlson-JLQ]

False Negative: IterableIterator.ql misses iterator() == this implementations once the guard logic is hidden behind helpers or trivial control flow.

Version
codeql 2.24.3

Checker

• Checker id: Language Abuse/IterableIterator.ql
• Checker description: This checker detects classes that implement Iterable by returning themselves as the Iterator but lack a guard to prevent multiple concurrent iterations.

Description of the false negative

All three samples still implement the same dangerous pattern: iterator() returns this, the object is also its own Iterator, and hasNext() does not provide a real guard against repeated or concurrent iteration. The only changes are that this is returned through a helper or ternary expression, and hasNext() is dressed up with extra control flow.

Affected test cases

PosCase4_Var2.java

The class still returns itself as the iterator and still lacks a real reuse guard. The helper only hides that.

// A class implements Iterable, declares an iterator() method that returns "this", and any declared method named "hasNext" has a body with exactly one statement, which is not a return statement returning the boolean literal false should be flagged as lacking a guard against multiple concurrent iterations.
package scensct.var.pos;

import java.util.Iterator;

public class PosCase4_Var2 implements Iterable<Object>, Iterator<Object> {
    // Keep iterator returning this
    public Iterator<Object> iterator() {
        Iterator<Object> it = this;
        return it;
    }

    // hasNext with a try-catch that doesn't affect the single return statement
    public boolean hasNext() {
        try {
            return 1 < 2;
        } catch (Exception e) {
            throw new RuntimeException(e);
        }
    }

    // next with a dummy operation
    public Object next() {
        System.gc();
        return null;
    }
}

PosCase4_Var3.java

This is the same unsafe self-iterable pattern with one extra layer of indirection.

// A class implements Iterable, declares an iterator() method that returns "this", and any declared method named "hasNext" has a body with exactly one statement, which is not a return statement returning the boolean literal false should be flagged as lacking a guard against multiple concurrent iterations.
package scensct.var.pos;

import java.util.Iterator;

public class PosCase4_Var3 implements Iterable<Object>, Iterator<Object> {
    // Inline a helper method call in iterator
    public Iterator<Object> iterator() {
        return getSelf();
    }

    private Iterator<Object> getSelf() {
        return this;
    }

    // hasNext with a single statement that computes true via method call
    public boolean hasNext() {
        return checkHasNext();
    }

    private boolean checkHasNext() {
        return true;
    }

    public Object next() {
        return "dummy";
    }
}

PosCase4_Var4.java

The control-flow refactoring does not change the fact that repeated iteration is still unsafe.

// A class implements Iterable, declares an iterator() method that returns "this", and any declared method named "hasNext" has a body with exactly one statement, which is not a return statement returning the boolean literal false should be flagged as lacking a guard against multiple concurrent iterations.
package scensct.var.pos;

import java.util.Iterator;

public class PosCase4_Var4 implements Iterable<Object>, Iterator<Object> {
    // iterator returns this via a ternary operator (trivial)
    public Iterator<Object> iterator() {
        return (System.currentTimeMillis() > 0) ? this : this;
    }

    // hasNext with a single statement that is not a simple return false
    public boolean hasNext() {
        for (int i = 0; i < 1; i++) {
            return i == 0;
        }
        return false; // This line is unreachable, but the method body still has one reachable return
    }

    public Object next() {
        return Integer.valueOf(42);
    }
}

Cause analysis

The query appears to rely on a very direct return this / return false style match. Once either method is wrapped in a helper, a ternary, or a small control-flow construct, it stops recognizing the same iterator misuse pattern.

That is narrower than it should be. These implementations are still self-iterating objects without a real reentrancy guard.

References

None known.

[redsun82]

@Carlson-JLQ thanks, noted alongside #21550. Same feedback as on #21546 — for a narrow code-style query like this, the effort is better spent on security findings or cases you've observed in real repositories.