Skip to content

fix(config): make ClientIpWhiteList.isOpen volatile and null-safe#14780

Open
daguimu wants to merge 1 commit intoalibaba:developfrom
daguimu:fix/client-ip-whitelist-volatile
Open

fix(config): make ClientIpWhiteList.isOpen volatile and null-safe#14780
daguimu wants to merge 1 commit intoalibaba:developfrom
daguimu:fix/client-ip-whitelist-volatile

Conversation

@daguimu
Copy link
Copy Markdown
Contributor

@daguimu daguimu commented Mar 29, 2026

Problem

ClientIpWhiteList.isOpen has two issues:

  1. Visibility bug: The field is written by the config-load thread (in load()) and read by HTTP request threads (in isEnableWhitelist()). Without volatile, the Java Memory Model does not guarantee cross-thread visibility, so request threads may read a stale value indefinitely.

  2. NullPointerException risk: isOpen is a Boolean wrapper, and acl.getIsOpen() can return null when the JSON field is absent. The isEnableWhitelist() method returns primitive boolean, so a null isOpen would cause auto-unboxing NPE.

Root Cause

  • private static Boolean isOpen = false; — not volatile, not thread-safe for cross-thread reads/writes.
  • isOpen = acl.getIsOpen(); — no null guard, yet downstream returns primitive boolean.

Fix

  • Change Boolean isOpen to volatile boolean isOpen for guaranteed visibility.
  • Use Boolean.TRUE.equals(acl.getIsOpen()) for null-safe assignment — treats null as false.

Tests Added

Change Point Test
isOpen field is volatile testIsOpenFieldIsVolatile() — reflection check on field modifiers
Valid JSON enables whitelist testLoadValidContentEnablesWhitelist() — verifies isOpen=true and IP matching
Blank content disables whitelist testLoadBlankContentDisablesWhitelist() — load then clear
Null isOpen does not throw NPE testLoadNullIsOpenDoesNotThrowNpe() — JSON without isOpen field
isOpen=false works correctly testLoadWithIsOpenFalse() — explicit false
Blank/null client IP validation testIsLegalClientThrowsOnBlankInput() — boundary check
Invalid JSON does not crash testLoadInvalidJsonDoesNotCrash() — malformed input

Impact

Only affects ClientIpWhiteList internal state. No API or behavioral change for correctly-formed inputs — the fix prevents stale reads and NPE on malformed ACL config.

@github-actions
Copy link
Copy Markdown

Thanks for your this PR. 🙏
Please check again for your PR changes whether contains any usage/api/configuration change such as Add new API , Add new configuration, Change default value of configuration.
If so, please add or update documents(markdown type) in docs/next/ for repository nacos-group/nacos-group.github.io


感谢您提交的PR。 🙏
请再次查看您的PR内容,确认是否包含任何使用方式/API/配置参数的变更,如:新增API新增配置参数修改默认配置等操作。
如果是,请确保在提交之前,在仓库nacos-group/nacos-group.github.io中的docs/next/目录下添加或更新文档(markdown格式)。

The isOpen field is written by the config-load thread and read by
HTTP request threads. Without volatile, changes may not be visible
across threads due to the Java Memory Model.

Additionally, acl.getIsOpen() returns a Boolean wrapper that can be
null. Assigning a null Boolean to the previous Boolean field and then
auto-unboxing it in isEnableWhitelist() (which returns primitive
boolean) would throw NullPointerException.

Change isOpen to volatile boolean and use Boolean.TRUE.equals() for
null-safe assignment.
@daguimu daguimu force-pushed the fix/client-ip-whitelist-volatile branch from 9964119 to aa0e159 Compare March 29, 2026 09:14
@KomachiSion
Copy link
Copy Markdown
Collaborator

@daguimu Hi,

we are welcome you contribute nacos community and do many PR help nacos to do enhancement and bugfix.

But I suggest you submit issue to describe issues before submit PRs.

@KomachiSion
Copy link
Copy Markdown
Collaborator

And welcome provide an dingtalk number, which we can invite you join the nacos community contributor groups.

@daguimu
Copy link
Copy Markdown
Contributor Author

daguimu commented Mar 31, 2026

And welcome provide an dingtalk number, which we can invite you join the nacos community contributor groups.
my dingtalk number is bazs8u9

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