do not serve rtmp on ServerOptions.internal_port

[sahvx655-wq]

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

#3525 limited ServerOptions.internal_port to builtin and Tabbed services in baidu_std, hulu, sofa, nshead, thrift, mongo and redis, but ParseRtmpMessage still accepts RTMP there. That leaves the authentication latch described in #3525 open for rtmp: VerifyHttpRequest() returns true without a credential for a builtin service on internal_port, the connection is marked authenticated, and the IsAuthenticated() check added to ParseRtmpMessage in #3598 then passes.

With auth, internal_port and rtmp_service all set, sending GET /version followed by an RTMP C0C1 on the same connection to internal_port gets an S0S1S2 back and reaches rtmp_service, even with an authenticator that rejects every credential.

What is changed and the side effects?

Changed:

ParseRtmpMessage returns PARSE_ERROR_TRY_OTHERS on internal_port, the same way ParseRedisMessage does, since there is no Controller to fail the request with. RtmpTest.not_served_on_internal_port covers the sequence above; it fails on master (the server answers the handshake) and passes with the fix. All of brpc_rtmp_unittest passes locally.

Side effects:

• Performance effects: one port comparison when a new RTMP connection is parsed.

• Breaking backward compatibility: RTMP clients pointed at internal_port are no longer served there and should use the port passed to Server::Start(), as already documented for the other protocols.

---

Check List:

• Please make sure your changes are compilable.
• When providing us with a new feature, it is best to add related tests.
• Please follow Contributor Covenant Code of Conduct.

[Copilot]

🟡 Changes recommended

The regression test must handle partial TCP reads safely.

1 open issue

• Accumulate TCP reads before comparing the 12-byte status prefix · New

What changed in this PR

Prevents RTMP connections from being served on ServerOptions.internal_port, closing the authentication bypass.

Changes:

• Rejects RTMP parsing on the internal port.
• Adds regression coverage for authenticated HTTP-to-RTMP connection reuse.

File	Summary	Review Notes
test/​brpc_rtmp_unittest.cpp	Adds regression coverage for the internal-port restriction.	Accumulate socket reads before comparing the 12-byte response prefix to avoid flaky or invalid comparisons.
src/​brpc/​policy/​rtmp_protocol.cpp	Rejects RTMP connections on the internal port.	No issues noted.

_{Review effort: Lite}

[Copilot]

🔵 Needs a closer look

The regression test can pass on a receive timeout instead of verifying that the connection is closed.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Test treats receive timeout as success instead of requiring EOF

test/​brpc_rtmp_unittest.cpp:1357

This assertion also treats a receive timeout (read() returning -1 after the 2-second SO_RCVTIMEO) as success. A regression that leaves the connection open without sending an RTMP response would therefore pass, so the test does not verify the intended close/rejection behavior. Assert EOF (or explicitly distinguish EOF from timeout) after sending C0C1.

1 resolved since last review

• Accumulate TCP reads before comparing the 12-byte status prefix

_{Review effort: Lite}

[sahvx655-wq]

The timeout point stands. ASSERT_NE(1, read(...)) was also satisfied by read() coming back -1 on the 2s SO_RCVTIMEO, so a regression that left the connection open without answering the handshake would still have passed. Traced what the server actually does with the C0C1 on internal_port: the HTTP parser fails on the 0x03 method byte before the query stage and declines with TRY_OTHERS, rtmp now declines as well, and InputMessengerProcessor::ProcessNewMessage closes the socket with the 'Close ... due to unknown message' warning, which the client sees as EOF. The server drains the 1537 bytes in one read (MIN_ONCE_READ is 4096), so the close is a plain FIN rather than a reset.

The test now requires read() == 0 and prints n and errno on failure. Checked both directions after rebuilding: with the fix all 32 rtmp tests pass and that warning shows up in the log, without the fix the assertion fails with n=1, i.e. the S0 byte.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

5 open issues

• The test sends an HTTP/1.1 request without a Host header, which is not valid per HTTP/1.1 and can… · New
• write() is not guaranteed to write the full buffer in one call; partial writes can occur, causing… · New
• write() is not guaranteed to write the full buffer in one call; partial writes can occur, causing… · New
• The test treats any read() error as a hard failure without handling EINTR, which can create… · New
• The test treats any read() error as a hard failure without handling EINTR, which can create… · New

_{Review effort: Lite}

[sahvx655-wq]

Picked up the remaining test nits in e300720. Both writes now go through butil::WriteFileDescriptor, which already loops on short writes and retries EINTR, and the two read() calls are wrapped in HANDLE_EINTR so a stray signal can't turn into a false failure. The EOF check keeps its meaning: a timeout is still -1 with EAGAIN and still fails the assertion, only EINTR gets retried. The /version request also carries a Host header now. brpc's HTTP parser doesn't insist on one, so this wasn't failing anywhere, but there's no reason to rely on that leniency in a regression test.

None of this touches the fix in rtmp_protocol.cpp or what the test asserts, it only takes the socket plumbing out of the list of things that could flake. I couldn't rerun brpc_rtmp_unittest on the box I had to hand this time (no gflags/protobuf there), so I've only checked that the changed lines compile against the real butil headers. CI should confirm the full run.