Browse Source

Close the listening socket in stop() even when not serving (#2517)

* Close the listening socket in stop() even when not serving

Server::stop() released svr_sock_ only under `if (is_running_)`, and
is_running_ is set inside listen_internal(). A server that bound with
bind_to_port() / bind_to_any_port() and never reached listen_after_bind()
therefore kept its listening descriptor for the life of the process:
~Server() is defaulted and svr_sock_ is a bare atomic<socket_t>, so nothing
else closes it. The port stayed held too.

Close whenever the socket is open instead. The exchange already made this
safe against a concurrent accept loop, so dropping the is_running_ gate also
removes a TOCTOU between the check and the exchange.

* Fail listen_after_bind() when stop() already closed the socket

With stop() now releasing a bound-but-not-serving socket, a stop() that
lands between bind and listen used to slip through listen_internal():
the accept loop saw INVALID_SOCKET, never iterated, and returned success
without ever serving, pulsing is_running_ just long enough that a
wait_until_ready() caller could miss it and spin forever. Return false
instead and mark the server decommissioned the way any failed listen
does, so waiters wake up.

Also drop the assert() in stop(). It read is_running_ and svr_sock_
separately, which is exactly the race the exchange removes: a second
stop() while the accept loop is still unwinding sees is_running_ true
and svr_sock_ already INVALID_SOCKET, aborting debug builds.

The regression test checks the released port with a raw connect()
instead of a Client request: against the old code the connection is
accepted into the backlog and never answered, which would hang the test
rather than fail it.
yhirose 1 week ago
parent
commit
a691e531c3
2 changed files with 54 additions and 4 deletions
  1. 13 4
      httplib.h
  2. 41 0
      test/test.cc

+ 13 - 4
httplib.h

@@ -11644,9 +11644,11 @@ inline void Server::wait_until_ready() const {
 }
 
 inline void Server::stop() noexcept {
-  if (is_running_) {
-    assert(svr_sock_ != INVALID_SOCKET);
-    std::atomic<socket_t> sock(svr_sock_.exchange(INVALID_SOCKET));
+  // Release the listening socket whether or not the accept loop is running:
+  // bind_to_port() without listen_after_bind() still owns the descriptor. The
+  // exchange is what makes this safe to call concurrently with the accept loop.
+  socket_t sock = svr_sock_.exchange(INVALID_SOCKET);
+  if (sock != INVALID_SOCKET) {
     detail::shutdown_socket(sock);
     detail::close_socket(sock);
   }
@@ -12207,7 +12209,14 @@ inline int Server::bind_internal(const std::string &host, int port,
 }
 
 inline bool Server::listen_internal() {
-  if (is_decommissioned) { return false; }
+  // A stop() between bind and listen leaves nothing to accept on. Report
+  // failure instead of returning success without ever serving, and mark the
+  // server decommissioned the way any failed listen does so that a concurrent
+  // wait_until_ready() wakes up instead of spinning forever.
+  if (is_decommissioned || svr_sock_ == INVALID_SOCKET) {
+    is_decommissioned = true;
+    return false;
+  }
 
   auto ret = true;
   is_running_ = true;

+ 41 - 0
test/test.cc

@@ -3510,6 +3510,47 @@ TEST(BindServerTest, BindAndListenSeparately) {
   svr.stop();
 }
 
+// Reports whether anything is still listening on a loopback port. A plain
+// connect() is used instead of a Client request because a socket left bound by
+// mistake accepts the connection into its backlog and never answers, which
+// would hang the test instead of failing it.
+static bool is_loopback_port_accepting(int port) {
+  sockaddr_in addr{};
+  addr.sin_family = AF_INET;
+  addr.sin_port = htons(static_cast<uint16_t>(port));
+  addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
+
+  socket_t sock = ::socket(AF_INET, SOCK_STREAM, 0);
+  EXPECT_NE(sock, INVALID_SOCKET);
+  if (sock == INVALID_SOCKET) { return false; }
+
+  auto ret = ::connect(sock, reinterpret_cast<sockaddr *>(&addr),
+                       static_cast<socklen_t>(sizeof(addr)));
+  detail::close_socket(sock);
+  return ret == 0;
+}
+
+TEST(BindServerTest, StopClosesBoundSocketWithoutListen) {
+  Server svr;
+  auto port = svr.bind_to_any_port("127.0.0.1");
+  ASSERT_TRUE(port > 0);
+  svr.stop();
+
+  // bind_to_any_port() already called listen(2), so until stop() closed the
+  // descriptor the port kept accepting connections into the backlog. ASSERT
+  // rather than EXPECT: with the socket still open, the listen_after_bind()
+  // below blocks forever in the accept loop.
+  ASSERT_FALSE(is_loopback_port_accepting(port));
+
+  // Nothing is left to accept on, so listen_after_bind() must report failure
+  // instead of returning success without ever serving.
+  EXPECT_FALSE(svr.listen_after_bind());
+
+  // The failed listen marks the server decommissioned, so a waiter returns
+  // instead of spinning forever.
+  svr.wait_until_ready();
+}
+
 #ifdef CPPHTTPLIB_SSL_ENABLED
 TEST(BindServerTest, BindAndListenSeparatelySSL) {
   SSLServer svr(SERVER_CERT_FILE, SERVER_PRIVATE_KEY_FILE, CLIENT_CA_CERT_FILE,