Skip to content

[Bug] Fix TOCTOU race condition in MQClientInstance.brokerVersionTable access#10235

Open
Senrian wants to merge 1 commit intoapache:developfrom
Senrian:fix-toctou-race-broker-version
Open

[Bug] Fix TOCTOU race condition in MQClientInstance.brokerVersionTable access#10235
Senrian wants to merge 1 commit intoapache:developfrom
Senrian:fix-toctou-race-broker-version

Conversation

@Senrian
Copy link
Copy Markdown

@Senrian Senrian commented Mar 30, 2026

Bug Description

Issue: #10214

In MQClientInstance.java, brokerVersionTable (a ConcurrentHashMap) is accessed using a non-atomic check-then-act pattern that can cause NullPointerException under concurrent access.

Locations Fixed

  1. sendHeartbeatToBroker() - Race between containsKey() and get()
  2. sendHeartbeatToBrokerV2() - Same issue
  3. findBrokerVersion() - Triple get() calls with potential NPE

Fix Pattern

Use computeIfAbsent for atomic get-or-create:

// Before (race condition):
if (!this.brokerVersionTable.containsKey(brokerName)) {
    this.brokerVersionTable.put(brokerName, new ConcurrentHashMap<>(4));
}
this.brokerVersionTable.get(brokerName).put(addr, version);

// After (thread-safe):
this.brokerVersionTable.computeIfAbsent(brokerName, k -> new ConcurrentHashMap<>(4))
        .put(addr, version);

For findBrokerVersion(), use a local variable to avoid NPE if entry is removed between calls.

Issue: apache#10214

Replace non-atomic check-then-act pattern with thread-safe operations:
- sendHeartbeatToBroker(): use computeIfAbsent instead of containsKey+put
- sendHeartbeatToBrokerV2(): same fix
- findBrokerVersion(): use local variable to avoid NPE if entry removed between calls
@Kris20030907
Copy link
Copy Markdown
Contributor

@Senrian This change is not recommended as it may cause performance issues. Please refer to the documentation:https://juejin.cn/post/7094561581631012878

@Senrian
Copy link
Copy Markdown
Author

Senrian commented Apr 6, 2026

Hi @Kris20030907, thank you for the review. Could you clarify which specific change you are concerned about? The computeIfAbsent pattern used here is a standard ConcurrentHashMap atomic operation and is actually the recommended approach for this type of race condition (check-then-act on a map entry). It avoids the performance overhead of synchronized blocks while ensuring thread-safety. Would you prefer a different approach, such as using synchronized blocks or accepting the race condition?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants