You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "sahvx655-wq (via GitHub)" <gi...@apache.org> on 2026/10/18 12:49:05 UTC
[PR] do not serve rtmp on ServerOptions.internal_port (brpc)
sahvx655-wq opened a new pull request, #3622:
URL: https://github.com/apache/brpc/pull/3622
### 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](https://github.com/apache/brpc/blob/master/CODE_OF_CONDUCT.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] do not serve rtmp on ServerOptions.internal_port (brpc)
Posted by "sahvx655-wq (via GitHub)" <gi...@apache.org>.
sahvx655-wq commented on PR #3622:
URL: https://github.com/apache/brpc/pull/3622#issuecomment-6262979230
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] do not serve rtmp on ServerOptions.internal_port (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3622:
URL: https://github.com/apache/brpc/pull/3622#discussion_r4307361206
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -1286,6 +1286,71 @@ TEST(RtmpTest, reject_unauthenticated_connection) {
server.Join();
}
+class RejectAllAuthenticator : public brpc::Authenticator {
+public:
+ int GenerateCredential(std::string*) const override { return 0; }
+ int VerifyCredential(const std::string&, const butil::EndPoint&,
+ brpc::AuthContext*) const override { return -1; }
+};
+
+// Returns a port nothing is listening on, or -1.
+static int PickUnusedPort() {
+ butil::fd_guard sockfd(butil::tcp_listen(butil::EndPoint(butil::IP_ANY, 0)));
+ if (sockfd < 0) {
+ return -1;
+ }
+ butil::EndPoint point;
+ if (butil::get_local_side(sockfd, &point) != 0) {
+ return -1;
+ }
+ return point.port;
+}
+
+TEST(RtmpTest, not_served_on_internal_port) {
+ PlayingDummyService rtmp_service;
+ RejectAllAuthenticator auth;
+ brpc::Server server;
+ brpc::ServerOptions server_opt;
+ server_opt.rtmp_service = &rtmp_service;
+ server_opt.auth = &auth;
+ bool started = false;
+ for (int i = 0; i < 10 && !started; ++i) {
+ server_opt.internal_port = PickUnusedPort();
+ started = (server_opt.internal_port >= 0 &&
+ 0 == server.Start("127.0.0.1:0", &server_opt));
+ }
+ ASSERT_TRUE(started);
+
+ butil::EndPoint internal_point;
+ ASSERT_EQ(0, butil::str2endpoint(
+ "127.0.0.1", server_opt.internal_port, &internal_point));
+ butil::fd_guard fd(butil::tcp_connect(internal_point, nullptr));
+ ASSERT_GE(fd, 0);
+ timeval tv = { 2, 0 };
+ ASSERT_EQ(0, setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)));
+
+ // Builtin services on internal_port pass authentication without any
+ // credential, which latches the connection as authenticated.
+ const std::string http_req = "GET /version HTTP/1.1\r\n\r\n";
+ ASSERT_EQ((ssize_t)http_req.size(),
+ write(fd, http_req.data(), http_req.size()));
+ char resp[1024];
+ ASSERT_GT(read(fd, resp, sizeof(resp)), 0);
+ ASSERT_EQ(0, memcmp(resp, "HTTP/1.1 200", 12));
Review Comment:
`read()` on a TCP socket is allowed to return fewer than 12 bytes even when the response is available; this assertion then calls `memcmp` on bytes beyond the returned count, which can read uninitialized data and make the regression test flaky. Accumulate reads until the 12-byte status prefix is available before comparing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] do not serve rtmp on ServerOptions.internal_port (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3622:
URL: https://github.com/apache/brpc/pull/3622#discussion_r4313682656
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -1286,6 +1286,85 @@ TEST(RtmpTest, reject_unauthenticated_connection) {
server.Join();
}
+class RejectAllAuthenticator : public brpc::Authenticator {
+public:
+ int GenerateCredential(std::string*) const override { return 0; }
+ int VerifyCredential(const std::string&, const butil::EndPoint&,
+ brpc::AuthContext*) const override { return -1; }
+};
+
+// Returns a port nothing is listening on, or -1.
+static int PickUnusedPort() {
+ butil::fd_guard sockfd(butil::tcp_listen(butil::EndPoint(butil::IP_ANY, 0)));
+ if (sockfd < 0) {
+ return -1;
+ }
+ butil::EndPoint point;
+ if (butil::get_local_side(sockfd, &point) != 0) {
+ return -1;
+ }
+ return point.port;
+}
+
+TEST(RtmpTest, not_served_on_internal_port) {
+ PlayingDummyService rtmp_service;
+ RejectAllAuthenticator auth;
+ brpc::Server server;
+ brpc::ServerOptions server_opt;
+ server_opt.rtmp_service = &rtmp_service;
+ server_opt.auth = &auth;
+ bool started = false;
+ for (int i = 0; i < 10 && !started; ++i) {
+ server_opt.internal_port = PickUnusedPort();
+ started = (server_opt.internal_port >= 0 &&
+ 0 == server.Start("127.0.0.1:0", &server_opt));
+ }
+ ASSERT_TRUE(started);
+
+ butil::EndPoint internal_point;
+ ASSERT_EQ(0, butil::str2endpoint(
+ "127.0.0.1", server_opt.internal_port, &internal_point));
+ butil::fd_guard fd(butil::tcp_connect(internal_point, nullptr));
+ ASSERT_GE(fd, 0);
+ timeval tv = { 2, 0 };
+ ASSERT_EQ(0, setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)));
+
+ // Builtin services on internal_port pass authentication without any
+ // credential, which latches the connection as authenticated.
+ const std::string http_req = "GET /version HTTP/1.1\r\n\r\n";
Review Comment:
The test sends an HTTP/1.1 request without a `Host` header, which is not valid per HTTP/1.1 and can be rejected by stricter parsers (returning 400), making the test flaky across environments. Consider adding at least `Host: 127.0.0.1` (and optionally `Connection: keep-alive`) so the “authenticate via builtin on internal_port” step is robust and spec-compliant.
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -1286,6 +1286,85 @@ TEST(RtmpTest, reject_unauthenticated_connection) {
server.Join();
}
+class RejectAllAuthenticator : public brpc::Authenticator {
+public:
+ int GenerateCredential(std::string*) const override { return 0; }
+ int VerifyCredential(const std::string&, const butil::EndPoint&,
+ brpc::AuthContext*) const override { return -1; }
+};
+
+// Returns a port nothing is listening on, or -1.
+static int PickUnusedPort() {
+ butil::fd_guard sockfd(butil::tcp_listen(butil::EndPoint(butil::IP_ANY, 0)));
+ if (sockfd < 0) {
+ return -1;
+ }
+ butil::EndPoint point;
+ if (butil::get_local_side(sockfd, &point) != 0) {
+ return -1;
+ }
+ return point.port;
+}
+
+TEST(RtmpTest, not_served_on_internal_port) {
+ PlayingDummyService rtmp_service;
+ RejectAllAuthenticator auth;
+ brpc::Server server;
+ brpc::ServerOptions server_opt;
+ server_opt.rtmp_service = &rtmp_service;
+ server_opt.auth = &auth;
+ bool started = false;
+ for (int i = 0; i < 10 && !started; ++i) {
+ server_opt.internal_port = PickUnusedPort();
+ started = (server_opt.internal_port >= 0 &&
+ 0 == server.Start("127.0.0.1:0", &server_opt));
+ }
+ ASSERT_TRUE(started);
+
+ butil::EndPoint internal_point;
+ ASSERT_EQ(0, butil::str2endpoint(
+ "127.0.0.1", server_opt.internal_port, &internal_point));
+ butil::fd_guard fd(butil::tcp_connect(internal_point, nullptr));
+ ASSERT_GE(fd, 0);
+ timeval tv = { 2, 0 };
+ ASSERT_EQ(0, setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)));
+
+ // Builtin services on internal_port pass authentication without any
+ // credential, which latches the connection as authenticated.
+ const std::string http_req = "GET /version HTTP/1.1\r\n\r\n";
+ ASSERT_EQ((ssize_t)http_req.size(),
+ write(fd, http_req.data(), http_req.size()));
Review Comment:
`write()` is not guaranteed to write the full buffer in one call; partial writes can occur, causing this test to fail even when the server behavior is correct. To avoid spurious failures, write in a loop until all bytes are sent (or use an existing helper that guarantees full writes, if available in this codebase).
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -1286,6 +1286,85 @@ TEST(RtmpTest, reject_unauthenticated_connection) {
server.Join();
}
+class RejectAllAuthenticator : public brpc::Authenticator {
+public:
+ int GenerateCredential(std::string*) const override { return 0; }
+ int VerifyCredential(const std::string&, const butil::EndPoint&,
+ brpc::AuthContext*) const override { return -1; }
+};
+
+// Returns a port nothing is listening on, or -1.
+static int PickUnusedPort() {
+ butil::fd_guard sockfd(butil::tcp_listen(butil::EndPoint(butil::IP_ANY, 0)));
+ if (sockfd < 0) {
+ return -1;
+ }
+ butil::EndPoint point;
+ if (butil::get_local_side(sockfd, &point) != 0) {
+ return -1;
+ }
+ return point.port;
+}
+
+TEST(RtmpTest, not_served_on_internal_port) {
+ PlayingDummyService rtmp_service;
+ RejectAllAuthenticator auth;
+ brpc::Server server;
+ brpc::ServerOptions server_opt;
+ server_opt.rtmp_service = &rtmp_service;
+ server_opt.auth = &auth;
+ bool started = false;
+ for (int i = 0; i < 10 && !started; ++i) {
+ server_opt.internal_port = PickUnusedPort();
+ started = (server_opt.internal_port >= 0 &&
+ 0 == server.Start("127.0.0.1:0", &server_opt));
+ }
+ ASSERT_TRUE(started);
+
+ butil::EndPoint internal_point;
+ ASSERT_EQ(0, butil::str2endpoint(
+ "127.0.0.1", server_opt.internal_port, &internal_point));
+ butil::fd_guard fd(butil::tcp_connect(internal_point, nullptr));
+ ASSERT_GE(fd, 0);
+ timeval tv = { 2, 0 };
+ ASSERT_EQ(0, setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)));
+
+ // Builtin services on internal_port pass authentication without any
+ // credential, which latches the connection as authenticated.
+ const std::string http_req = "GET /version HTTP/1.1\r\n\r\n";
+ ASSERT_EQ((ssize_t)http_req.size(),
+ write(fd, http_req.data(), http_req.size()));
+ // The reply may arrive in several segments, read until the status
+ // line prefix is complete.
+ const char status_prefix[] = "HTTP/1.1 200";
+ const size_t prefix_len = sizeof(status_prefix) - 1;
+ char resp[1024];
+ size_t nread = 0;
+ while (nread < prefix_len) {
+ const ssize_t n = read(fd, resp + nread, sizeof(resp) - nread);
+ ASSERT_GT(n, 0);
+ nread += n;
+ }
+ ASSERT_EQ(0, memcmp(resp, status_prefix, prefix_len));
+
+ // internal_port serves builtin and Tabbed services only, an RTMP
+ // handshake on the same connection must not reach rtmp_service. No
+ // protocol accepts the bytes there, so the server closes the connection
+ // and read() sees EOF. S0 (1 byte) means rtmp_service answered and -1
+ // is the receive timeout, i.e. the connection was left open; both fail.
+ char c0c1[1 + 1536];
+ memset(c0c1, 0, sizeof(c0c1));
+ c0c1[0] = 0x03;
+ ASSERT_EQ((ssize_t)sizeof(c0c1), write(fd, c0c1, sizeof(c0c1)));
Review Comment:
`write()` is not guaranteed to write the full buffer in one call; partial writes can occur, causing this test to fail even when the server behavior is correct. To avoid spurious failures, write in a loop until all bytes are sent (or use an existing helper that guarantees full writes, if available in this codebase).
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -1286,6 +1286,85 @@ TEST(RtmpTest, reject_unauthenticated_connection) {
server.Join();
}
+class RejectAllAuthenticator : public brpc::Authenticator {
+public:
+ int GenerateCredential(std::string*) const override { return 0; }
+ int VerifyCredential(const std::string&, const butil::EndPoint&,
+ brpc::AuthContext*) const override { return -1; }
+};
+
+// Returns a port nothing is listening on, or -1.
+static int PickUnusedPort() {
+ butil::fd_guard sockfd(butil::tcp_listen(butil::EndPoint(butil::IP_ANY, 0)));
+ if (sockfd < 0) {
+ return -1;
+ }
+ butil::EndPoint point;
+ if (butil::get_local_side(sockfd, &point) != 0) {
+ return -1;
+ }
+ return point.port;
+}
+
+TEST(RtmpTest, not_served_on_internal_port) {
+ PlayingDummyService rtmp_service;
+ RejectAllAuthenticator auth;
+ brpc::Server server;
+ brpc::ServerOptions server_opt;
+ server_opt.rtmp_service = &rtmp_service;
+ server_opt.auth = &auth;
+ bool started = false;
+ for (int i = 0; i < 10 && !started; ++i) {
+ server_opt.internal_port = PickUnusedPort();
+ started = (server_opt.internal_port >= 0 &&
+ 0 == server.Start("127.0.0.1:0", &server_opt));
+ }
+ ASSERT_TRUE(started);
+
+ butil::EndPoint internal_point;
+ ASSERT_EQ(0, butil::str2endpoint(
+ "127.0.0.1", server_opt.internal_port, &internal_point));
+ butil::fd_guard fd(butil::tcp_connect(internal_point, nullptr));
+ ASSERT_GE(fd, 0);
+ timeval tv = { 2, 0 };
+ ASSERT_EQ(0, setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)));
+
+ // Builtin services on internal_port pass authentication without any
+ // credential, which latches the connection as authenticated.
+ const std::string http_req = "GET /version HTTP/1.1\r\n\r\n";
+ ASSERT_EQ((ssize_t)http_req.size(),
+ write(fd, http_req.data(), http_req.size()));
+ // The reply may arrive in several segments, read until the status
+ // line prefix is complete.
+ const char status_prefix[] = "HTTP/1.1 200";
+ const size_t prefix_len = sizeof(status_prefix) - 1;
+ char resp[1024];
+ size_t nread = 0;
+ while (nread < prefix_len) {
+ const ssize_t n = read(fd, resp + nread, sizeof(resp) - nread);
+ ASSERT_GT(n, 0);
Review Comment:
The test treats any `read()` error as a hard failure without handling `EINTR`, which can create rare flakes (signals can interrupt blocking syscalls). It would be more robust to retry `read()` on `EINTR` in both the HTTP response read loop and the final EOF check while maintaining the existing timeout behavior for other errors (e.g. `EAGAIN`).
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -1286,6 +1286,85 @@ TEST(RtmpTest, reject_unauthenticated_connection) {
server.Join();
}
+class RejectAllAuthenticator : public brpc::Authenticator {
+public:
+ int GenerateCredential(std::string*) const override { return 0; }
+ int VerifyCredential(const std::string&, const butil::EndPoint&,
+ brpc::AuthContext*) const override { return -1; }
+};
+
+// Returns a port nothing is listening on, or -1.
+static int PickUnusedPort() {
+ butil::fd_guard sockfd(butil::tcp_listen(butil::EndPoint(butil::IP_ANY, 0)));
+ if (sockfd < 0) {
+ return -1;
+ }
+ butil::EndPoint point;
+ if (butil::get_local_side(sockfd, &point) != 0) {
+ return -1;
+ }
+ return point.port;
+}
+
+TEST(RtmpTest, not_served_on_internal_port) {
+ PlayingDummyService rtmp_service;
+ RejectAllAuthenticator auth;
+ brpc::Server server;
+ brpc::ServerOptions server_opt;
+ server_opt.rtmp_service = &rtmp_service;
+ server_opt.auth = &auth;
+ bool started = false;
+ for (int i = 0; i < 10 && !started; ++i) {
+ server_opt.internal_port = PickUnusedPort();
+ started = (server_opt.internal_port >= 0 &&
+ 0 == server.Start("127.0.0.1:0", &server_opt));
+ }
+ ASSERT_TRUE(started);
+
+ butil::EndPoint internal_point;
+ ASSERT_EQ(0, butil::str2endpoint(
+ "127.0.0.1", server_opt.internal_port, &internal_point));
+ butil::fd_guard fd(butil::tcp_connect(internal_point, nullptr));
+ ASSERT_GE(fd, 0);
+ timeval tv = { 2, 0 };
+ ASSERT_EQ(0, setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)));
+
+ // Builtin services on internal_port pass authentication without any
+ // credential, which latches the connection as authenticated.
+ const std::string http_req = "GET /version HTTP/1.1\r\n\r\n";
+ ASSERT_EQ((ssize_t)http_req.size(),
+ write(fd, http_req.data(), http_req.size()));
+ // The reply may arrive in several segments, read until the status
+ // line prefix is complete.
+ const char status_prefix[] = "HTTP/1.1 200";
+ const size_t prefix_len = sizeof(status_prefix) - 1;
+ char resp[1024];
+ size_t nread = 0;
+ while (nread < prefix_len) {
+ const ssize_t n = read(fd, resp + nread, sizeof(resp) - nread);
+ ASSERT_GT(n, 0);
+ nread += n;
+ }
+ ASSERT_EQ(0, memcmp(resp, status_prefix, prefix_len));
+
+ // internal_port serves builtin and Tabbed services only, an RTMP
+ // handshake on the same connection must not reach rtmp_service. No
+ // protocol accepts the bytes there, so the server closes the connection
+ // and read() sees EOF. S0 (1 byte) means rtmp_service answered and -1
+ // is the receive timeout, i.e. the connection was left open; both fail.
+ char c0c1[1 + 1536];
+ memset(c0c1, 0, sizeof(c0c1));
+ c0c1[0] = 0x03;
+ ASSERT_EQ((ssize_t)sizeof(c0c1), write(fd, c0c1, sizeof(c0c1)));
+ char s0 = 0;
+ const ssize_t n = read(fd, &s0, 1);
+ const int saved_errno = errno;
+ ASSERT_EQ(0, n) << "n=" << n << " errno=" << saved_errno;
Review Comment:
The test treats any `read()` error as a hard failure without handling `EINTR`, which can create rare flakes (signals can interrupt blocking syscalls). It would be more robust to retry `read()` on `EINTR` in both the HTTP response read loop and the final EOF check while maintaining the existing timeout behavior for other errors (e.g. `EAGAIN`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] do not serve rtmp on ServerOptions.internal_port (brpc)
Posted by "sahvx655-wq (via GitHub)" <gi...@apache.org>.
sahvx655-wq commented on code in PR #3622:
URL: https://github.com/apache/brpc/pull/3622#discussion_r4308539564
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -1286,6 +1286,71 @@ TEST(RtmpTest, reject_unauthenticated_connection) {
server.Join();
}
+class RejectAllAuthenticator : public brpc::Authenticator {
+public:
+ int GenerateCredential(std::string*) const override { return 0; }
+ int VerifyCredential(const std::string&, const butil::EndPoint&,
+ brpc::AuthContext*) const override { return -1; }
+};
+
+// Returns a port nothing is listening on, or -1.
+static int PickUnusedPort() {
+ butil::fd_guard sockfd(butil::tcp_listen(butil::EndPoint(butil::IP_ANY, 0)));
+ if (sockfd < 0) {
+ return -1;
+ }
+ butil::EndPoint point;
+ if (butil::get_local_side(sockfd, &point) != 0) {
+ return -1;
+ }
+ return point.port;
+}
+
+TEST(RtmpTest, not_served_on_internal_port) {
+ PlayingDummyService rtmp_service;
+ RejectAllAuthenticator auth;
+ brpc::Server server;
+ brpc::ServerOptions server_opt;
+ server_opt.rtmp_service = &rtmp_service;
+ server_opt.auth = &auth;
+ bool started = false;
+ for (int i = 0; i < 10 && !started; ++i) {
+ server_opt.internal_port = PickUnusedPort();
+ started = (server_opt.internal_port >= 0 &&
+ 0 == server.Start("127.0.0.1:0", &server_opt));
+ }
+ ASSERT_TRUE(started);
+
+ butil::EndPoint internal_point;
+ ASSERT_EQ(0, butil::str2endpoint(
+ "127.0.0.1", server_opt.internal_port, &internal_point));
+ butil::fd_guard fd(butil::tcp_connect(internal_point, nullptr));
+ ASSERT_GE(fd, 0);
+ timeval tv = { 2, 0 };
+ ASSERT_EQ(0, setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)));
+
+ // Builtin services on internal_port pass authentication without any
+ // credential, which latches the connection as authenticated.
+ const std::string http_req = "GET /version HTTP/1.1\r\n\r\n";
+ ASSERT_EQ((ssize_t)http_req.size(),
+ write(fd, http_req.data(), http_req.size()));
+ char resp[1024];
+ ASSERT_GT(read(fd, resp, sizeof(resp)), 0);
+ ASSERT_EQ(0, memcmp(resp, "HTTP/1.1 200", 12));
Review Comment:
Fair point, a short read there would have had memcmp looking past the bytes that actually arrived. It never showed up locally because the whole /version reply comes back in one segment on loopback, but nothing guarantees that. The test now loops on read() until the 12-byte status prefix is buffered and only then compares it, so a split reply just takes another iteration instead of a stale comparison.
The RTMP part of the test is unchanged, it still reads a single byte after sending C0C1 and expects the connection to be closed rather than an S0. Rebuilt brpc_rtmp_unittest with the change and all 32 tests pass, including not_served_on_internal_port.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] do not serve rtmp on ServerOptions.internal_port (brpc)
Posted by "sahvx655-wq (via GitHub)" <gi...@apache.org>.
sahvx655-wq commented on PR #3622:
URL: https://github.com/apache/brpc/pull/3622#issuecomment-6286014999
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org