github / github/codeql

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

Open
#21,551 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
CodeQL
Stars
10.1k
Forks
2.1k
Avg merge
2d 15h
Merged PRs (30d)
141

Description

# 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.

```java
// 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, Iterator {
// Keep iterator returning this
public Iterator iterator() {
Iterator 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.

```java
// 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, Iterator {
// Inline a helper method call in iterator
public Iterator iterator() {
return getSelf();
}

private Iterator 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.

```java
// 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, Iterator {
// iterator returns this via a ternary operator (trivial)
public Iterator 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.

Contributor guide

Open the contributing guide

Research direction

Start with the Language Abuse/IterableIterator.ql checker and compare its matching logic with PosCase4_Var2.java, PosCase4_Var3.java, and PosCase4_Var4.java. Update the checker and regression coverage so all three self-iterating implementations are flagged despite helper calls or trivial control flow, while preserving the intended guard behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
devtools, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.