[lldb] When using Socket to listen on localhost:0 on systems supporting ting ipv4 and ipv6 the second socket to initialize will not update the listening address correctly after the call to bind.
#118565
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ting ipv4 and ipv6 the second socket to initialize will not update the listening address correctly after the call to `bind`. This results in the second address listed in `Socket::GetListeningConnectionURI` to have port `:0`, which is incorrect. To fix this, correct which address is used to detect the port and update the unit tests to cover this use case. Additionally, I updated the SocketTest's to only parameterize tests that can work on ipv4 or ipv6. This means tests like `SocketTest::DecodeHostAndPort` are only run once, instead of twice since they do not change behavior based on parameters. I also included a new unit test to cover listening on `localhost:0`, validating both sockets correctly list the updated port.
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis results in the second address listed in To fix this, correct which address is used to detect the port and update the unit tests to cover this use case. Additionally, I updated the SocketTest's to only parameterize tests that can work on ipv4 or ipv6. This means tests like I also included a new unit test to cover listening on Full diff: https://github.com/llvm/llvm-project/pull/118565.diff 2 Files Affected:
diff --git a/lldb/source/Host/common/TCPSocket.cpp b/lldb/source/Host/common/TCPSocket.cpp
index d0055c3b6c44fb..d3282ab58b8185 100644
--- a/lldb/source/Host/common/TCPSocket.cpp
+++ b/lldb/source/Host/common/TCPSocket.cpp
@@ -216,11 +216,11 @@ Status TCPSocket::Listen(llvm::StringRef name, int backlog) {
}
if (host_port->port == 0) {
- socklen_t sa_len = address.GetLength();
- if (getsockname(fd, &address.sockaddr(), &sa_len) == 0)
- host_port->port = address.GetPort();
+ socklen_t sa_len = listen_address.GetLength();
+ if (getsockname(fd, &listen_address.sockaddr(), &sa_len) == 0)
+ host_port->port = listen_address.GetPort();
}
- m_listen_sockets[fd] = address;
+ m_listen_sockets[fd] = listen_address;
}
if (m_listen_sockets.empty()) {
diff --git a/lldb/unittests/Host/SocketTest.cpp b/lldb/unittests/Host/SocketTest.cpp
index a74352c19725d2..689ef4019c618c 100644
--- a/lldb/unittests/Host/SocketTest.cpp
+++ b/lldb/unittests/Host/SocketTest.cpp
@@ -35,7 +35,7 @@ class SocketTest : public testing::TestWithParam<SocketTestParams> {
}
};
-TEST_P(SocketTest, DecodeHostAndPort) {
+TEST_F(SocketTest, DecodeHostAndPort) {
EXPECT_THAT_EXPECTED(Socket::DecodeHostAndPort("localhost:1138"),
llvm::HasValue(Socket::HostAndPort{"localhost", 1138}));
@@ -63,9 +63,8 @@ TEST_P(SocketTest, DecodeHostAndPort) {
EXPECT_THAT_EXPECTED(Socket::DecodeHostAndPort("*:65535"),
llvm::HasValue(Socket::HostAndPort{"*", 65535}));
- EXPECT_THAT_EXPECTED(
- Socket::DecodeHostAndPort("[::1]:12345"),
- llvm::HasValue(Socket::HostAndPort{"::1", 12345}));
+ EXPECT_THAT_EXPECTED(Socket::DecodeHostAndPort("[::1]:12345"),
+ llvm::HasValue(Socket::HostAndPort{"::1", 12345}));
EXPECT_THAT_EXPECTED(
Socket::DecodeHostAndPort("[abcd:12fg:AF58::1]:12345"),
@@ -73,9 +72,10 @@ TEST_P(SocketTest, DecodeHostAndPort) {
}
#if LLDB_ENABLE_POSIX
-TEST_P(SocketTest, DomainListenConnectAccept) {
+TEST_F(SocketTest, DomainListenConnectAccept) {
llvm::SmallString<64> Path;
- std::error_code EC = llvm::sys::fs::createUniqueDirectory("DomainListenConnectAccept", Path);
+ std::error_code EC =
+ llvm::sys::fs::createUniqueDirectory("DomainListenConnectAccept", Path);
ASSERT_FALSE(EC);
llvm::sys::path::append(Path, "test");
@@ -88,7 +88,7 @@ TEST_P(SocketTest, DomainListenConnectAccept) {
CreateDomainConnectedSockets(Path, &socket_a_up, &socket_b_up);
}
-TEST_P(SocketTest, DomainListenGetListeningConnectionURI) {
+TEST_F(SocketTest, DomainListenGetListeningConnectionURI) {
llvm::SmallString<64> Path;
std::error_code EC =
llvm::sys::fs::createUniqueDirectory("DomainListenConnectAccept", Path);
@@ -110,7 +110,7 @@ TEST_P(SocketTest, DomainListenGetListeningConnectionURI) {
testing::ElementsAre(llvm::formatv("unix-connect://{0}", Path).str()));
}
-TEST_P(SocketTest, DomainMainLoopAccept) {
+TEST_F(SocketTest, DomainMainLoopAccept) {
llvm::SmallString<64> Path;
std::error_code EC =
llvm::sys::fs::createUniqueDirectory("DomainListenConnectAccept", Path);
@@ -270,6 +270,25 @@ TEST_P(SocketTest, TCPListen0GetListeningConnectionURI) {
.str()));
}
+TEST_F(SocketTest, TCPListen0MultiListenerGetListeningConnectionURI) {
+ if (!HostSupportsIPv6() || !HostSupportsIPv4())
+ return;
+
+ llvm::Expected<std::unique_ptr<TCPSocket>> sock =
+ Socket::TcpListen("localhost:0", 5);
+ ASSERT_THAT_EXPECTED(sock, llvm::Succeeded());
+ ASSERT_TRUE(sock.get()->IsValid());
+
+ EXPECT_THAT(sock.get()->GetListeningConnectionURI(),
+ testing::UnorderedElementsAre(
+ llvm::formatv("connection://[::1]:{0}",
+ sock->get()->GetLocalPortNumber())
+ .str(),
+ llvm::formatv("connection://[127.0.0.1]:{0}",
+ sock->get()->GetLocalPortNumber())
+ .str()));
+}
+
TEST_P(SocketTest, TCPGetConnectURI) {
std::unique_ptr<TCPSocket> socket_a_up;
std::unique_ptr<TCPSocket> socket_b_up;
@@ -297,10 +316,10 @@ TEST_P(SocketTest, UDPGetConnectURI) {
}
#if LLDB_ENABLE_POSIX
-TEST_P(SocketTest, DomainGetConnectURI) {
+TEST_F(SocketTest, DomainGetConnectURI) {
llvm::SmallString<64> domain_path;
- std::error_code EC =
- llvm::sys::fs::createUniqueDirectory("DomainListenConnectAccept", domain_path);
+ std::error_code EC = llvm::sys::fs::createUniqueDirectory(
+ "DomainListenConnectAccept", domain_path);
ASSERT_FALSE(EC);
llvm::sys::path::append(domain_path, "test");
@@ -325,7 +344,7 @@ INSTANTIATE_TEST_SUITE_P(
testing::Values(SocketTestParams{/*is_ipv6=*/false,
/*localhost_ip=*/"127.0.0.1"},
SocketTestParams{/*is_ipv6=*/true, /*localhost_ip=*/"::1"}),
- // Prints "SocketTests/SocketTest.DecodeHostAndPort/ipv4" etc. in test logs.
+ // Prints "SocketTests/SocketTest.TCPGetAddress/ipv4" etc. in test logs.
[](const testing::TestParamInfo<SocketTestParams> &info) {
return info.param.is_ipv6 ? "ipv6" : "ipv4";
});
|
|
It seems this patch break the test |
|
#118673 Should fix the fature |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This results in the second address listed in
Socket::GetListeningConnectionURIto have port:0, which is incorrect.To fix this, correct which address is used to detect the port and update the unit tests to cover this use case.
Additionally, I updated the SocketTest's to only parameterize tests that can work on ipv4 or ipv6. This means tests like
SocketTest::DecodeHostAndPortare only run once, instead of twice since they do not change behavior based on parameters.I also included a new unit test to cover listening on
localhost:0, validating both sockets correctly list the updated port.