HTTPServer: implement and test AcceptConnection()
What changed, and why it matters
This commit adds a new internal method for accepting incoming HTTP connections and includes unit tests for it. It is a routine code refactoring and testing change with no apparent security relevance.
No security action required; review as normal code-quality/test coverage change.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The commit introduces HTTPServer::AcceptConnection(), modeled on CConnman::AcceptConnection() in net.cpp, plus a public wrapper AcceptConnectionFromListeningSocket() intended only for unit tests. It also adds test helpers (ConnectClient) and test cases verifying that accepting from an empty queue returns null and that a queued mock client is accepted. The change is additive and test-focused.
Changed components
src/httpserver.cppsrc/httpserver.hsrc/test/httpserver_tests.cppsrc/test/util/setup_common.cppsrc/test/util/setup_common.hInspect captured patch +78 / −0
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 310907ba..0bda2843 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -976,4 +976,36 @@ void HTTPServer::StopListening()
{
m_listen.clear();
}
+
+std::unique_ptr<Sock> HTTPServer::AcceptConnection(const Sock& listen_sock, CService& addr)
+{
+ // Make sure we only operate on our own listening sockets
+ Assume(std::ranges::any_of(m_listen, [&](const auto& sock) { return sock.get() == &listen_sock; }));
+
+ sockaddr_storage storage;
+ socklen_t len{sizeof(storage)};
+ auto sa = reinterpret_cast<sockaddr*>(&storage);
+
+ auto sock{listen_sock.Accept(sa, &len)};
+
+ if (!sock) {
+ const int err{WSAGetLastError()};
+ if (err != WSAEWOULDBLOCK) {
+ LogDebug(BCLog::HTTP,
+ "Cannot accept new connection: %s",
+ NetworkErrorString(err));
+ }
+ return {};
+ }
+
+ // The OS handed us a valid socket but we can't determine its source address.
+ // In the unlikely event this occurs, the invalid address will be rejected
+ // by the downstream ClientAllowed() check.
+ if (!addr.SetSockAddr(sa, len)) {
+ LogDebug(BCLog::HTTP,
+ "Unknown socket family");
+ }
+
+ return sock;
+}
} // namespace http_bitcoin
diff --git a/src/httpserver.h b/src/httpserver.h
index ba93cbb8..e4dabc63 100644
--- a/src/httpserver.h
+++ b/src/httpserver.h
@@ -323,11 +323,29 @@ public:
*/
size_t GetListeningSocketCount() const { return m_listen.size(); }
+ /**
+ * This is a temporary method used to accept connections from a listening
+ * socket in the unit tests before the I/O loop is implemented.
+ * It will be removed in a future commit.
+ */
+ std::unique_ptr<Sock> AcceptConnectionFromListeningSocket(CService& addr)
+ {
+ return AcceptConnection(*m_listen.front(), addr);
+ }
+
private:
/**
* List of listening sockets.
*/
std::vector<std::shared_ptr<Sock>> m_listen;
+
+ /**
+ * Accept a connection.
+ * @param[in] listen_sock Socket on which to accept the connection.
+ * @param[out] addr Address of the peer that was accepted.
+ * @return Newly created socket for the accepted connection.
+ */
+ std::unique_ptr<Sock> AcceptConnection(const Sock& listen_sock, CService& addr);
};
} // namespace http_bitcoin
diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp
index 1114df05..22af3981 100644
--- a/src/test/httpserver_tests.cpp
+++ b/src/test/httpserver_tests.cpp
@@ -395,5 +395,15 @@ BOOST_AUTO_TEST_CASE(http_server_socket_tests)
BOOST_REQUIRE(server.BindAndStartListening(addr_bind));
// We are bound and listening
BOOST_REQUIRE_EQUAL(server.GetListeningSocketCount(), 1);
+
+ // Pick up the phone, there's no one there
+ CService addr_connection;
+ BOOST_REQUIRE(!server.AcceptConnectionFromListeningSocket(addr_connection));
+
+ // Create a mock client and add it to the local CreateSock queue
+ ConnectClient();
+ // Accept the connection
+ BOOST_REQUIRE(server.AcceptConnectionFromListeningSocket(addr_connection));
+ BOOST_CHECK_EQUAL(addr_connection.ToStringAddrPort(), "5.5.5.5:6789");
}
BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 2ba63efd..846f4703 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -658,6 +658,21 @@ SocketTestingSetup::~SocketTestingSetup()
CreateSock = m_create_sock_orig;
}
+void SocketTestingSetup::ConnectClient()
+{
+ // I/O pipes for a mock Connected Socket we can read and write to.
+ auto connected_socket_pipes(std::make_shared<DynSock::Pipes>());
+
+ // TODO: Insert a payload
+
+ // Create the Mock Connected Socket that represents a client.
+ // It needs I/O pipes but its queue can remain empty
+ std::unique_ptr<DynSock> connected_socket{std::make_unique<DynSock>(connected_socket_pipes)};
+
+ // Push into the queue of Accepted Sockets returned by the local CreateSock()
+ m_accepted_sockets.Push(std::move(connected_socket));
+}
+
/**
* @returns a real block (0000000000013b8ab2cd513b0261a14096412195a72a0c4827d229dcc7e0f7af)
* with 9 txs.
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 7a8f6b70..bf9ec42e 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -256,6 +256,9 @@ public:
explicit SocketTestingSetup();
~SocketTestingSetup();
+ //! Connect to the socket with a mock client (a DynSock)
+ void ConnectClient();
+
private:
//! Save the original value of CreateSock here and restore it when the test ends.
decltype(CreateSock) m_create_sock_orig;
Why this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.