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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
