From 7185c9e7dabe76fce29ea527c52b6053e53493a5 Mon Sep 17 00:00:00 2001
From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Tue, 9 Aug 2022 03:22:48 +0000
Subject: [PATCH] Do not "open" Comm::Connection on oldAccept() errors (#1103)

When Comm::TcpAcceptor::oldAccept() discovers an error, it should close
the socket it just accepted. We should use RAII to avoid leaking the
open socket, especially in the presence of multiple error detection
areas and C++ exceptions. Using the available Connection object for
controlling socket lifetime does not work well because that Connection
object often lingers well past oldAccept() -- the object delivers
various low-level error details (e.g., the remote HTTP client address of
the failed attempt) to TcpAcceptor users.

Instead of "opening" the Connection object ASAP to avoid FD leaks and
then struggling to find the right time to close it, we now delay that
opening until oldAccept() succeeds. Meanwhile, the socket lifetime and
Comm registration are controlled by a simple RAII Descriptor object.

Eventually, we will reuse Comm::Descriptor in Connection and other
descriptor-owning code. One known prerequisite for those improvements is
Optional<> support for non-trivial types (a work in progress).
---
 src/comm/TcpAcceptor.cc | 11 +++++------
 src/fd.cc               | 19 +++++++++++++++++++
 src/fd.h                | 28 ++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/src/comm/TcpAcceptor.cc b/src/comm/TcpAcceptor.cc
index 0faa640a8c..46b997e666 100644
--- a/src/comm/TcpAcceptor.cc
+++ b/src/comm/TcpAcceptor.cc
@@ -342,12 +342,12 @@ Comm::Flag
 Comm::TcpAcceptor::oldAccept(Comm::ConnectionPointer &details)
 {
     ++statCounter.syscalls.sock.accepts;
-    int sock;
     struct addrinfo *gai = nullptr;
     Ip::Address::InitAddr(gai);
 
     errcode = 0; // reset local errno copy.
-    if ((sock = accept(conn->fd, gai->ai_addr, &gai->ai_addrlen)) < 0) {
+    const auto rawSock = accept(conn->fd, gai->ai_addr, &gai->ai_addrlen);
+    if (rawSock < 0) {
         errcode = errno; // store last accept errno locally.
 
         Ip::Address::FreeAddr(gai);
@@ -364,15 +364,12 @@ Comm::TcpAcceptor::oldAccept(Comm::ConnectionPointer &details)
         }
     }
 
-    Must(sock >= 0);
     ++incoming_sockets_accepted;
 
     // Sync with Comm ASAP so that abandoned details can properly close().
     // XXX : these are not all HTTP requests. use a note about type and ip:port details->
     // so we end up with a uniform "(HTTP|FTP-data|HTTPS|...) remote-ip:remote-port"
-    fd_open(sock, FD_SOCKET, "HTTP Request");
-    details->fd = sock;
-    details->enterOrphanage();
+    Descriptor sock(rawSock, FD_SOCKET, "HTTP Request");
 
     details->remote = *gai;
 
@@ -437,6 +434,8 @@ Comm::TcpAcceptor::oldAccept(Comm::ConnectionPointer &details)
     /* IFF the socket is (tproxy) transparent, pass the flag down to allow spoofing */
     F->flags.transparent = fd_table[conn->fd].flags.transparent; // XXX: can we remove this line yet?
 
+    details->fd = sock.release();
+    details->enterOrphanage();
     return Comm::OK;
 }
 
diff --git a/src/fd.cc b/src/fd.cc
index 02e8e1e909..46f0e187c8 100644
--- a/src/fd.cc
+++ b/src/fd.cc
@@ -12,6 +12,7 @@
 #include "comm/Loops.h"
 #include "debug/Messages.h"
 #include "debug/Stream.h"
+#include "error/SysErrorDetail.h"
 #include "fatal.h"
 #include "fd.h"
 #include "fde.h"
@@ -319,3 +320,21 @@ fdAdjustReserved(void)
     RESERVED_FD = newReserve;
 }
 
+/* Comm::Descriptor */
+
+Comm::Descriptor::Descriptor(const int fd, const unsigned int type, const char * const description): fd_(fd)
+{
+    fd_open(fd_, type, description);
+}
+
+Comm::Descriptor::~Descriptor()
+{
+    if (fd_ >= 0) {
+        fd_close(fd_);
+        if (close(fd_) != 0) {
+            const auto savedErrno = errno;
+            debugs(51, 7, "failed to close FD " << fd_ << ReportSysError(savedErrno));
+        }
+    }
+}
+
diff --git a/src/fd.h b/src/fd.h
index e3f1ba3618..267e7849a8 100644
--- a/src/fd.h
+++ b/src/fd.h
@@ -11,6 +11,34 @@
 #ifndef SQUID_FD_H_
 #define SQUID_FD_H_
 
+namespace Comm {
+
+/// An open Comm-registered file descriptor guard that, upon creation, registers
+/// the descriptor with Comm and, upon destruction, unregisters and closes the
+/// descriptor (unless the descriptor has been release()d by then).
+class Descriptor
+{
+public:
+    /// Starts owning the given FD of a given type, with a given description.
+    /// Assumes the given descriptor is open and calls legacy fd_open().
+    Descriptor(int fd, unsigned int type, const char *description);
+    Descriptor(Descriptor &&) = delete; // no copying (and, for now, moving) of any kind
+
+    /// Closes and calls legacy fd_close() unless release() was called earlier.
+    ~Descriptor();
+
+    /// A copy of the descriptor for use in system calls and such.
+    operator int() const { return fd_; }
+
+    /// Forgets the descriptor and prevents its automatic closure (by us).
+    int release() { const auto result = fd_; fd_ = -1; return result; }
+
+private:
+    int fd_;
+};
+
+} // namespace Comm
+
 void fd_close(int fd);
 void fd_open(int fd, unsigned int type, const char *);
 void fd_note(int fd, const char *);
-- 
2.39.5