[BUG] Integer overflow in three long-to-int comparators (same class as #10579)
- Dominant language
- Java
- Stars
- 22.6k
- Forks
- 12k
- Avg merge
- 3d 1h
- Merged PRs (30d)
- 27
Description
### Before Creating the Bug Report
- [x] I found a bug, not just asking a question
- [x] I have searched the GitHub Issues and believe this is not a duplicate
- [x] I have confirmed that this bug belongs to the current repository
### Runtime platform environment
Any (logic bug, environment-independent)
### RocketMQ version
branch: develop (5.5.0)
### JDK Version
JDK 8
### Describe the Bug
PR #10579 recently fixed an integer overflow in `DefaultElectPolicy`'s comparator, where a `long` offset delta was cast to `int` via `(int) (o2.getMaxOffset() - o1.getMaxOffset())`. When the offset difference exceeds `Integer.MAX_VALUE`, the cast overflows and the comparator returns the wrong ordering.
After scanning the codebase, I found **three more comparators** with the same `long`-subtraction-cast-to-`int` overflow pattern. All three involve `long` fields (offsets / timestamps / counters) and all three are in production hot paths.
#### 1. `PopRequest.COMPARATOR` (broker, long-polling)
`broker/src/main/java/org/apache/rocketmq/broker/longpolling/PopRequest.java:95,100`
```java
public static final Comparator COMPARATOR = (o1, o2) -> {
int ret = (int) (o1.getExpired() - o2.getExpired()); // line 95: long timestamp delta
if (ret != 0) {
return ret;
}
ret = (int) (o1.op - o2.op); // line 100: long counter delta
if (ret != 0) {
return ret;
}
return -1;
};
```
- `expired` is a `long` timestamp (ms). Two requests far apart in time can overflow `int`.
- `op` is a `long` initialized from `COUNTER.getAndIncrement()` starting at `Long.MIN_VALUE` (line 29, 34). **Overflow here is essentially guaranteed** for any non-trivial number of PopRequests, since the counter walks from `Long.MIN_VALUE` upward and deltas between early and late requests easily exceed `Integer.MAX_VALUE`.
- This comparator backs a `ConcurrentSkipListSet` for pop long-polling — a broken comparator corrupts the ordered set's invariants, causing subtle request ordering/delivery bugs.
#### 2. `PopCheckPoint.compareTo` (store, pop checkpoint)
`store/src/main/java/org/apache/rocketmq/store/pop/PopCheckPoint.java:215`
```java
@Override
public int compareTo(PopCheckPoint o) {
return (int) (this.getStartOffset() - o.getStartOffset());
}
```
- `startOffset` is a `long` queue offset. A long-running broker can produce queue offsets whose delta exceeds `Integer.MAX_VALUE`. The `Comparable` contract is relied upon by sorted collections; a broken `compareTo` violates sort invariants.
#### 3. `PopReviveService.genSortList` (broker, pop revival)
`broker/src/main/java/org/apache/rocketmq/broker/processor/PopReviveService.java:728`
```java
sortList.sort((o1, o2) -> (int) (o1.getReviveOffset() - o2.getReviveOffset()));
```
- `reviveOffset` is a `long` commit-log offset. Same overflow class as #10579.
### Impact
- `PopRequest.COMPARATOR` is the most severe: the `op` counter starts at `Long.MIN_VALUE`, so the overflow is not theoretical — it triggers for real workloads, corrupting the `ConcurrentSkipListSet` that orders pop long-polling requests. A broken comparator in a `SortedSet` can cause requests to be misplaced, starved, or delivered out of order.
- `PopCheckPoint.compareTo` and `PopReviveService` sort affect pop retry/checkpoint ordering correctness under large offsets.
All three are the **exact same bug class** as the already-fixed #10579.
### Steps to Reproduce
For `PopRequest.COMPARATOR` (the guaranteed-overflow case):
```java
// op counter starts at Long.MIN_VALUE; after many requests the delta overflows int
PopRequest a = ...; // op = Long.MIN_VALUE
PopRequest b = ...; // op = Long.MIN_VALUE + (long)Integer.MAX_VALUE + 2
int cmp = PopRequest.COMPARATOR.compare(a, b);
// (int)(a.op - b.op) overflows -> wrong sign -> wrong ordering
```
### What Did You Expect to See?
Comparators over `long` fields should use `Long.compare(a, b)` (and `Integer.compare` for the `int` tiebreaker) instead of subtraction-cast-to-`int`, exactly as #10579 did for `DefaultElectPolicy`.
### What Did You See Instead?
Subtraction-cast-to-`int`, which overflows when the `long` delta exceeds `Integer.MAX_VALUE`.
### Additional Context
This is a systematic scan of the same overflow class fixed by #10579. Happy to submit a PR mirroring the #10579 fix (`Long.compare` / `Integer.compare`) if the maintainers agree these are real.
Contributor guide
Research direction
Start by reading the three comparator definitions in broker/src/main/java/org/apache/rocketmq/broker/longpolling/PopRequest.java, store/src/main/java/org/apache/rocketmq/store/pop/PopCheckPoint.java, and broker/src/main/java/org/apache/rocketmq/broker/processor/PopReviveService.java, alongside the fix in PR #10579. Validate the ordering behavior at long-value boundaries; done means all three comparisons remain correct when differences exceed Integer.MAX_VALUE.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend, distributed-systems
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100