[ISSUE #10690] Tolerate malformed SimpleChannel address ports

[Aias00]

Which Issue(s) This PR Fixes

Fixes #10690

Brief Description

SimpleChannel.parseSocketAddress already returns null for blank addresses and strings that do not match host:port, but it directly parsed the port when two segments were present. A malformed value such as 127.0.0.1:not-a-port could make remoteAddress() or localAddress() throw NumberFormatException.

This PR makes malformed port values follow the same invalid-address behavior by returning null, with a diagnostic warning. Valid addresses keep the existing behavior.

How Did You Test This Change?

mvn -pl proxy -Dtest=SimpleChannelTest -DfailIfNoTests=false test

Result: BUILD SUCCESS, Tests run: 2, Failures: 0, Errors: 0, Skipped: 0.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[RockteMQ-AI]

Review by github-manager-bot

Summary

Defensive fix: SimpleChannel.parseSocketAddress now catches NumberFormatException on malformed port values and returns null instead of propagating the exception. Includes a new unit test.

Findings

• [Info] proxy/src/main/java/org/apache/rocketmq/proxy/service/channel/SimpleChannel.java:93-96 — The log.warn message could include the exception message (e.getMessage()) to aid debugging, e.g. "parse socket address failed. address:{}, error:{}". Minor, non-blocking.

• [Info] proxy/src/test/java/org/apache/rocketmq/proxy/service/channel/SimpleChannelTest.java — Test coverage looks good. Covers valid address parsing, null/empty input, wrong segment count, and malformed port. Consider also testing with an empty port string ("127.0.0.1:") since Integer.parseInt("") also throws NumberFormatException.

Overall

Clean, focused fix with appropriate test coverage. The approach of catching NumberFormatException and returning null is consistent with the existing invalid-address handling pattern. LGTM.

---

Automated review by github-manager-bot

[codecov-commenter]

Codecov Report

❌ Patch coverage is 68.57143% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.23%. Comparing base (00e45b8) to head (34196e4).

Files with missing lines	Patch %	Lines
...che/rocketmq/proxy/service/relay/ProxyChannel.java	58.82%	6 Missing and 1 partial ⚠️
...q/proxy/remoting/pipeline/ContextInitPipeline.java	33.33%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10691      +/-   ##
=============================================
- Coverage      48.32%   48.23%   -0.09%     
+ Complexity     13512    13488      -24     
=============================================
  Files           1380     1380              
  Lines         101091   101115      +24     
  Branches       13101    13102       +1     
=============================================
- Hits           48851    48774      -77     
- Misses         46279    46349      +70     
- Partials        5961     5992      +31     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[fuyou001]

[P1] The malformed-port handling is incomplete for out-of-range values.

The new catch handles NumberFormatException, but values such as 127.0.0.1:-1 and 127.0.0.1:65536 parse successfully as integers and then cause the InetSocketAddress constructor to throw IllegalArgumentException. Consequently, remoteAddress() and localAddress() can still throw for malformed ports instead of returning null as described by this PR.

Please validate the parsed port explicitly as 0..65535 before constructing the address (preferable for clear semantics), or otherwise handle the constructor exception as invalid input.

Please add assertions for -1, 65536, a value larger than Integer.MAX_VALUE, an empty port, and whitespace, covering both remoteAddress() and localAddress(). It would also be useful to exercise the callers in LocalProxyRelayService, ChannelManager, ContextInitPipeline, and RemotingChannel so that a null address and an asynchronously completed relay callback are consumed safely.

[RockteMQ-AI]

Review by github-manager-bot (Re-review)

PR #10691: [ISSUE #10690] Tolerate malformed SimpleChannel address ports
Re-review trigger: New maintainer feedback from @fuyou001 (2026-07-30)

Maintainer Feedback Summary

@fuyou001 raised a P1 correctness concern: the catch (NumberFormatException) only handles non-numeric port strings, but values like 127.0.0.1:-1 and 127.0.0.1:65536 parse successfully as integers. The InetSocketAddress constructor then throws IllegalArgumentException for out-of-range ports, which is not caught — so remoteAddress() and localAddress() can still throw for malformed input.

Assessment

The concern is valid. The current fix is incomplete:

try {
    return new InetSocketAddress(segments[0], Integer.parseInt(segments[1]));
} catch (NumberFormatException e) {
    log.warn("parse socket address failed. address:{}", address);
    return null;
}

Integer.parseInt("-1") succeeds → InetSocketAddress("127.0.0.1", -1) throws IllegalArgumentException → uncaught → propagates up.

Similarly, Integer.parseInt("65536") succeeds but InetSocketAddress rejects it.

Suggested Fix Direction

Two options (prefer option 1 for clearer semantics):

1. Validate port range explicitly before constructing the address:

int port = Integer.parseInt(segments[1]);
if (port < 0 || port > 65535) {
    log.warn("port out of range: {}", address);
    return null;
}
return new InetSocketAddress(segments[0], port);

2. Also catch IllegalArgumentException:

} catch (NumberFormatException | IllegalArgumentException e) {
    log.warn("parse socket address failed. address:{}", address);
    return null;
}

Suggested Test Cases

Add assertions for: -1, 65536, Integer.MAX_VALUE + 1 (overflow), empty port, whitespace port — covering both remoteAddress() and localAddress().

Status

No new commits since the maintainer feedback. Awaiting author response.

---

Automated review by github-manager-bot

[Aias00]

Addressed the malformed-port caller coverage in 34196e4d3.

Changes:

• ProxyChannel / RemotingChannel now consume null or unparseable socket addresses without throwing.
• ContextInitPipeline now tolerates null/unparseable local addresses.
• Added regression coverage for RemotingChannel and ContextInitPipeline, in addition to the existing SimpleChannel, ChannelManager, and LocalProxyRelayService coverage.

Local verification passed:
mvn -pl proxy -Dtest=SimpleChannelTest,LocalProxyRelayServiceTest,RemotingChannelTest,ContextInitPipelineTest -DfailIfNoTests=false test

[RockteMQ-AI]

Review by github-manager-bot (Re-review after new commits)

PR #10691: [ISSUE #10690] Tolerate malformed SimpleChannel address ports
Re-review trigger: New commits pushed on Jul 31 (after previous review on Jul 31 00:37)

Previous P1 Concern Resolution

The previous review raised a P1 correctness concern: out-of-range ports like -1 or 65536 could parse as integers but throw IllegalArgumentException from InetSocketAddress, which was not caught.

✅ Fully resolved. The new commits add defense in depth:

1. SimpleChannel.parseSocketAddress — validates port range (0-65535) before constructing InetSocketAddress. Invalid ports return null with a warning log.
2. ProxyChannel.socketAddress2String — wraps socketAddress2String with try-catch for RuntimeException, returning the raw string on failure.
3. ContextInitPipeline.socketAddress2String — same defensive wrapper for the pipeline path.
4. RemotingChannel — switched from NetworkUtil.socketAddress2String to ProxyChannel.socketAddress2String for consistent behavior.

Code Quality

• Port range validation: port < 0 || port > 65535 check is correct ✓
• Defensive layers: Multiple catch points ensure no uncaught IllegalArgumentException propagates ✓
• Logging: Warning logs for malformed addresses aid diagnostics ✓
• Tests: Comprehensive coverage including 127.0.0.1:-1, 127.0.0.1:65536, 127.0.0.1:abc, and blank addresses ✓

Minor Observation

There is code duplication in socketAddress2String — defined in both ProxyChannel and ContextInitPipeline. Consider extracting to a shared utility (e.g., NetworkUtil) in a follow-up. This is a style concern, not a blocker.

Verdict

Approve. The P1 correctness concern is fully addressed with proper port range validation and defensive exception handling.

[RockteMQ-AI]

Summary

Defensive fix with proper validation and test coverage. LGTM.

---

Automated review by github-manager-bot