]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Do not "open" Comm::Connection on oldAccept() errors (#1103)
authorAlex Rousskov <rousskov@measurement-factory.com>
Tue, 9 Aug 2022 03:22:48 +0000 (03:22 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Tue, 9 Aug 2022 18:53:33 +0000 (18:53 +0000)
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
src/fd.cc
src/fd.h

index 0faa640a8c2149d247c9a77138887574035d42ef..46b997e66673a0e6ad24a82bbf1e42ff18c57ed9 100644 (file)
@@ -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;
 }
 
index 02e8e1e9095bf4d1e26d11248a213af4dc913ac1..46f0e187c86023e929056a9f0e1aeeaf6f586699 100644 (file)
--- 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));
+        }
+    }
+}
+
index e3f1ba3618307193da2e7da5fc74e8bf2c6c75e9..267e7849a86bc45a2bef4891733cd2e764fba24b 100644 (file)
--- a/src/fd.h
+++ b/src/fd.h
 #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 *);