apache / apache/rocketmq

[BUG] Integer overflow in three long-to-int comparators (same class as #10579)

Open
#10,674 1 comment 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.